git cl patch <url>: better handling of URLs.

* if --rietveld or --gerrit is given, parse only using appropriate
  parser.
* if url has '$HOST-review' in the beginning, assume it's Gerrit
* if type of codereview is detected based on URL, then print this
  information for the user.
  This also applies to `git cl description` but message is logged instead,
  because in '-d|--display' option git cl is supposed to print only description,
  and some tooling likely relies on this :(

R=jochen@chromium.org
BUG=706406

Change-Id: I21c9355c5334fd71db27618cda11287f75168b59
Reviewed-on: https://chromium-review.googlesource.com/473186
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>

git_cl.py

@@ -1050,7 +1050,7 @@ class _ParsedIssueNumberArgument(object):
     return self.issue is not None
 
 
-def ParseIssueNumberArgument(arg):
+def ParseIssueNumberArgument(arg, codereview=None):
   """Parses the issue argument and returns _ParsedIssueNumberArgument."""
   fail_result = _ParsedIssueNumberArgument()
 
@@ -1064,6 +1064,10 @@ def ParseIssueNumberArgument(arg):
   except ValueError:
     return fail_result
 
+  if codereview is not None:
+    parsed = _CODEREVIEW_IMPLEMENTATIONS[codereview].ParseIssueURL(parsed_url)
+    return parsed or fail_result
+
   results = {}
   for name, cls in _CODEREVIEW_IMPLEMENTATIONS.iteritems():
     parsed = cls.ParseIssueURL(parsed_url)
@@ -1074,7 +1078,11 @@ def ParseIssueNumberArgument(arg):
     return fail_result
   if len(results) == 1:
     return results.values()[0]
-  # Choose Rietveld if there are two.
+
+  if parsed_url.netloc and parsed_url.netloc.split('.')[0].endswith('-review'):
+    # This is likely Gerrit.
+    return results['gerrit']
+  # Choose Rietveld as before if URL can parsed by either.
   return results['rietveld']
 
 
@@ -4422,10 +4430,10 @@ def CMDdescription(parser, args):
 
   target_issue_arg = None
   if len(args) > 0:
-    target_issue_arg = ParseIssueNumberArgument(args[0])
+    target_issue_arg = ParseIssueNumberArgument(args[0],
+                                                options.forced_codereview)
     if not target_issue_arg.valid:
       parser.error('invalid codereview url or CL id')
-      return 1
 
   auth_config = auth.extract_auth_config_from_options(options)
 
@@ -4433,14 +4441,23 @@ def CMDdescription(parser, args):
       'auth_config': auth_config,
       'codereview': options.forced_codereview,
   }
+  detected_codereview_from_url = False
   if target_issue_arg:
     kwargs['issue'] = target_issue_arg.issue
     kwargs['codereview_host'] = target_issue_arg.hostname
+    if target_issue_arg.codereview and not options.forced_codereview:
+      detected_codereview_from_url = True
+      kwargs['codereview'] = target_issue_arg.codereview
 
   cl = Changelist(**kwargs)
-
   if not cl.GetIssue():
+    assert not detected_codereview_from_url
     DieWithError('This branch has no associated changelist.')
+
+  if detected_codereview_from_url:
+    logging.info('canonical issue/change URL: %s (type: %s)\n',
+                 cl.GetIssueURL(), target_issue_arg.codereview)
+
   description = ChangeDescription(cl.GetDescription())
 
   if options.display:
@@ -5121,7 +5138,6 @@ def CMDpatch(parser, args):
   _process_codereview_select_options(parser, options)
   auth_config = auth.extract_auth_config_from_options(options)
 
-
   if options.reapply:
     if options.newbranch:
       parser.error('--reapply works on the current branch only')
@@ -5147,6 +5163,22 @@ def CMDpatch(parser, args):
   if len(args) != 1 or not args[0]:
     parser.error('Must specify issue number or url')
 
+  target_issue_arg = ParseIssueNumberArgument(args[0],
+                                              options.forced_codereview)
+  if not target_issue_arg.valid:
+    parser.error('invalid codereview url or CL id')
+
+  cl_kwargs = {
+      'auth_config': auth_config,
+      'codereview_host': target_issue_arg.hostname,
+      'codereview': options.forced_codereview,
+  }
+  detected_codereview_from_url = False
+  if target_issue_arg.codereview and not options.forced_codereview:
+    detected_codereview_from_url = True
+    cl_kwargs['codereview'] = target_issue_arg.codereview
+    cl_kwargs['issue'] = target_issue_arg.issue
+
   # We don't want uncommitted changes mixed up with the patch.
   if git_common.is_dirty_git_tree('patch'):
     return 1
@@ -5159,7 +5191,7 @@ def CMDpatch(parser, args):
   elif not GetCurrentBranch():
     DieWithError('A branch is required to apply patch. Hint: use -b option.')
 
-  cl = Changelist(auth_config=auth_config, codereview=options.forced_codereview)
+  cl = Changelist(**cl_kwargs)
 
   if cl.IsGerrit():
     if options.reject:
@@ -5169,8 +5201,12 @@ def CMDpatch(parser, args):
     if options.directory:
       parser.error('--directory is not supported with Gerrit codereview.')
 
-  return cl.CMDPatchIssue(args[0], options.reject, options.nocommit,
-                          options.directory)
+  if detected_codereview_from_url:
+    print('canonical issue/change URL: %s (type: %s)\n' %
+          (cl.GetIssueURL(), target_issue_arg.codereview))
+
+  return cl.CMDPatchWithParsedIssue(target_issue_arg, options.reject,
+                                    options.nocommit, options.directory)
 
 
 def GetTreeStatus(url=None):
@@ -5754,7 +5790,7 @@ def CMDcheckout(parser, args):
   issue_arg = ParseIssueNumberArgument(args[0])
   if not issue_arg.valid:
     parser.error('invalid codereview url or CL id')
-    return 1
+
   target_issue = str(issue_arg.issue)
 
   def find_issues(issueprefix):

tests/git_cl_test.py

@@ -469,7 +469,9 @@ class TestParseIssueURL(unittest.TestCase):
 
   def test_ParseIssueNumberArgument(self):
     def test(arg, *args, **kwargs):
-      self._validate(git_cl.ParseIssueNumberArgument(arg), *args, **kwargs)
+      codereview_hint = kwargs.pop('hint', None)
+      self._validate(git_cl.ParseIssueNumberArgument(arg, codereview_hint),
+                     *args, **kwargs)
 
     test('123', 123)
     test('', fail=True)
@@ -479,9 +481,13 @@ class TestParseIssueURL(unittest.TestCase):
     test('ssh://chrome-review.source.com/#/c/123/4/', fail=True)
     # Rietveld.
     test('https://codereview.source.com/www123', fail=True)
-    # Matches both, but we take Rietveld now.
+    # Matches both, but we take Rietveld now by default.
     test('https://codereview.source.com/123',
          123, None, 'codereview.source.com', 'rietveld')
+    # Matches both, but we take Gerrit if specifically requested.
+    test('https://codereview.source.com/123',
+         123, None, 'codereview.source.com', 'gerrit',
+         hint='gerrit')
     # Gerrrit.
     test('https://chrome-review.source.com/c/123/4',
          123, 4, 'chrome-review.source.com', 'gerrit')
@@ -1638,7 +1644,7 @@ class TestGitCl(TestCase):
         squash=False,
         squash_mode='override_nosquash')
 
-  def test_gerrit_patch_bad_chars(self):
+  def test_gerrit_patchset_title_bad_chars(self):
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     self._run_gerrit_upload_test(
         ['-f', '-t', 'Don\'t put bad cha,.rs'],
@@ -1940,9 +1946,10 @@ class TestGitCl(TestCase):
       ]
     return calls
 
-  def _patch_common(self, is_gerrit=False, force_codereview=False,
+  def _patch_common(self, default_codereview=None, force_codereview=False,
                     new_branch=False, git_short_host='host',
-                    detect_gerrit_server=True):
+                    detect_gerrit_server=True,
+                    actual_codereview=None):
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     self.mock(git_cl._RietveldChangelistImpl, 'GetMostRecentPatchset',
               lambda x: '60001')
@@ -1955,25 +1962,43 @@ class TestGitCl(TestCase):
     else:
       self.calls = [((['git', 'symbolic-ref', 'HEAD'],), 'master')]
 
-    if not force_codereview:
-      # These calls detect codereview to use.
-      self.calls += [
-        ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
-        ((['git', 'config', 'branch.master.rietveldissue'],), CERR1),
-        ((['git', 'config', 'branch.master.gerritissue'],), CERR1),
-        ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
-      ]
-
-    if is_gerrit:
+    if default_codereview:
       if not force_codereview:
+        # These calls detect codereview to use.
+        self.calls += [
+          ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+          ((['git', 'config', 'branch.master.rietveldissue'],), CERR1),
+          ((['git', 'config', 'branch.master.gerritissue'],), CERR1),
+          ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
+        ]
+      if default_codereview == 'gerrit':
+        if not force_codereview:
+          self.calls += [
+            ((['git', 'config', 'gerrit.host'],), 'true'),
+          ]
+        if detect_gerrit_server:
+          self.calls += self._get_gerrit_codereview_server_calls(
+              'master', git_short_host=git_short_host,
+              detect_branch=not new_branch and force_codereview)
+        actual_codereview = 'gerrit'
+
+      elif default_codereview == 'rietveld':
         self.calls += [
-          ((['git', 'config', 'gerrit.host'],), 'true'),
+          ((['git', 'config', 'gerrit.host'],), CERR1),
+          ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'),
+          ((['git', 'config', 'branch.master.rietveldserver',],), CERR1),
         ]
-      if detect_gerrit_server:
-        self.calls += self._get_gerrit_codereview_server_calls(
-            'master', git_short_host=git_short_host,
-            detect_branch=not new_branch and force_codereview)
+        actual_codereview = 'rietveld'
 
+    if actual_codereview == 'rietveld':
+      self.calls += [
+        ((['git', 'rev-parse', '--show-cdup'],), ''),
+      ]
+      if not default_codereview:
+        self.calls += [
+          ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+        ]
+    elif actual_codereview == 'gerrit':
       self.calls += [
         (('GetChangeDetail', git_short_host + '-review.googlesource.com',
           '123456', ['ALL_REVISIONS', 'CURRENT_COMMIT']),
@@ -1997,16 +2022,9 @@ class TestGitCl(TestCase):
            },
          }),
       ]
-    else:
-      self.calls += [
-        ((['git', 'config', 'gerrit.host'],), CERR1),
-        ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'),
-        ((['git', 'config', 'branch.master.rietveldserver',],), CERR1),
-        ((['git', 'rev-parse', '--show-cdup'],), ''),
-      ]
 
-  def _common_patch_successful(self, new_branch=False):
-    self._patch_common(new_branch=new_branch)
+  def _common_patch_rietveld_successful(self, **kwargs):
+    self._patch_common(**kwargs)
     self.calls += [
       ((['git', 'config', 'branch.master.rietveldissue', '123456'],),
        ''),
@@ -2020,21 +2038,29 @@ class TestGitCl(TestCase):
          '(http://crrev.com/123456#ps60001)'],), ''),
     ]
 
-  def test_patch_successful(self):
-    self._common_patch_successful()
+  def test_patch_rietveld_default(self):
+    self._common_patch_rietveld_successful(default_codereview='rietveld')
     self.assertEqual(git_cl.main(['patch', '123456']), 0)
 
-  def test_patch_successful_new_branch(self):
-    self._common_patch_successful(new_branch=True)
+  def test_patch_rietveld_successful_new_branch(self):
+    self._common_patch_rietveld_successful(default_codereview='rietveld',
+                                           new_branch=True)
     self.assertEqual(git_cl.main(['patch', '-b', 'master', '123456']), 0)
 
-  def test_patch_conflict(self):
-    self._patch_common()
+  def test_patch_rietveld_guess_by_url(self):
+    self._common_patch_rietveld_successful(
+        default_codereview=None,  # It doesn't matter.
+        actual_codereview='rietveld')
+    self.assertEqual(git_cl.main(
+        ['patch', 'https://codereview.example.com/123456']), 0)
+
+  def test_patch_rietveld_conflict(self):
+    self._patch_common(default_codereview='rietveld')
     GitCheckoutMock.conflict = True
     self.assertNotEqual(git_cl.main(['patch', '123456']), 0)
 
-  def test_gerrit_patch_successful(self):
-    self._patch_common(is_gerrit=True, git_short_host='chromium',
+  def test_patch_gerrit_default(self):
+    self._patch_common(default_codereview='gerrit', git_short_host='chromium',
                        detect_gerrit_server=True)
     self.calls += [
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
@@ -2048,8 +2074,8 @@ class TestGitCl(TestCase):
     ]
     self.assertEqual(git_cl.main(['patch', '123456']), 0)
 
-  def test_patch_force_codereview(self):
-    self._patch_common(is_gerrit=True, force_codereview=True,
+  def test_patch_gerrit_force(self):
+    self._patch_common(default_codereview='gerrit', force_codereview=True,
                        git_short_host='host', detect_gerrit_server=True)
     self.calls += [
       ((['git', 'fetch', 'https://host.googlesource.com/my/repo',
@@ -2063,12 +2089,15 @@ class TestGitCl(TestCase):
     ]
     self.assertEqual(git_cl.main(['patch', '--gerrit', '123456']), 0)
 
-  def test_gerrit_patch_url_successful(self):
-    self._patch_common(is_gerrit=True, git_short_host='else',
-                       detect_gerrit_server=False)
+  def test_patch_gerrit_guess_by_url(self):
+    self._patch_common(
+        default_codereview=None,  # It doesn't matter what's default.
+        actual_codereview='gerrit',
+        git_short_host='else', detect_gerrit_server=False)
     self.calls += [
       ((['git', 'fetch', 'https://else.googlesource.com/my/repo',
          'refs/changes/56/123456/1'],), ''),
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       ((['git', 'config', 'branch.master.gerritissue', '123456'],),
        ''),
       ((['git', 'config', 'branch.master.gerritserver',
@@ -2079,32 +2108,49 @@ class TestGitCl(TestCase):
     self.assertEqual(git_cl.main(
       ['patch', 'https://else-review.googlesource.com/#/c/123456/1']), 0)
 
-  def test_gerrit_patch_conflict(self):
-    self._patch_common(is_gerrit=True, detect_gerrit_server=False,
+  def test_patch_gerrit_guess_by_url_like_rietveld(self):
+    self._patch_common(
+        default_codereview=None,  # It doesn't matter what's default.
+        actual_codereview='gerrit',
+        git_short_host='else', detect_gerrit_server=False)
+    self.calls += [
+      ((['git', 'fetch', 'https://else.googlesource.com/my/repo',
+         'refs/changes/56/123456/1'],), ''),
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'config', 'branch.master.gerritissue', '123456'],),
+       ''),
+      ((['git', 'config', 'branch.master.gerritserver',
+        'https://else-review.googlesource.com'],), ''),
+      ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
+    ]
+    self.assertEqual(git_cl.main(
+      ['patch', 'https://else-review.googlesource.com/123456/1']), 0)
+
+  def test_patch_gerrit_conflict(self):
+    self._patch_common(default_codereview='gerrit', detect_gerrit_server=True,
                        git_short_host='chromium')
     self.calls += [
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
-         'refs/changes/56/123456/1'],), ''),
+         'refs/changes/56/123456/7'],), ''),
       ((['git', 'config', 'branch.master.gerritissue', '123456'],),
        ''),
       ((['git', 'config', 'branch.master.gerritserver',
         'https://chromium-review.googlesource.com'],), ''),
-      ((['git', 'config', 'branch.master.gerritpatchset', '1'],), ''),
+      ((['git', 'config', 'branch.master.gerritpatchset', '7'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1),
       ((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],),
        SystemExitMock()),
     ]
     with self.assertRaises(SystemExitMock):
-      git_cl.main(['patch',
-                   'https://chromium-review.googlesource.com/#/c/123456/1'])
+      git_cl.main(['patch', '123456'])
 
-  def test_gerrit_patch_not_exists(self):
+  def test_patch_gerrit_not_exists(self):
     def notExists(_issue, *_, **kwargs):
       self.assertFalse(kwargs['ignore_404'])
       raise git_cl.gerrit_util.GerritError(404, '')
     self.mock(git_cl.gerrit_util, 'GetChangeDetail', notExists)
 
-    url = 'https://chromium-review.googlesource.com'
     self.calls = [
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
@@ -2112,11 +2158,17 @@ class TestGitCl(TestCase):
       ((['git', 'config', 'branch.master.gerritissue'],), CERR1),
       ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
       ((['git', 'config', 'gerrit.host'],), 'true'),
-      ((['DieWithError', 'change 123456 at ' + url + ' does not exist '
-                         'or you have no access to it'],), SystemExitMock()),
+      ((['git', 'config', 'branch.master.gerritserver'],), CERR1),
+      ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'),
+      ((['git', 'config', 'branch.master.remote'],), 'origin'),
+      ((['git', 'config', 'remote.origin.url'],),
+       'https://chromium.googlesource.com/my/repo'),
+      ((['DieWithError',
+         'change 123456 at https://chromium-review.googlesource.com does not '
+         'exist or you have no access to it'],), SystemExitMock()),
     ]
     with self.assertRaises(SystemExitMock):
-      self.assertEqual(1, git_cl.main(['patch', url + '/#/c/123456/1']))
+      self.assertEqual(1, git_cl.main(['patch', '123456']))
 
   def _checkout_calls(self):
     return [