diff --git a/git_cl.py b/git_cl.py index 2646fd631..4b91f5d55 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2999,6 +2999,7 @@ class ChangeDescription(object): R_LINE = r'^[ \t]*(TBR|R)[ \t]*=[ \t]*(.*?)[ \t]*$' CC_LINE = r'^[ \t]*(CC)[ \t]*=[ \t]*(.*?)[ \t]*$' BUG_LINE = r'^[ \t]*(BUG)[ \t]*=[ \t]*(.*?)[ \t]*$' + CHERRY_PICK_LINE = r'^\(cherry picked from commit [a-fA-F0-9]{40}\)$' def __init__(self, description): self._description_lines = (description or '').strip().splitlines() @@ -3151,6 +3152,57 @@ class ChangeDescription(object): cced = [match.group(2).strip() for match in matches if match] return cleanup_list(cced) + def update_with_git_number_footers(self, parent_hash, parent_msg, dest_ref): + """Updates this commit description given the parent. + + This is essentially what Gnumbd used to do. + Consult https://goo.gl/WMmpDe for more details. + """ + assert parent_msg # No, orphan branch creation isn't supported. + assert parent_hash + assert dest_ref + parent_footer_map = git_footers.parse_footers(parent_msg) + # This will also happily parse svn-position, which GnumbD is no longer + # supporting. While we'd generate correct footers, the verifier plugin + # installed in Gerrit will block such commit (ie git push below will fail). + parent_position = git_footers.get_position(parent_footer_map) + + # Cherry-picks may have last line obscuring their prior footers, + # from git_footers perspective. This is also what Gnumbd did. + cp_line = None + if (self._description_lines and + re.match(self.CHERRY_PICK_LINE, self._description_lines[-1])): + cp_line = self._description_lines.pop() + + top_lines, _, parsed_footers = git_footers.split_footers(self.description) + + # Original-ify all Cr- footers, to avoid re-lands, cherry-picks, or just + # user interference with actual footers we'd insert below. + for i, (k, v) in enumerate(parsed_footers): + if k.startswith('Cr-'): + parsed_footers[i] = (k.replace('Cr-', 'Cr-Original-'), v) + + # Add Position and Lineage footers based on the parent. + lineage = parent_footer_map.get('Cr-Branched-From', []) + if parent_position[0] == dest_ref: + # Same branch as parent. + number = int(parent_position[1]) + 1 + else: + number = 1 # New branch, and extra lineage. + lineage.insert(0, '%s-%s@{#%d}' % (parent_hash, parent_position[0], + int(parent_position[1]))) + + parsed_footers.append(('Cr-Commit-Position', + '%s@{#%d}' % (dest_ref, number))) + parsed_footers.extend(('Cr-Branched-From', v) for v in lineage) + + self._description_lines = top_lines + if cp_line: + self._description_lines.append(cp_line) + if self._description_lines[-1] != '': + self._description_lines.append('') # Ensure footer separator. + self._description_lines.extend('%s: %s' % kv for kv in parsed_footers) + def get_approving_reviewers(props): """Retrieves the reviewers that approved a CL from the issue properties with @@ -4441,6 +4493,21 @@ def SendUpstream(parser, args, cmd): mirror = settings.GetGitMirror(remote) pushurl = mirror.url if mirror else remote pending_prefix = settings.GetPendingRefPrefix() + + if ShouldGenerateGitNumberFooters(): + # 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') + parent_msg = RunGit(['show', '-s', '--format=%B', merge_base]).strip() + commit_desc.update_with_git_number_footers(merge_base, parent_msg, + branch) + # TODO(tandrii): timestamp handling is missing here. + RunGitSilent(['commit', '--amend', '-m', commit_desc.description]) + change_desc = ChangeDescription(commit_desc.description) + # If gnumbd is sitll ON and we ultimately push to branch with + # pending_prefix, gnumbd will modify footers we've just inserted with + # 'Original-', which is annoying but still technically correct. + if not pending_prefix or branch.startswith(pending_prefix): # If not using refs/pending/heads/* at all, or target ref is already set # to pending, then push to the target ref directly. @@ -4507,6 +4574,7 @@ 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 6d2290641..8c397ec02 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -271,6 +271,143 @@ class TestGitClBasic(unittest.TestCase): self.assertEqual(f('v8', 'chromium:123,456,v8:123'), ['v8:456', 'chromium:123', 'v8:123']) + def _test_git_number(self, parent_msg, dest_ref, child_msg, + parent_hash='parenthash'): + desc = git_cl.ChangeDescription(child_msg) + desc.update_with_git_number_footers(parent_hash, parent_msg, dest_ref) + return desc.description + + def assertEqualByLine(self, actual, expected): + self.assertEqual(actual.splitlines(), expected.splitlines()) + + def test_git_number_bad_parent(self): + with self.assertRaises(ValueError): + self._test_git_number('Parent', 'refs/heads/master', 'Child') + + def test_git_number_bad_parent_footer(self): + with self.assertRaises(AssertionError): + self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: wrong', + 'refs/heads/master', 'Child') + + def test_git_number_bad_lineage_ignored(self): + actual = self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#1}\n' + 'Cr-Branched-From: mustBeReal40CharHash-branch@{#pos}', + 'refs/heads/master', 'Child') + self.assertEqualByLine( + actual, + 'Child\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#2}\n' + 'Cr-Branched-From: mustBeReal40CharHash-branch@{#pos}') + + def test_git_number_same_branch(self): + actual = self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#12}', + dest_ref='refs/heads/master', + child_msg='Child') + self.assertEqualByLine( + actual, + 'Child\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#13}') + + def test_git_number_same_branch_with_originals(self): + actual = self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#12}', + dest_ref='refs/heads/master', + child_msg='Child\n' + '\n' + 'Some users are smart and insert their own footers\n' + '\n' + 'Cr-Whatever: value\n' + 'Cr-Commit-Position: refs/copy/paste@{#22}') + self.assertEqualByLine( + actual, + 'Child\n' + '\n' + 'Some users are smart and insert their own footers\n' + '\n' + 'Cr-Original-Whatever: value\n' + 'Cr-Original-Commit-Position: refs/copy/paste@{#22}\n' + 'Cr-Commit-Position: refs/heads/master@{#13}') + + def test_git_number_new_branch(self): + actual = self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#12}', + dest_ref='refs/heads/branch', + child_msg='Child') + self.assertEqualByLine( + actual, + 'Child\n' + '\n' + 'Cr-Commit-Position: refs/heads/branch@{#1}\n' + 'Cr-Branched-From: parenthash-refs/heads/master@{#12}') + + def test_git_number_lineage(self): + actual = self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: refs/heads/branch@{#1}\n' + 'Cr-Branched-From: somehash-refs/heads/master@{#12}', + dest_ref='refs/heads/branch', + child_msg='Child') + self.assertEqualByLine( + actual, + 'Child\n' + '\n' + 'Cr-Commit-Position: refs/heads/branch@{#2}\n' + 'Cr-Branched-From: somehash-refs/heads/master@{#12}') + + def test_git_number_moooooooore_lineage(self): + actual = self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: refs/heads/branch@{#5}\n' + 'Cr-Branched-From: somehash-refs/heads/master@{#12}', + dest_ref='refs/heads/mooore', + child_msg='Child') + self.assertEqualByLine( + actual, + 'Child\n' + '\n' + 'Cr-Commit-Position: refs/heads/mooore@{#1}\n' + 'Cr-Branched-From: parenthash-refs/heads/branch@{#5}\n' + 'Cr-Branched-From: somehash-refs/heads/master@{#12}') + + + def test_git_number_cherry_pick(self): + actual = self._test_git_number( + 'Parent\n' + '\n' + 'Cr-Commit-Position: refs/heads/branch@{#1}\n' + 'Cr-Branched-From: somehash-refs/heads/master@{#12}', + dest_ref='refs/heads/branch', + child_msg='Child, which is cherry-pick from master\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#100}\n' + '(cherry picked from commit deadbeef12345678deadbeef12345678deadbeef)') + self.assertEqualByLine( + actual, + 'Child, which is cherry-pick from master\n' + '\n' + '(cherry picked from commit deadbeef12345678deadbeef12345678deadbeef)\n' + '\n' + 'Cr-Original-Commit-Position: refs/heads/master@{#100}\n' + 'Cr-Commit-Position: refs/heads/branch@{#2}\n' + 'Cr-Branched-From: somehash-refs/heads/master@{#12}') + class TestGitCl(TestCase): def setUp(self): @@ -928,6 +1065,69 @@ class TestGitCl(TestCase): ] git_cl.main(['land']) + def test_land_rietveld_git_numberer(self): + self._land_rietveld_common() + self.mock(git_cl, 'ShouldGenerateGitNumberFooters', + lambda *a: self._mocked_call(['ShouldGenerateGitNumberFooters'])) + self.calls += [ + ((['git', 'config', 'rietveld.pending-ref-prefix'],), CERR1), + ((['ShouldGenerateGitNumberFooters'],), True), + + ((['git', 'show', '-s', '--format=%B', 'fake_ancestor_sha'],), + 'This is parent commit.\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#543}\n' + 'Cr-Branched-From: refs/svn/2014@{#2208}'), + + ((['git', 'commit', '--amend', '-m', + 'Issue: 123\n\nR=john@chromium.org\n' + '\n' + 'Review URL: https://codereview.chromium.org/123 .\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#544}\n' + 'Cr-Branched-From: refs/svn/2014@{#2208}'],), ''), + + ((['git', 'push', '--porcelain', 'origin', 'HEAD:refs/heads/master'],), + ''), + ((['git', 'rev-parse', 'HEAD'],), 'fake_sha_rebased'), + ((['git', 'checkout', '-q', 'feature'],), ''), + ((['git', 'branch', '-D', 'git-cl-commit'],), ''), + ((['git', 'config', 'rietveld.viewvc-url'],), + 'https://chromium.googlesource.com/infra/infra/+/'), + ((['update_description', 123, + 'Issue: 123\n\nR=john@chromium.org\n' + '\n' + 'Review URL: https://codereview.chromium.org/123 .\n' + '\n' + 'Cr-Commit-Position: refs/heads/master@{#544}\n' + 'Cr-Branched-From: refs/svn/2014@{#2208}\n' + 'Committed: ' + 'https://chromium.googlesource.com/infra/infra/+/fake_sha_rebased'],), + ''), + ((['add_comment', 123, 'Committed patchset #2 (id:20001) manually as ' + 'fake_sha_rebased (presubmit successful).'],), ''), + ] + 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.calls += [ + ((['git', 'config', 'rietveld.pending-ref-prefix'],), CERR1), + ((['ShouldGenerateGitNumberFooters'],), True), + + ((['git', 'show', '-s', '--format=%B', 'fake_ancestor_sha'],), + 'This is parent commit with no footer.'), + + ((['git', 'checkout', '-q', 'feature'],), ''), + ((['git', 'branch', '-D', 'git-cl-commit'],), ''), + ] + with self.assertRaises(ValueError) as cm: + git_cl.main(['land']) + 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'