git cl: cache GetRemoteUrl result.

Sadly, this makes maintaining one test handling covering
exceptional circumstance very complex, so it was removed.

I've decoupled test that ensures that GetRemoteUrl works from
upload codepath.

R=ehmaldonado@chromium.org

Bug: 876910
Change-Id: I39de410c72d893e73492d5c3fc8f60a6ebc4f11f
Reviewed-on: https://chromium-review.googlesource.com/1186142Reviewed-by: Ryan Tseng <hinoka@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

git_cl.py

@@ -1114,6 +1114,7 @@ class Changelist(object):
     self.cc = None
     self.more_cc = []
     self._remote = None
+    self._cached_remote_url = (False, None)  # (is_cached, value)
 
     self._codereview_impl = None
     self._codereview = None
@@ -1343,6 +1344,10 @@ class Changelist(object):
 
     Returns None if there is no remote.
     """
+    is_cached, value = self._cached_remote_url
+    if is_cached:
+      return value
+
     remote, _ = self.GetRemoteBranch()
     url = RunGit(['config', 'remote.%s.url' % remote], error_ok=True).strip()
 
@@ -1351,6 +1356,7 @@ class Changelist(object):
       url = RunGit(['config', 'remote.%s.url' % remote],
                    error_ok=True,
                    cwd=url).strip()
+    self._cached_remote_url = (True, url)
     return url
 
   def GetIssue(self):
@@ -3084,13 +3090,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
           'spaces not allowed in refspec: "%s"' % refspec_suffix)
     refspec = '%s:refs/for/%s%s' % (ref_to_push, branch, refspec_suffix)
 
-    # TODO(tandrii): hack to avoid messing with tests while resolving
-    # https://crbug.com/876910. Instead, remote_url should be cached inside this
-    # class, just like GetIssue caches issue.
-    remote_url = self.GetRemoteUrl()
     try:
       push_stdout = gclient_utils.CheckCallAndFilter(
-          ['git', 'push', remote_url, refspec],
+          ['git', 'push', self.GetRemoteUrl(), refspec],
           print_stdout=True,
           # Flush after every line: useful for seeing progress when running as
           # recipe.
@@ -3134,7 +3136,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       gerrit_util.AddReviewers(
           self._GetGerritHost(),
           gerrit_util.ChangeIdentifier(
-              self._GetGerritProject(remote_url), self.GetIssue()),
+              self._GetGerritProject(), self.GetIssue()),
           reviewers, cc,
           notify=bool(options.send_mail))
 
@@ -3147,7 +3149,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       gerrit_util.SetReview(
           self._GetGerritHost(),
           gerrit_util.ChangeIdentifier(
-              self._GetGerritProject(remote_url), self.GetIssue()),
+              self._GetGerritProject(), self.GetIssue()),
           msg='Self-approving for TBR',
           labels={'Code-Review': score})
 

tests/git_cl_test.py

@@ -1091,8 +1091,6 @@ class TestGitCl(TestCase):
         ((['git', 'config', 'branch.master.remote'],), 'origin'),
         ((['git', 'config', 'remote.origin.url'],),
          'https://chromium.googlesource.com/my/repo'),
-        ((['git', 'config', 'remote.origin.url'],),
-         'https://chromium.googlesource.com/my/repo'),
     ])
     return calls
 
@@ -1184,7 +1182,7 @@ class TestGitCl(TestCase):
                            expected_upstream_ref='origin/refs/heads/master',
                            title=None, notify=False,
                            post_amend_description=None, issue=None, cc=None,
-                           git_mirror=None, custom_cl_base=None, tbr=None):
+                           custom_cl_base=None, tbr=None):
     if post_amend_description is None:
       post_amend_description = description
     cc = cc or []
@@ -1295,25 +1293,9 @@ class TestGitCl(TestCase):
     if title:
       ref_suffix += ',m=' + title
 
-    if git_mirror is None:
-      calls += [
-          ((['git', 'config', 'remote.origin.url'],),
-           'https://chromium.googlesource.com/yyy/zzz'),
-      ]
-    else:
-      calls += [
-          ((['git', 'config', 'remote.origin.url'],),
-           '/cache/this-dir-exists'),
-          (('os.path.isdir', '/cache/this-dir-exists'),
-           True),
-          # Runs in /cache/this-dir-exists.
-          ((['git', 'config', 'remote.origin.url'],),
-           'https://chromium.googlesource.com/yyy/zzz'),
-      ]
-
     calls.append((
       (['git', 'push',
-        'https://chromium.googlesource.com/yyy/zzz',
+        'https://chromium.googlesource.com/my/repo',
         ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],),
       ('remote:\n'
        'remote: Processing changes: (\)\n'
@@ -1324,10 +1306,10 @@ class TestGitCl(TestCase):
        'remote: Processing changes: new: 1, done\n'
        'remote:\n'
        'remote: New Changes:\n'
-       'remote:   https://chromium-review.googlesource.com/#/c/yyy/zzz/+/123456'
+       'remote:   https://chromium-review.googlesource.com/#/c/my/repo/+/123456'
            ' XXX\n'
        'remote:\n'
-       'To https://chromium.googlesource.com/yyy/zzz\n'
+       'To https://chromium.googlesource.com/my/repo\n'
        ' * [new branch]      hhhh -> refs/for/refs/heads/master\n')
     ))
     if squash:
@@ -1345,7 +1327,7 @@ class TestGitCl(TestCase):
     if squash:
       calls += [
           (('AddReviewers',
-            'chromium-review.googlesource.com', 'yyy%2Fzzz~123456',
+            'chromium-review.googlesource.com', 'my%2Frepo~123456',
             sorted(reviewers),
             ['joe@example.com', 'chromium-reviews+test-more-cc@chromium.org'] +
             cc, notify), ''),
@@ -1369,7 +1351,7 @@ class TestGitCl(TestCase):
           }),
         (('SetReview',
           'chromium-review.googlesource.com',
-          'yyy%2Fzzz~123456',
+          'my%2Frepo~123456',
           'Self-approving for TBR',
           {'Code-Review': 2}, None), ''),
       ]
@@ -1389,7 +1371,6 @@ class TestGitCl(TestCase):
       post_amend_description=None,
       issue=None,
       cc=None,
-      git_mirror=None,
       fetched_status=None,
       other_cl_owner=None,
       custom_cl_base=None,
@@ -1416,13 +1397,6 @@ class TestGitCl(TestCase):
     self.mock(git_cl, 'DownloadGerritHook', lambda force: self._mocked_call(
       'DownloadGerritHook', force))
 
-    original_os_path_isdir = os.path.isdir
-    def selective_os_path_isdir_mock(path):
-      if path == '/cache/this-dir-exists':
-        return self._mocked_call('os.path.isdir', path)
-      return original_os_path_isdir(path)
-    self.mock(os.path, 'isdir', selective_os_path_isdir_mock)
-
     self.calls = self._gerrit_base_calls(
         issue=issue,
         fetched_description=description,
@@ -1439,7 +1413,7 @@ class TestGitCl(TestCase):
           expected_upstream_ref=expected_upstream_ref,
           title=title, notify=notify,
           post_amend_description=post_amend_description,
-          issue=issue, cc=cc, git_mirror=git_mirror,
+          issue=issue, cc=cc,
           custom_cl_base=custom_cl_base, tbr=tbr)
     # Uncomment when debugging.
     # print '\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls)))
@@ -1527,15 +1501,6 @@ class TestGitCl(TestCase):
         'If you proceed with upload, more than 1 CL may be created by Gerrit',
         sys.stdout.getvalue())
 
-  def test_gerrit_upload_squash_first_with_mirror(self):
-    self._run_gerrit_upload_test(
-        ['--squash'],
-        'desc\nBUG=\n\nChange-Id: 123456789',
-        [],
-        squash=True,
-        expected_upstream_ref='origin/master',
-        git_mirror='https://chromium.googlesource.com/yyy/zzz')
-
   def test_gerrit_upload_squash_reupload(self):
     description = 'desc\nBUG=\n\nChange-Id: 123456789'
     self._run_gerrit_upload_test(
@@ -1949,8 +1914,6 @@ class TestGitCl(TestCase):
     self._patch_common(default_codereview='gerrit', git_short_host='chromium',
                        detect_gerrit_server=True)
     self.calls += [
-      ((['git', 'config', 'remote.origin.url'],),
-        'https://chromium.googlesource.com/my/repo'),
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/7'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
@@ -1969,8 +1932,6 @@ class TestGitCl(TestCase):
     self._patch_common(default_codereview='gerrit', force_codereview=True,
                        git_short_host='host', detect_gerrit_server=True)
     self.calls += [
-      ((['git', 'config', 'remote.origin.url'],),
-        'https://host.googlesource.com/my/repo'),
       ((['git', 'fetch', 'https://host.googlesource.com/my/repo',
          'refs/changes/56/123456/7'],), ''),
       ((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
@@ -2059,8 +2020,6 @@ class TestGitCl(TestCase):
     self._patch_common(default_codereview='gerrit', detect_gerrit_server=True,
                        git_short_host='chromium')
     self.calls += [
-      ((['git', 'config', 'remote.origin.url'],),
-        'https://chromium.googlesource.com/my/repo'),
       ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
          'refs/changes/56/123456/7'],), ''),
       ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1),
@@ -2093,22 +2052,6 @@ class TestGitCl(TestCase):
     with self.assertRaises(SystemExitMock):
       self.assertEqual(1, git_cl.main(['patch', '123456']))
 
-  def test_patch_gerrit_wrong_repo(self):
-    self._patch_common(default_codereview='gerrit', git_short_host='chromium',
-                       detect_gerrit_server=True)
-    self.calls += [
-      ((['git', 'config', 'remote.origin.url'],),
-        'https://chromium.googlesource.com/wrong/repo'),
-      ((['DieWithError',
-         'Trying to patch a change from '
-         'https://chromium.googlesource.com/my/repo but this repo appears '
-         'to be https://chromium.googlesource.com/wrong/repo.'],),
-        SystemExitMock()),
-    ]
-    with self.assertRaises(SystemExitMock):
-      self.assertEqual(1, git_cl.main(['patch', '123456']))
-
-
   def _checkout_calls(self):
     return [
         ((['git', 'config', '--local', '--get-regexp',
@@ -3482,6 +3425,31 @@ class TestGitCl(TestCase):
         u'disapproval': False,
         u'sender': u'reviewer@example.com'})
 
+  def test_get_remote_url_with_mirror(self):
+    original_os_path_isdir = os.path.isdir
+    def selective_os_path_isdir_mock(path):
+      if path == '/cache/this-dir-exists':
+        return self._mocked_call('os.path.isdir', path)
+      return original_os_path_isdir(path)
+    self.mock(os.path, 'isdir', selective_os_path_isdir_mock)
+
+    url = 'https://chromium.googlesource.com/my/repo'
+    self.calls = [
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'config', 'branch.master.merge'],), 'master'),
+      ((['git', 'config', 'branch.master.remote'],), 'origin'),
+      ((['git', 'config', 'remote.origin.url'],),
+       '/cache/this-dir-exists'),
+      (('os.path.isdir', '/cache/this-dir-exists'),
+       True),
+      # Runs in /cache/this-dir-exists.
+      ((['git', 'config', 'remote.origin.url'],),
+       url),
+    ]
+    cl = git_cl.Changelist(codereview='gerrit', issue=1)
+    self.assertEqual(cl.GetRemoteUrl(), url)
+    self.assertEqual(cl.GetRemoteUrl(), url)  # Must be cached.
+
 
 if __name__ == '__main__':
   logging.basicConfig(