git cl creds-check: improve report and give better recommendation.

R=machenbach@chromium.org,agable@chromium.org
Bug: N/A
Change-Id: I831e9e72c8687c248022f49ea405e149538ef02a
Reviewed-on: https://chromium-review.googlesource.com/563671Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

gerrit_util.py

@@ -98,6 +98,15 @@ class CookiesAuthenticator(Authenticator):
     self.netrc = self._get_netrc()
     self.gitcookies = self._get_gitcookies()
 
+  @classmethod
+  def get_new_password_url(cls, host):
+    assert not host.startswith('http')
+    # Assume *.googlesource.com pattern.
+    parts = host.split('.')
+    if not parts[0].endswith('-review'):
+      parts[0] += '-review'
+    return 'https://%s/new-password' % ('.'.join(parts))
+
   @classmethod
   def get_new_password_message(cls, host):
     assert not host.startswith('http')

git_cl.py

@@ -3845,6 +3845,12 @@ class _GitCookiesChecker(object):
     prefix = git_host.split('.', 1)[0]
     return prefix + '-review.' + self._GOOGLESOURCE
 
+  def _get_counterpart_host(self, host):
+    assert host.endswith(self._GOOGLESOURCE)
+    git = self._canonical_git_googlesource_host(host)
+    gerrit = self._canonical_gerrit_googlesource_host(git)
+    return git if gerrit == host else gerrit
+
   def has_generic_host(self):
     """Returns whether generic .googlesource.com has been configured.
 
@@ -3870,14 +3876,14 @@ class _GitCookiesChecker(object):
 
   def get_partially_configured_hosts(self):
     return set(
-        host for host, identities_pair in
-        self._get_git_gerrit_identity_pairs().iteritems()
-        if None in identities_pair and host != '.' + self._GOOGLESOURCE)
+        (host if i1 else self._canonical_gerrit_googlesource_host(host))
+        for host, (i1, i2) in self._get_git_gerrit_identity_pairs().iteritems()
+        if None in (i1, i2) and host != '.' + self._GOOGLESOURCE)
 
   def get_conflicting_hosts(self):
     return set(
-        host for host, (i1, i2) in
-        self._get_git_gerrit_identity_pairs().iteritems()
+        host
+        for host, (i1, i2) in self._get_git_gerrit_identity_pairs().iteritems()
         if None not in (i1, i2) and i1 != i2)
 
   def get_duplicated_hosts(self):
@@ -3904,61 +3910,83 @@ class _GitCookiesChecker(object):
     return hosts
 
   @staticmethod
-  def print_hosts(hosts, extra_column_func=None):
+  def _format_hosts(hosts, extra_column_func=None):
     hosts = sorted(hosts)
     assert hosts
     if extra_column_func is None:
       extras = [''] * len(hosts)
     else:
       extras = [extra_column_func(host) for host in hosts]
-    tmpl = '    %%-%ds %%-%ds' % (max(map(len, hosts)), max(map(len, extras)))
+    tmpl = '%%-%ds    %%-%ds' % (max(map(len, hosts)), max(map(len, extras)))
+    lines = []
     for he in zip(hosts, extras):
-      print(tmpl % he)
-    print()
-
-  def find_and_report_problems(self):
-    """Returns True if there was at least one problem, else False."""
-    problems = [False]
-    def add_problem():
-      if not problems[0]:
-        print('\n\n.gitcookies problem report:\n')
-      problems[0] = True
+      lines.append(tmpl % he)
+    return lines
 
+  def _find_problems(self):
     if self.has_generic_host():
-      add_problem()
-      print('  .googlesource.com record detected\n'
-            '    Chrome Infrastructure team recommends to list full host names '
-            'explicitly.\n')
+      yield ('.googlesource.com wildcard record detected',
+             ['Chrome Infrastructure team recommends to list full host names '
+              'explicitly.'],
+             None)
 
     dups = self.get_duplicated_hosts()
     if dups:
-      add_problem()
-      print('  The following hosts were defined twice:\n')
-      self.print_hosts(dups)
+      yield ('The following hosts were defined twice',
+             self._format_hosts(dups),
+             None)
 
     partial = self.get_partially_configured_hosts()
     if partial:
-      add_problem()
-      print('  Credentials should come in pairs for Git and Gerrit hosts. '
-            'These hosts are missing:')
-      self.print_hosts(partial)
+      yield ('Credentials should come in pairs for Git and Gerrit hosts. '
+             'These hosts are missing',
+             self._format_hosts(partial, lambda host: 'but %s defined' %
+                self._get_counterpart_host(host)),
+             partial)
 
     conflicting = self.get_conflicting_hosts()
     if conflicting:
-      add_problem()
-      print('  The following Git hosts have differing credentials from their '
-            'Gerrit counterparts:\n')
-      self.print_hosts(conflicting, lambda host: '%s vs %s' %
-                       tuple(self._get_git_gerrit_identity_pairs()[host]))
+      yield ('The following Git hosts have differing credentials from their '
+             'Gerrit counterparts',
+             self._format_hosts(conflicting, lambda host: '%s vs %s' %
+                 tuple(self._get_git_gerrit_identity_pairs()[host])),
+             conflicting)
 
     wrong = self.get_hosts_with_wrong_identities()
     if wrong:
-      add_problem()
-      print('  These hosts likely use wrong identity:\n')
-      self.print_hosts(wrong, lambda host: '%s but %s recommended' %
+      yield ('These hosts likely use wrong identity',
+             self._format_hosts(wrong, lambda host: '%s but %s recommended' %
                 (self._get_git_gerrit_identity_pairs()[host][0],
-                        self._EXPECTED_HOST_IDENTITY_DOMAINS[host]))
-    return problems[0]
+                 self._EXPECTED_HOST_IDENTITY_DOMAINS[host])),
+             wrong)
+
+  def find_and_report_problems(self):
+    """Returns True if there was at least one problem, else False."""
+    found = False
+    bad_hosts = set()
+    for title, sublines, hosts in self._find_problems():
+      if not found:
+        found = True
+        print('\n\n.gitcookies problem report:\n')
+      bad_hosts.update(hosts or [])
+      print('  %s%s' % (title , (':' if sublines else '')))
+      if sublines:
+        print()
+        print('    %s' % '\n    '.join(sublines))
+      print()
+
+    if bad_hosts:
+      assert found
+      print('  You can manually remove corresponding lines in your %s file and '
+            'visit the following URLs with correct account to generate '
+            'correct credential lines:\n' %
+            gerrit_util.CookiesAuthenticator.get_gitcookies_path())
+      print('    %s' % '\n    '.join(sorted(set(
+          gerrit_util.CookiesAuthenticator().get_new_password_url(
+              self._canonical_git_googlesource_host(host))
+          for host in bad_hosts
+      ))))
+    return found
 
 
 def CMDcreds_check(parser, args):

tests/git_cl_creds_check_report.txt

 .gitcookies problem report:
 
-  .googlesource.com record detected
+  .googlesource.com wildcard record detected:
+
     Chrome Infrastructure team recommends to list full host names explicitly.
 
   The following hosts were defined twice:
@@ -8,7 +9,9 @@
     dup.googlesource.com
 
   Credentials should come in pairs for Git and Gerrit hosts. These hosts are missing:
-    partial.googlesource.com
+
+    gpartial-review.googlesource.com    but gpartial.googlesource.com defined
+    partial.googlesource.com            but partial-review.googlesource.com defined
 
   The following Git hosts have differing credentials from their Gerrit counterparts:
 
@@ -18,3 +21,11 @@
 
     chrome-internal.googlesource.com    git-example.chromium.org but google.com recommended
     chromium.googlesource.com           git-example.google.com but chromium.org recommended
+
+  You can manually remove corresponding lines in your ~/.gitcookies file and visit the following URLs with correct account to generate correct credential lines:
+
+    https://chrome-internal-review.googlesource.com/new-password
+    https://chromium-review.googlesource.com/new-password
+    https://conflict-review.googlesource.com/new-password
+    https://gpartial-review.googlesource.com/new-password
+    https://partial-review.googlesource.com/new-password

tests/git_cl_test.py

@@ -550,13 +550,15 @@ class GitCookiesCheckerTest(TestCase):
       ('dup',                    'git-example.google.com'),
       ('dup-review',             'git-example.google.com'),
       ('partial',                'git-example.google.com'),
+      ('gpartial-review',        'git-example.google.com'),
     ])
     self.assertTrue(self.c.has_generic_host())
     self.assertEqual(set(['conflict.googlesource.com']),
                      self.c.get_conflicting_hosts())
     self.assertEqual(set(['dup.googlesource.com']),
                      self.c.get_duplicated_hosts())
-    self.assertEqual(set(['partial.googlesource.com']),
+    self.assertEqual(set(['partial.googlesource.com',
+                          'gpartial-review.googlesource.com']),
                      self.c.get_partially_configured_hosts())
     self.assertEqual(set(['chromium.googlesource.com',
                           'chrome-internal.googlesource.com']),
@@ -571,12 +573,15 @@ class GitCookiesCheckerTest(TestCase):
   def test_report(self):
     self.test_analysis()
     self.mock(sys, 'stdout', StringIO.StringIO())
+    self.mock(git_cl.gerrit_util.CookiesAuthenticator, 'get_gitcookies_path',
+              classmethod(lambda _: '~/.gitcookies'))
     self.assertTrue(self.c.find_and_report_problems())
     with open(os.path.join(os.path.dirname(__file__),
                            'git_cl_creds_check_report.txt')) as f:
       expected = f.read()
     def by_line(text):
       return [l.rstrip() for l in text.rstrip().splitlines()]
+    self.maxDiff = 10000
     self.assertEqual(by_line(sys.stdout.getvalue().strip()), by_line(expected))
 
 class TestGitCl(TestCase):