git cl land for not Gerrit: add autorebasing.

Bug: 682934
Tested: https://chromium.googlesource.com/infra/experimental/+log/aef8ad..0378e4
Adapted-from: https://chromium-review.googlesource.com/c/431976/
Change-Id: Ida8141bc92d7501d8e17a1b8571033180f02a7b3
Reviewed-on: https://chromium-review.googlesource.com/458353
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

git_cl.py

@@ -964,6 +964,7 @@ def _is_git_numberer_enabled(remote_url, remote_ref):
       'chromium/src',
       'external/webrtc',
       'v8/v8',
+      'infra/experimental',
   ]
 
   assert remote_ref and remote_ref.startswith('refs/'), remote_ref
@@ -4687,6 +4688,11 @@ def CMDdcommit(parser, args):
   return 1
 
 
+# Two special branches used by git cl land.
+MERGE_BRANCH = 'git-cl-commit'
+CHERRY_PICK_BRANCH = 'git-cl-cherry-pick'
+
+
 @subcommand.usage('[upstream branch to apply against]')
 def CMDland(parser, args):
   """Commits the current changelist via git.
@@ -4840,9 +4846,7 @@ def CMDland(parser, args):
   # We want to squash all this branch's commits into one commit with the proper
   # description. We do this by doing a "reset --soft" to the base branch (which
   # keeps the working copy the same), then landing that.
-  MERGE_BRANCH = 'git-cl-commit'
-  CHERRY_PICK_BRANCH = 'git-cl-cherry-pick'
-  # Delete the branches if they exist.
+  # Delete the special branches if they exist.
   for branch in [MERGE_BRANCH, CHERRY_PICK_BRANCH]:
     showref_cmd = ['show-ref', '--quiet', '--verify', 'refs/heads/%s' % branch]
     result = RunGitWithCode(showref_cmd)
@@ -4883,24 +4887,13 @@ def CMDland(parser, args):
       git_numberer_enabled = _is_git_numberer_enabled(
           RunGit(['config', 'remote.%s.url' % remote]).strip(), branch)
 
-    if git_numberer_enabled:
-      # TODO(tandrii): maybe do autorebase + retry on failure
-      # http://crbug.com/682934, but better just use Gerrit :)
-      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)
-      # Ensure timestamps are monotonically increasing.
-      timestamp = max(1 + _get_committer_timestamp(merge_base),
-                      _get_committer_timestamp('HEAD'))
-      _git_amend_head(commit_desc.description, timestamp)
-      change_desc = ChangeDescription(commit_desc.description)
-
-    retcode, output = RunGitWithCode(
-        ['push', '--porcelain', pushurl, 'HEAD:%s' % branch])
+    retcode = PushToGitWithAutoRebase(
+        pushurl, branch, commit_desc.description, git_numberer_enabled)
     if retcode == 0:
       revision = RunGit(['rev-parse', 'HEAD']).strip()
-    logging.debug(output)
+      if git_numberer_enabled:
+        change_desc = ChangeDescription(
+            RunGit(['show', '-s', '--format=%B', 'HEAD']).strip())
   except:  # pylint: disable=bare-except
     if _IS_BEING_TESTED:
       logging.exception('this is likely your ACTUAL cause of test failure.\n'
@@ -4911,6 +4904,7 @@ def CMDland(parser, args):
     # And then swap back to the original branch and clean up.
     RunGit(['checkout', '-q', cl.GetBranch()])
     RunGit(['branch', '-D', MERGE_BRANCH])
+    RunGit(['branch', '-D', CHERRY_PICK_BRANCH], error_ok=True)
 
   if not revision:
     print('Failed to push. If this persists, please file a bug.')
@@ -4943,6 +4937,75 @@ def CMDland(parser, args):
   return 0
 
 
+def PushToGitWithAutoRebase(remote, branch, original_description,
+                            git_numberer_enabled, max_attempts=3):
+  """Pushes current HEAD commit on top of remote's branch.
+
+  Attempts to fetch and autorebase on push failures.
+  Adds git number footers on the fly.
+
+  Returns integer code from last command.
+  """
+  cherry = RunGit(['rev-parse', 'HEAD']).strip()
+  code = 0
+  attempts_left = max_attempts
+  while attempts_left:
+    attempts_left -= 1
+    print('Attempt %d of %d' % (max_attempts - attempts_left, max_attempts))
+
+    # Fetch remote/branch into local cherry_pick_branch, overriding the latter.
+    # If fetch fails, retry.
+    print('Fetching %s/%s...' % (remote, branch))
+    code, out = RunGitWithCode(
+        ['retry', 'fetch', remote,
+         '+%s:refs/heads/%s' % (branch, CHERRY_PICK_BRANCH)])
+    if code:
+      print('Fetch failed with exit code %d.' % code)
+      print(out.strip())
+      continue
+
+    print('Cherry-picking commit on top of latest %s' % branch)
+    RunGitWithCode(['checkout', 'refs/heads/%s' % CHERRY_PICK_BRANCH],
+                   suppress_stderr=True)
+    parent_hash = RunGit(['rev-parse', 'HEAD']).strip()
+    code, out = RunGitWithCode(['cherry-pick', cherry])
+    if code:
+      print('Your patch doesn\'t apply cleanly to \'%s\' HEAD @ %s, '
+            'the following files have merge conflicts:' %
+            (branch, parent_hash))
+      print(RunGit(['diff', '--name-status', '--diff-filter=U']).strip())
+      print('Please rebase your patch and try again.')
+      RunGitWithCode(['cherry-pick', '--abort'])
+      break
+
+    commit_desc = ChangeDescription(original_description)
+    if git_numberer_enabled:
+      logging.debug('Adding git number footers')
+      parent_msg = RunGit(['show', '-s', '--format=%B', parent_hash]).strip()
+      commit_desc.update_with_git_number_footers(parent_hash, parent_msg,
+                                                 branch)
+      # Ensure timestamps are monotonically increasing.
+      timestamp = max(1 + _get_committer_timestamp(parent_hash),
+                      _get_committer_timestamp('HEAD'))
+      _git_amend_head(commit_desc.description, timestamp)
+
+    code, out = RunGitWithCode(
+        ['push', '--porcelain', remote, 'HEAD:%s' % branch])
+    print(out)
+    if code == 0:
+      break
+    if IsFatalPushFailure(out):
+      print('Fatal push error. Make sure your .netrc credentials and git '
+            'user.email are correct and you have push access to the repo.')
+      break
+  return code
+
+
+def IsFatalPushFailure(push_stdout):
+  """True if retrying push won't help."""
+  return '(prohibited by Gerrit)' in push_stdout
+
+
 @subcommand.usage('<patch url or issue id or issue url>')
 def CMDpatch(parser, args):
   """Patches in a code review."""

tests/git_cl_test.py

@@ -957,7 +957,13 @@ class TestGitCl(TestCase):
         'desc\n\nBUG=500658\nBUG=proj:1234',
         [])
 
-  def _land_rietveld_common(self, debug=False):
+  def _land_rietveld_calls(self, repo_url, git_numberer_enabled=False,
+                            parent_msg=None,
+                            new_msg=None,
+                            fail_after_parent_msg=False,
+                            fail_cherry_pick=False,
+                            fail_push_count=0,
+                            debug=False):
     if debug:
       # Very useful due to finally clause in git cl land raising exceptions and
       # shadowing real cause of failure.
@@ -965,6 +971,14 @@ class TestGitCl(TestCase):
     else:
       self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
 
+    if git_numberer_enabled:
+      # Special mocks to check validity of timestamp.
+      original_git_amend_head = git_cl._git_amend_head
+      def _git_amend_head_mock(msg, tstamp):
+        self._mocked_call(['git_amend_head committer timestamp', tstamp])
+        return original_git_amend_head(msg, tstamp)
+      self.mock(git_cl, '_git_amend_head', _git_amend_head_mock)
+
     self.mock(git_cl, '_is_git_numberer_enabled', lambda url, ref:
         self._mocked_call('_is_git_numberer_enabled', url, ref))
     self.mock(RietveldMock, 'update_description', staticmethod(
@@ -1023,76 +1037,126 @@ class TestGitCl(TestCase):
       ((['git', 'config', 'branch.feature.merge'],), 'refs/heads/master'),
       ((['git', 'config', 'branch.feature.remote'],), 'origin'),
       ((['git', 'config', '--get', 'remote.origin.url'],),
-       'https://chromium.googlesource.com/infra/infra'),
+       repo_url),
+
+      ((['git', 'config', 'remote.origin.url'],), repo_url),
+      (('_is_git_numberer_enabled', repo_url, 'refs/heads/master'),
+       git_numberer_enabled),
+
+      # Auto-rebase init.
+      ((['git', 'rev-parse', 'HEAD'],), 'sha1_for_cherry_pick'),
     ]
+    # Auto-rebase loops.
+    for attempt in xrange(fail_push_count + 1):
+      self.calls += [
+        # Auto-rebase loop start.
+        ((['git', 'retry', 'fetch', 'origin',
+          '+refs/heads/master:refs/heads/git-cl-cherry-pick'],), ''),
+        ((['git', 'checkout', 'refs/heads/git-cl-cherry-pick'],), ''),
+        # NOTE: if fail_push_count>0, this sha1_of_parent should probably be
+        # different in each loop iteration for truly realistic test.
+        ((['git', 'rev-parse', 'HEAD'],), 'sha1_of_parent'),
+        ((['git', 'cherry-pick', 'sha1_for_cherry_pick'],),
+         callError(1, 'failed to cherry pick') if fail_cherry_pick
+         else ''),
+      ]
+      if fail_cherry_pick:
+        return
+
+      if git_numberer_enabled:
+        assert parent_msg
+        self.calls += [
+          ((['git', 'show', '-s', '--format=%B', 'sha1_of_parent'],),
+           parent_msg),
+        ]
+        if fail_after_parent_msg:
+          return
+        assert new_msg
+        self.calls += [
+          ((['git', 'show', '-s', '--format=%ct', 'sha1_of_parent'],),
+           '1480022355'),  # Committer's unix timestamp.
+          ((['git', 'show', '-s', '--format=%ct', 'HEAD'],),
+           '1480024000'),
+          ((['git_amend_head committer timestamp', 1480024000],), ''),
+          ((['git', 'commit', '--amend', '-m', new_msg],), ''),
+        ]
+      self.calls += [
+        ((['git', 'push', '--porcelain', 'origin', 'HEAD:refs/heads/master'],),
+         CERR1 if attempt < fail_push_count else ''),
+      ]
+    # End of autorebase + push.
 
-  def test_land_rietveld(self):
-    self._land_rietveld_common(debug=False)
     self.calls += [
-      ((['git', 'config', 'remote.origin.url'],),
-       'https://chromium.googlesource.com/infra/infra'),
-      (('_is_git_numberer_enabled',
-        'https://chromium.googlesource.com/infra/infra',
-        'refs/heads/master'),
-       False),
-      ((['git', 'push', '--porcelain', 'origin', 'HEAD:refs/heads/master'],),
-       ''),
-      ((['git', 'rev-parse', 'HEAD'],), 'fake_sha_rebased'),
+      ((['git', 'rev-parse', 'HEAD'],), 'sha1_committed'),
+    ]
+    if git_numberer_enabled:
+      self.calls += [
+        ((['git', 'show', '-s', '--format=%B', 'HEAD'],), new_msg),
+      ]
+
+    # Cleanup calls.
+    self.calls += [
       ((['git', 'checkout', '-q', 'feature'],), ''),
       ((['git', 'branch', '-D', 'git-cl-commit'],), ''),
+      ((['git', 'branch', '-D', 'git-cl-cherry-pick'],), ''),
+    ]
+
+  def test_land_rietveld(self):
+    self._land_rietveld_calls(
+        repo_url='https://chromium.googlesource.com/infra/infra',
+        git_numberer_enabled=False,
+        debug=False)
+    self.calls += [
       ((['git', 'config', 'rietveld.viewvc-url'],),
        'https://chromium.googlesource.com/infra/infra/+/'),
       ((['update_description', 123,
          'Issue: 123\n\nR=john@chromium.org\n\nCommitted: '
-         'https://chromium.googlesource.com/infra/infra/+/fake_sha_rebased'],),
+         'https://chromium.googlesource.com/infra/infra/+/sha1_committed'],),
        ''),
       ((['add_comment', 123, 'Committed patchset #2 (id:20001) manually as '
-                             'fake_sha_rebased (presubmit successful).'],), ''),
+                             'sha1_committed (presubmit successful).'],), ''),
     ]
     git_cl.main(['land'])
 
-  def test_land_rietveld_git_numberer(self):
-    self._land_rietveld_common(debug=False)
-
-    # Special mocks to check validity of timestamp.
-    original_git_amend_head = git_cl._git_amend_head
-    def _git_amend_head_mock(msg, tstamp):
-      self._mocked_call(['git_amend_head committer timestamp', tstamp])
-      return original_git_amend_head(msg, tstamp)
-    self.mock(git_cl, '_git_amend_head', _git_amend_head_mock)
-
+  def test_land_rietveld_fail_cherry_pick(self):
+    self._land_rietveld_calls(
+        repo_url='https://chromium.googlesource.com/infra/infra',
+        git_numberer_enabled=False,
+        fail_cherry_pick=True,
+        debug=False)
     self.calls += [
-      ((['git', 'config', 'remote.origin.url'],),
-       'https://chromium.googlesource.com/chromium/src'),
-      (('_is_git_numberer_enabled',
-        'https://chromium.googlesource.com/chromium/src',
-        'refs/heads/master'),
-       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', 'show', '-s', '--format=%ct', 'fake_ancestor_sha'],),
-       '1480022355'),  # Committer's unix timestamp.
-      ((['git', 'show', '-s', '--format=%ct', 'HEAD'],),
-       '1480024000'),
-
-      ((['git_amend_head committer timestamp', 1480024000],), None),
-      ((['git', 'commit', '--amend', '-m',
-        'Issue: 123\n\nR=john@chromium.org\n'
-        '\n'
-        'Review-Url: https://codereview.chromium.org/123 .\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', 'diff', '--name-status', '--diff-filter=U'],),
+       'U       path/file1\n'
+       'U       file2.cpp\n'),
+      ((['git', 'cherry-pick', '--abort'],), ''),
       ((['git', 'checkout', '-q', 'feature'],), ''),
       ((['git', 'branch', '-D', 'git-cl-commit'],), ''),
+      ((['git', 'branch', '-D', 'git-cl-cherry-pick'],), ''),
+    ]
+    git_cl.main(['land'])
+
+  def test_land_rietveld_git_numberer_fail_1_push(self):
+    return self.test_land_rietveld_git_numberer(fail_push_count=1)
+
+  def test_land_rietveld_git_numberer(self, fail_push_count=0):
+    self._land_rietveld_calls(
+        repo_url='https://chromium.googlesource.com/chromium/src',
+        git_numberer_enabled=True,
+        parent_msg=('This is parent commit.\n'
+                    '\n'
+                    'Cr-Commit-Position: refs/heads/master@{#543}\n'
+                    'Cr-Branched-From: refs/svn/2014@{#2208}'),
+        new_msg=('Issue: 123\n\nR=john@chromium.org\n'
+                 '\n'
+                 'Review-Url: https://codereview.chromium.org/123 .\n'
+                 'Cr-Commit-Position: refs/heads/master@{#544}\n'
+                 'Cr-Branched-From: refs/svn/2014@{#2208}'),
+        fail_push_count=fail_push_count,
+        debug=False)
+
+    self.calls += [
       ((['git', 'config', 'rietveld.viewvc-url'],),
-       'https://chromium.googlesource.com/infra/infra/+/'),
+       'https://chromium.googlesource.com/chromium/src/+/'),
       ((['update_description', 123,
          'Issue: 123\n\nR=john@chromium.org\n'
          '\n'
@@ -1100,27 +1164,25 @@ class TestGitCl(TestCase):
          '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'],),
+         'https://chromium.googlesource.com/chromium/src/+/sha1_committed'],),
        ''),
       ((['add_comment', 123, 'Committed patchset #2 (id:20001) manually as '
-                             'fake_sha_rebased (presubmit successful).'],), ''),
+                             'sha1_committed (presubmit successful).'],), ''),
     ]
     git_cl.main(['land'])
 
   def test_land_rietveld_git_numberer_bad_parent(self):
-    self._land_rietveld_common(debug=False)
+    self._land_rietveld_calls(
+        repo_url='https://chromium.googlesource.com/v8/v8',
+        git_numberer_enabled=True,
+        parent_msg='This is parent commit with no footer.',
+        fail_after_parent_msg=True,
+        debug=False)
     self.calls += [
-      ((['git', 'config', 'remote.origin.url'],),
-       'https://chromium.googlesource.com/v8/v8'),
-      (('_is_git_numberer_enabled',
-        'https://chromium.googlesource.com/v8/v8', 'refs/heads/master'),
-       True),
-
-      ((['git', 'show', '-s', '--format=%B', 'fake_ancestor_sha'],),
-       'This is parent commit with no footer.'),
-
+      # Cleanup calls to restore original state.
       ((['git', 'checkout', '-q', 'feature'],), ''),
       ((['git', 'branch', '-D', 'git-cl-commit'],), ''),
+      ((['git', 'branch', '-D', 'git-cl-cherry-pick'],), ''),
     ]
     with self.assertRaises(ValueError) as cm:
       git_cl.main(['land'])