git cl: use gnumbd config instead of PENDING_REF_PREFIX of codereview.settings.

BUG=chromium:642493,672043
R=machenbach@chromium.org,iannucci@chromium.org

Change-Id: I0abc31b95b1766fd5fd24c1379b538d0c5291011
Reviewed-on: https://chromium-review.googlesource.com/417259
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>

git_cl.py

@@ -87,6 +87,12 @@ Fore = colorama.Fore
 # Initialized in main()
 settings = None
 
+# Used by tests/git_cl_test.py to add extra logging.
+# Inside the weirdly failing test, add this:
+# >>> self.mock(git_cl, '_IS_BEING_TESTED', True)
+# And scroll up to see the strack trace printed.
+_IS_BEING_TESTED = False
+
 
 def DieWithError(message):
   print(message, file=sys.stderr)
@@ -1028,19 +1034,6 @@ class Settings(object):
     return RunGit(['config', param], **kwargs).strip()
 
 
-def ShouldGenerateGitNumberFooters():
-  """Decides depending on codereview.settings file in the current checkout HEAD.
-  """
-  # TODO(tandrii): this has to be removed after Rietveld is read-only.
-  # see also bugs http://crbug.com/642493 and http://crbug.com/600469.
-  cr_settings_file = FindCodereviewSettingsFile()
-  if not cr_settings_file:
-    return False
-  keyvals = gclient_utils.ParseCodereviewSettingsContent(
-      cr_settings_file.read())
-  return keyvals.get('GENERATE_GIT_NUMBER_FOOTERS', '').lower() == 'true'
-
-
 class _GitNumbererState(object):
   KNOWN_PROJECTS_WHITELIST = [
       'chromium/src',
@@ -1126,6 +1119,8 @@ class _GitNumbererState(object):
           return out.strip().splitlines()
         return []
       enabled, disabled = map(get_opts, ['enabled', 'disabled'])
+    logging.info('validator config enabled %s disabled %s refglobs for '
+                 '(this ref: %s)', enabled, disabled, ref)
 
     if cls.match_refglobs(ref, disabled):
       return False
@@ -1152,8 +1147,13 @@ class _GitNumbererState(object):
 
   def __init__(self, pending_prefix, validator_enabled):
     # TODO(tandrii): remove pending_prefix after gnumbd is no more.
+    if pending_prefix:
+      if not pending_prefix.endswith('/'):
+        pending_prefix += '/'
     self._pending_prefix = pending_prefix or None
     self._validator_enabled = validator_enabled or False
+    logging.debug('_GitNumbererState(pending: %s, validator: %s)',
+                  self._pending_prefix, self._validator_enabled)
 
   @property
   def pending_prefix(self):
@@ -2352,7 +2352,8 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
     if remote_url:
       remote, remote_branch = self.GetRemoteBranch()
       target_ref = GetTargetRef(remote, remote_branch, options.target_branch,
-                                settings.GetPendingRefPrefix())
+                                pending_prefix_check=True,
+                                remote_url=self.GetRemoteUrl())
       if target_ref:
         upload_args.extend(['--target_ref', target_ref])
 
@@ -2805,8 +2806,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     gerrit_remote = 'origin'
 
     remote, remote_branch = self.GetRemoteBranch()
+    # Gerrit will not support pending prefix at all.
     branch = GetTargetRef(remote, remote_branch, options.target_branch,
-                          pending_prefix='')
+                          pending_prefix_check=False)
 
     if options.squash:
       self._GerritCommitMsgHookCheck(offer_removal=not options.force)
@@ -4248,14 +4250,17 @@ def GenerateGerritChangeId(message):
   return 'I%s' % change_hash.strip()
 
 
-def GetTargetRef(remote, remote_branch, target_branch, pending_prefix):
+def GetTargetRef(remote, remote_branch, target_branch, pending_prefix_check,
+                 remote_url=None):
   """Computes the remote branch ref to use for the CL.
 
   Args:
     remote (str): The git remote for the CL.
     remote_branch (str): The git remote branch for the CL.
     target_branch (str): The target branch specified by the user.
-    pending_prefix (str): The pending prefix from the settings.
+    pending_prefix_check (bool): If true, determines if pending_prefix should be
+      used.
+    remote_url (str): Only used for checking if pending_prefix should be used.
   """
   if not (remote and remote_branch):
     return None
@@ -4297,9 +4302,12 @@ def GetTargetRef(remote, remote_branch, target_branch, pending_prefix):
                                           'refs/heads/')
   elif remote_branch.startswith('refs/remotes/branch-heads'):
     remote_branch = remote_branch.replace('refs/remotes/', 'refs/')
-  # If a pending prefix exists then replace refs/ with it.
-  if pending_prefix:
-    remote_branch = remote_branch.replace('refs/', pending_prefix)
+
+  if pending_prefix_check:
+    # If a pending prefix exists then replace refs/ with it.
+    state = _GitNumbererState.load(remote_url, remote_branch)
+    if state.pending_prefix:
+      remote_branch = remote_branch.replace('refs/', state.pending_prefix)
   return remote_branch
 
 
@@ -4635,11 +4643,19 @@ def SendUpstream(parser, args, cmd):
       RunGit(['cherry-pick', cherry_pick_commit])
     if cmd == 'land':
       remote, branch = cl.FetchUpstreamTuple(cl.GetBranch())
+      logging.debug('remote: %s, branch %s', remote, branch)
       mirror = settings.GetGitMirror(remote)
-      pushurl = mirror.url if mirror else remote
-      pending_prefix = settings.GetPendingRefPrefix()
+      if mirror:
+        pushurl = mirror.url
+        git_numberer = _GitNumbererState.load(pushurl, branch)
+      else:
+        pushurl = remote  # Usually, this is 'origin'.
+        git_numberer = _GitNumbererState.load(
+            RunGit(['config', 'remote.%s.url' % remote]).strip(), branch)
+
+      pending_prefix = git_numberer.pending_prefix
 
-      if ShouldGenerateGitNumberFooters():
+      if git_numberer.should_git_number:
         # TODO(tandrii): run git fetch in a loop + autorebase when there there
         # is no pending ref to push to?
         logging.debug('Adding git number footers')
@@ -4699,6 +4715,12 @@ def SendUpstream(parser, args, cmd):
         revision = re.match(
           '.*?\nCommitted r(\\d+)', output, re.DOTALL).group(1)
     logging.debug(output)
+  except:  # pylint: disable=W0702
+    if _IS_BEING_TESTED:
+      logging.exception('this is likely your ACTUAL cause of test failure.\n'
+                        + '-' * 30 + '8<' + '-' * 30)
+      logging.error('\n' + '-' * 30 + '8<' + '-' * 30 + '\n\n\n')
+    raise
   finally:
     # And then swap back to the original branch and clean up.
     RunGit(['checkout', '-q', cl.GetBranch()])
@@ -4721,7 +4743,6 @@ def SendUpstream(parser, args, cmd):
       killed = True
 
   if cl.GetIssue():
-    # TODO(tandrii): figure out story of to pending + git numberer.
     to_pending = ' to pending queue' if pushed_to_pending else ''
     viewvc_url = settings.GetViewVCUrl()
     if not to_pending:

tests/git_cl_test.py

@@ -943,9 +943,16 @@ class TestGitCl(TestCase):
         self._dcommit_calls_3())
     git_cl.main(['dcommit', '--bypass-hooks'])
 
-  def _land_rietveld_common(self, debug_stdout=False):
-    if not debug_stdout:
+  def _land_rietveld_common(self, debug=False):
+    if debug:
+      # Very useful due to finally clause in git cl land raising exceptions and
+      # shadowing real cause of failure.
+      self.mock(git_cl, '_IS_BEING_TESTED', True)
+    else:
       self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
+
+    self.mock(git_cl._GitNumbererState, 'load', classmethod(lambda _, url, ref:
+        self._mocked_call(['_GitNumbererState', url, ref])))
     self.mock(RietveldMock, 'update_description', staticmethod(
               lambda i, d: self._mocked_call(['update_description', i, d])))
     self.mock(RietveldMock, 'add_comment', staticmethod(
@@ -1012,9 +1019,14 @@ class TestGitCl(TestCase):
     ]
 
   def test_land_rietveld(self):
-    self._land_rietveld_common()
+    self._land_rietveld_common(debug=False)
     self.calls += [
-      ((['git', 'config', 'rietveld.pending-ref-prefix'],), CERR1),
+      ((['git', 'config', 'remote.origin.url'],),
+       'https://chromium.googlesource.com/infra/infra'),
+      ((['_GitNumbererState',
+         'https://chromium.googlesource.com/infra/infra',
+         'refs/heads/master'],),
+       git_cl._GitNumbererState(None, False)),
       ((['git', 'push', '--porcelain', 'origin', 'HEAD:refs/heads/master'],),
        ''),
       ((['git', 'rev-parse', 'HEAD'],), 'fake_sha_rebased'),
@@ -1032,11 +1044,16 @@ class TestGitCl(TestCase):
     git_cl.main(['land'])
 
   def test_land_rietveld_gnumbd(self):
-    self._land_rietveld_common()
+    self._land_rietveld_common(debug=False)
     self.mock(git_cl, 'WaitForRealCommit',
               lambda *a: self._mocked_call(['WaitForRealCommit'] + list(a)))
     self.calls += [
-      ((['git', 'config', 'rietveld.pending-ref-prefix'],), 'refs/pending/'),
+      ((['git', 'config', 'remote.origin.url'],),
+       'https://chromium.googlesource.com/chromium/src'),
+      ((['_GitNumbererState',
+         'https://chromium.googlesource.com/chromium/src',
+         'refs/heads/master'],),
+       git_cl._GitNumbererState('refs/pending', True)),
       ((['git', 'rev-parse', 'HEAD'],), 'fake_sha_rebased'),
       ((['git', 'retry', 'fetch', 'origin',
          '+refs/pending/heads/master:refs/git-cl/pending/heads/master'],), ''),
@@ -1067,9 +1084,7 @@ class TestGitCl(TestCase):
     git_cl.main(['land'])
 
   def test_land_rietveld_git_numberer(self):
-    self._land_rietveld_common(debug_stdout=False)
-    self.mock(git_cl, 'ShouldGenerateGitNumberFooters',
-              lambda *a: self._mocked_call(['ShouldGenerateGitNumberFooters']))
+    self._land_rietveld_common(debug=False)
 
     # Special mocks to check validity of timestamp.
     original_git_amend_head = git_cl._git_amend_head
@@ -1079,8 +1094,12 @@ class TestGitCl(TestCase):
     self.mock(git_cl, '_git_amend_head', _git_amend_head_mock)
 
     self.calls += [
-      ((['git', 'config', 'rietveld.pending-ref-prefix'],), CERR1),
-      ((['ShouldGenerateGitNumberFooters'],), True),
+      ((['git', 'config', 'remote.origin.url'],),
+       'https://chromium.googlesource.com/chromium/src'),
+      ((['_GitNumbererState',
+         'https://chromium.googlesource.com/chromium/src',
+         'refs/heads/master'],),
+       git_cl._GitNumbererState(None, True)),
 
       ((['git', 'show', '-s', '--format=%B', 'fake_ancestor_sha'],),
        'This is parent commit.\n'
@@ -1124,12 +1143,13 @@ class TestGitCl(TestCase):
     git_cl.main(['land'])
 
   def test_land_rietveld_git_numberer_bad_parent(self):
-    self._land_rietveld_common()
-    self.mock(git_cl, 'ShouldGenerateGitNumberFooters',
-              lambda *a: self._mocked_call(['ShouldGenerateGitNumberFooters']))
+    self._land_rietveld_common(debug=False)
     self.calls += [
-      ((['git', 'config', 'rietveld.pending-ref-prefix'],), CERR1),
-      ((['ShouldGenerateGitNumberFooters'],), True),
+      ((['git', 'config', 'remote.origin.url'],),
+       'https://chromium.googlesource.com/v8/v8'),
+      ((['_GitNumbererState',
+         'https://chromium.googlesource.com/v8/v8', 'refs/heads/master'],),
+       git_cl._GitNumbererState(None, True)),
 
       ((['git', 'show', '-s', '--format=%B', 'fake_ancestor_sha'],),
        'This is parent commit with no footer.'),
@@ -1142,30 +1162,6 @@ class TestGitCl(TestCase):
     self.assertEqual(cm.exception.message,
                      'Unable to infer commit position from footers')
 
-  def test_ShouldGenerateGitNumberFooters(self):
-    self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: StringIO.StringIO(
-      'GENERATE_GIT_NUMBER_FOOTERS: true\n'
-    ))
-    self.assertTrue(git_cl.ShouldGenerateGitNumberFooters())
-
-    self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: StringIO.StringIO(
-      'GENERATE_GIT_NUMBER_FOOTERS: false\n'
-    ))
-    self.assertFalse(git_cl.ShouldGenerateGitNumberFooters())
-
-    self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: StringIO.StringIO(
-      'GENERATE_GIT_NUMBER_FOOTERS: anything but true is false\n'
-    ))
-    self.assertFalse(git_cl.ShouldGenerateGitNumberFooters())
-
-    self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: StringIO.StringIO(
-      'whatever: ignored'
-    ))
-    self.assertFalse(git_cl.ShouldGenerateGitNumberFooters())
-
-    self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: None)
-    self.assertFalse(git_cl.ShouldGenerateGitNumberFooters())
-
   def test_GitNumbererState_not_whitelisted_repo(self):
     self.calls = [
         ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
@@ -1190,7 +1186,7 @@ class TestGitCl(TestCase):
     res = git_cl._GitNumbererState.load(
         remote_url='https://chromium.googlesource.com/chromium/src',
         remote_ref='refs/whatever')
-    self.assertEqual(res.pending_prefix, 'refs/pending-prefix')
+    self.assertEqual(res.pending_prefix, 'refs/pending-prefix/')
     self.assertEqual(res.should_git_number, False)
 
   def test_GitNumbererState_fail_gnumbd_and_validator(self):
@@ -1253,7 +1249,7 @@ class TestGitCl(TestCase):
     res = git_cl._GitNumbererState.load(
         remote_url='https://chromium.googlesource.com/chromium/src',
         remote_ref='refs/heads/master')
-    self.assertEqual(res.pending_prefix, 'refs/pending')
+    self.assertEqual(res.pending_prefix, 'refs/pending/')
     self.assertEqual(res.should_git_number, False)
 
     res = git_cl._GitNumbererState.load(
@@ -1715,33 +1711,33 @@ class TestGitCl(TestCase):
 
   def test_get_target_ref(self):
     # Check remote or remote branch not present.
-    self.assertEqual(None, git_cl.GetTargetRef('origin', None, 'master', None))
+    self.assertEqual(None, git_cl.GetTargetRef('origin', None, 'master', False))
     self.assertEqual(None, git_cl.GetTargetRef(None,
                                                'refs/remotes/origin/master',
-                                               'master', None))
+                                               'master', False))
 
     # Check default target refs for branches.
     self.assertEqual('refs/heads/master',
                      git_cl.GetTargetRef('origin', 'refs/remotes/origin/master',
-                                         None, None))
+                                         None, False))
     self.assertEqual('refs/heads/master',
                      git_cl.GetTargetRef('origin', 'refs/remotes/origin/lkgr',
-                                         None, None))
+                                         None, False))
     self.assertEqual('refs/heads/master',
                      git_cl.GetTargetRef('origin', 'refs/remotes/origin/lkcr',
-                                         None, None))
+                                         None, False))
     self.assertEqual('refs/branch-heads/123',
                      git_cl.GetTargetRef('origin',
                                          'refs/remotes/branch-heads/123',
-                                         None, None))
+                                         None, False))
     self.assertEqual('refs/diff/test',
                      git_cl.GetTargetRef('origin',
                                          'refs/remotes/origin/refs/diff/test',
-                                         None, None))
+                                         None, False))
     self.assertEqual('refs/heads/chrome/m42',
                      git_cl.GetTargetRef('origin',
                                          'refs/remotes/origin/chrome/m42',
-                                         None, None))
+                                         None, False))
 
     # Check target refs for user-specified target branch.
     for branch in ('branch-heads/123', 'remotes/branch-heads/123',
@@ -1749,23 +1745,26 @@ class TestGitCl(TestCase):
       self.assertEqual('refs/branch-heads/123',
                        git_cl.GetTargetRef('origin',
                                            'refs/remotes/origin/master',
-                                           branch, None))
+                                           branch, False))
     for branch in ('origin/master', 'remotes/origin/master',
                    'refs/remotes/origin/master'):
       self.assertEqual('refs/heads/master',
                        git_cl.GetTargetRef('origin',
                                            'refs/remotes/branch-heads/123',
-                                           branch, None))
+                                           branch, False))
     for branch in ('master', 'heads/master', 'refs/heads/master'):
       self.assertEqual('refs/heads/master',
                        git_cl.GetTargetRef('origin',
                                            'refs/remotes/branch-heads/123',
-                                           branch, None))
+                                           branch, False))
 
     # Check target refs for pending prefix.
+    self.mock(git_cl._GitNumbererState, 'load',
+              classmethod(lambda *_: git_cl._GitNumbererState('prefix', False)))
     self.assertEqual('prefix/heads/master',
                      git_cl.GetTargetRef('origin', 'refs/remotes/origin/master',
-                                         None, 'prefix/'))
+                                         None, True,
+                                         'https://remote.url/some.git'))
 
   def test_patch_when_dirty(self):
     # Patch when local tree is dirty