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>
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index 9c179c6..70cb28d 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -469,7 +469,9 @@
 
   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 @@
     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 @@
         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 @@
       ]
     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 @@
     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', 'config', 'gerrit.host'],), 'true'),
+          ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+          ((['git', 'config', 'branch.master.rietveldissue'],), CERR1),
+          ((['git', 'config', 'branch.master.gerritissue'],), CERR1),
+          ((['git', 'config', 'rietveld.autoupdate'],), 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)
+      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'],), CERR1),
+          ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'),
+          ((['git', 'config', 'branch.master.rietveldserver',],), CERR1),
+        ]
+        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 @@
            },
          }),
       ]
-    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 @@
          '(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 @@
     ]
     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 @@
     ]
     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 @@
     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 @@
       ((['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 [