From 768f1d88a0c5cf478f84e3706e92b7caf9c5e8c7 Mon Sep 17 00:00:00 2001 From: Andrii Shyshkalov Date: Thu, 8 Dec 2016 15:10:13 +0100 Subject: [PATCH] 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 Commit-Queue: Michael Achenbach --- git_cl.py | 69 ++++++++++++++++++---------- tests/git_cl_test.py | 107 +++++++++++++++++++++---------------------- 2 files changed, 98 insertions(+), 78 deletions(-) diff --git a/git_cl.py b/git_cl.py index c80184ce1..8b7c4ea7f 100755 --- a/git_cl.py +++ b/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: diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 1a6652e3f..6b6fa89d5 100755 --- a/tests/git_cl_test.py +++ b/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