git cl: implement git number footer generation.

BUG=642493

Change-Id: Ic8eb121b0ad7adcc7a3f3f1967ef2261f415e731
Reviewed-on: https://chromium-review.googlesource.com/414466Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>

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:

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'