Reland "Reland "gclient_scm: Use cherry-picking instead of rebasing.""

Abort any cherry-picks before applying the patch, so that if the bots are in a
bad state, we don't fail.

Original change's description:
> Reland "gclient_scm: Use cherry-picking instead of rebasing."
>
> The failures were caused by:
>  1 - When one change (call it #2) has been uploaded on top of another (#1),
>      and (#1) has already landed, git cherry-pick complains that the range
>      '<merge-base>..<change #2>' contains empty commits, since the contents
>      of (#1) are already present in the tree.
>  2 - We did not abort the cherry-picking when 'git cherry-pick' failed,
>      so a failure made all further CLs in that bot fail.
>
> This CL fixes it and prevents further regressions.
>
> Original change's description:
> > gclient_scm: Use cherry-picking instead of rebasing.
> >
> > Currently gclient might include extra commits when applying patches.
> > For example, in this case we checkout |patch| and rebase it on top of |base|,
> > thus including an |extra commit| that we shouldn't.
> >
> > o master
> > |
> > . o patch
> > |/
> > o extra commit
> > |
> > o base (what gclient synced src at)
> >
> > This change uses the merge-base between |patch| and |master| to cherry-pick only
> > the changes belonging to the patch.
> >
> > Bug: 850812
> > Change-Id: I138192f96bc62b1bb19b0e1ad952c8f8c67631c4
> > Reviewed-on: https://chromium-review.googlesource.com/1137052
> > Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
> > Reviewed-by: Aaron Gable <agable@chromium.org>
>
> Bug: 850812
> Change-Id: I83f38d0a258df3f5cd89e277f0d648badff29a22
> Reviewed-on: https://chromium-review.googlesource.com/1139554
> Reviewed-by: Aaron Gable <agable@chromium.org>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

Bug: 850812
Change-Id: Ic65bda67c792bd7af5ec013a62d9615d1498eb3a
Reviewed-on: https://chromium-review.googlesource.com/1142805
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

gclient_scm.py

@@ -345,29 +345,80 @@ class GitWrapper(SCMWrapper):
               self.Print('FAILED to break lock: %s: %s' % (to_break, ex))
               raise
 
+  def _GetBranchForCommit(self, commit):
+    """Get the remote branch a commit is part of."""
+    if scm.GIT.IsAncestor(self.checkout_path, commit,
+                          'refs/remotes/origin/master'):
+      return 'refs/remotes/origin/master'
+    remote_refs = self._Capture(
+        ['for-each-ref', 'refs/remotes/%s/*' % self.remote,
+         '--format=%(refname)']).splitlines()
+    for ref in remote_refs:
+      if scm.GIT.IsAncestor(self.checkout_path, commit, ref):
+        return ref
+    self.Print('Failed to find a remote ref that contains %s. '
+               'Candidate refs were %s.' % (commit, remote_refs))
+    # Fallback to the commit we got.
+    # This means that apply_path_ref will try to find the merge-base between the
+    # patch and the commit (which is most likely the commit) and cherry-pick
+    # everything in between.
+    return commit
+
   def apply_patch_ref(self, patch_repo, patch_ref, options, file_list):
+    # Abort any cherry-picks in progress.
+    try:
+      self._Capture(['cherry-pick', '--abort'])
+    except subprocess2.CalledProcessError:
+      pass
+
     base_rev = self._Capture(['rev-parse', 'HEAD'])
+    branch_rev = self._GetBranchForCommit(base_rev)
     self.Print('===Applying patch ref===')
-    self.Print('Repo is %r @ %r, ref is %r, root is %r' % (
-        patch_repo, patch_ref, base_rev, self.checkout_path))
+    self.Print('Repo is %r @ %r, ref is %r (%r), root is %r' % (
+        patch_repo, patch_ref, base_rev, branch_rev, self.checkout_path))
     self._Capture(['reset', '--hard'])
     self._Capture(['fetch', patch_repo, patch_ref])
-    if file_list is not None:
-      file_list.extend(self._GetDiffFilenames('FETCH_HEAD'))
-    self._Capture(['checkout', 'FETCH_HEAD'])
+    patch_rev = self._Capture(['rev-parse', 'FETCH_HEAD'])
+
+    try:
+      if not options.rebase_patch_ref:
+        self._Capture(['checkout', patch_rev])
+      else:
+        # Find the merge-base between the branch_rev and patch_rev to find out
+        # the changes we need to cherry-pick on top of base_rev.
+        merge_base = self._Capture(['merge-base', branch_rev, patch_rev])
+        self.Print('Merge base of %s and %s is %s' % (
+            branch_rev, patch_rev, merge_base))
+        if merge_base == patch_rev:
+          # If the merge-base is patch_rev, it means patch_rev is already part
+          # of the history, so just check it out.
+          self._Capture(['checkout', patch_rev])
+        else:
+          # If a change was uploaded on top of another change, which has already
+          # landed, one of the commits in the cherry-pick range will be
+          # redundant, since it has already landed and its changes incorporated
+          # in the tree.
+          # We pass '--keep-redundant-commits' to ignore those changes.
+          self._Capture(['cherry-pick', merge_base + '..' + patch_rev,
+                         '--keep-redundant-commits'])
 
-    if options.rebase_patch_ref:
+      if file_list is not None:
+        file_list.extend(self._GetDiffFilenames(base_rev))
+
+    except subprocess2.CalledProcessError as e:
+      self.Print('Failed to apply %r @ %r to %r at %r' % (
+              patch_repo, patch_ref, base_rev, self.checkout_path))
+      self.Print('git returned non-zero exit status %s:\n%s' % (
+          e.returncode, e.stderr))
+      # Print the current status so that developers know what changes caused the
+      # patch failure, since git cherry-pick doesn't show that information.
+      self.Print(self._Capture(['status']))
       try:
-        # TODO(ehmaldonado): Look into cherry-picking to avoid an expensive
-        # checkout + rebase.
-        self._Capture(['rebase', base_rev])
-      except subprocess2.CalledProcessError as e:
-        self.Print('Failed to apply %r @ %r to %r at %r' % (
-                patch_repo, patch_ref, base_rev, self.checkout_path))
-        self.Print('git returned non-zero exit status %s:\n%s' % (
-            e.returncode, e.stderr))
-        self._Capture(['rebase', '--abort'])
-        raise
+        self._Capture(['cherry-pick', '--abort'])
+      except subprocess2.CalledProcessError:
+        pass
+      raise
+
     if options.reset_patch_ref:
       self._Capture(['reset', '--soft', base_rev])
 

scm.py

@@ -255,6 +255,15 @@ class GIT(object):
         upstream_branch = ''.join(remote_ref)
     return upstream_branch
 
+  @staticmethod
+  def IsAncestor(cwd, maybe_ancestor, ref):
+    """Verifies if |maybe_ancestor| is an ancestor of |ref|."""
+    try:
+      GIT.Capture(['merge-base', '--is-ancestor', maybe_ancestor, ref], cwd=cwd)
+      return True
+    except subprocess2.CalledProcessError:
+      return False
+
   @staticmethod
   def GetOldContents(cwd, filename, branch=None):
     if not branch:

tests/gclient_scm_test.py

@@ -127,6 +127,7 @@ class BaseGitWrapperTestCase(GCBaseTestCase, StdoutCheck, TestCaseUtils,
       self.patch_ref = None
       self.patch_repo = None
       self.rebase_patch_ref = True
+      self.reset_patch_ref = True
 
   sample_git_import = """blob
 mark :1
@@ -1011,12 +1012,18 @@ class GerritChangesFakeRepo(fake_repos.FakeReposBase):
   def populateGit(self):
     # Creates a tree that looks like this:
     #
-    #           6 refs/changes/35/1235/1
-    #          /
-    #         5 refs/changes/34/1234/1
-    #        /
+    #       6 refs/changes/35/1235/1
+    #       |
+    #       5 refs/changes/34/1234/1
+    #       |
     # 1--2--3--4 refs/heads/master
-
+    #    |  |
+    #    |  11(5)--12 refs/heads/master-with-5
+    #    |
+    #    7--8--9 refs/heads/feature
+    #       |
+    #       10 refs/changes/36/1236/1
+    #
 
     self._commit_git('repo_1', {'commit 1': 'touched'})
     self._commit_git('repo_1', {'commit 2': 'touched'})
@@ -1035,6 +1042,30 @@ class GerritChangesFakeRepo(fake_repos.FakeReposBase):
                       'change': '1235'})
     self._create_ref('repo_1', 'refs/changes/35/1235/1', 6)
 
+    # Create a refs/heads/feature branch on top of commit 2, consisting of three
+    # commits.
+    self._commit_git('repo_1', {'commit 7': 'touched'}, base=2)
+    self._commit_git('repo_1', {'commit 8': 'touched'})
+    self._commit_git('repo_1', {'commit 9': 'touched'})
+    self._create_ref('repo_1', 'refs/heads/feature', 9)
+
+    # Create a change of top of commit 8.
+    self._commit_git('repo_1',
+                     {'commit 10': 'touched',
+                      'change': '1236'},
+                     base=8)
+    self._create_ref('repo_1', 'refs/changes/36/1236/1', 10)
+
+    # Create a refs/heads/master-with-5 on top of commit 3 which is a branch
+    # where refs/changes/34/1234/1 (commit 5) has already landed as commit 11.
+    self._commit_git('repo_1',
+                     # This is really commit 11, but has the changes of commit 5
+                     {'commit 5': 'touched',
+                      'change': '1234'},
+                     base=3)
+    self._commit_git('repo_1', {'commit 12': 'touched'})
+    self._create_ref('repo_1', 'refs/heads/master-with-5', 12)
+
 
 class GerritChangesTest(fake_repos.FakeReposTestBase):
   FAKE_REPOS_CLASS = GerritChangesFakeRepo
@@ -1052,6 +1083,18 @@ class GerritChangesTest(fake_repos.FakeReposTestBase):
     self.addCleanup(rmtree, self.mirror)
     self.addCleanup(git_cache.Mirror.SetCachePath, None)
 
+  def assertCommits(self, commits):
+    """Check that all, and only |commits| are present in the current checkout.
+    """
+    for i in commits:
+      name = os.path.join(self.root_dir, 'commit ' + str(i))
+      self.assertTrue(os.path.exists(name))
+
+    all_commits = set(range(1, len(self.FAKE_REPOS.git_hashes['repo_1'])))
+    for i in all_commits - set(commits):
+      name = os.path.join(self.root_dir, 'commit ' + str(i))
+      self.assertFalse(os.path.exists(name))
+
   def testCanCloneGerritChange(self):
     scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
     file_list = []
@@ -1080,6 +1123,201 @@ class GerritChangesTest(fake_repos.FakeReposTestBase):
     self.setUpMirror()
     self.testCanSyncToGerritChange()
 
+  def testAppliesPatchOnTopOfMasterByDefault(self):
+    """Test the default case, where we apply a patch on top of master."""
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    # Make sure we don't specify a revision.
+    self.options.revision = None
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
+
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+
+    self.assertCommits([1, 2, 3, 4, 5, 6])
+    self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
+
+  def testCheckoutOlderThanPatchBase(self):
+    """Test applying a patch on an old checkout.
+
+    We first checkout commit 1, and try to patch refs/changes/35/1235/1, which
+    contains commits 5 and 6, and is based on top of commit 3.
+    The final result should contain commits 1, 5 and 6, but not commits 2 or 3.
+    """
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    # Sync to commit 1
+    self.options.revision = self.githash('repo_1', 1)
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir))
+
+    # Apply the change on top of that.
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+
+    self.assertCommits([1, 5, 6])
+    self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir))
+
+  def testCheckoutOriginFeature(self):
+    """Tests that we can apply a patch on a branch other than master."""
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    # Sync to origin/feature
+    self.options.revision = 'origin/feature'
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 9), self.gitrevparse(self.root_dir))
+
+    # Apply the change on top of that.
+    scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', self.options,
+                        file_list)
+
+    self.assertCommits([1, 2, 7, 8, 9, 10])
+    self.assertEqual(self.githash('repo_1', 9), self.gitrevparse(self.root_dir))
+
+  def testCheckoutOriginFeatureOnOldRevision(self):
+    """Tests that we can apply a patch on an old checkout, on a branch other
+    than master."""
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    # Sync to origin/feature on an old revision
+    self.options.revision = self.githash('repo_1', 7)
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 7), self.gitrevparse(self.root_dir))
+
+    # Apply the change on top of that.
+    scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', self.options,
+                        file_list)
+
+    # We shouldn't have rebased on top of 2 (which is the merge base between
+    # origin/master and the change) but on top of 7 (which is the merge base
+    # between origin/feature and the change).
+    self.assertCommits([1, 2, 7, 10])
+    self.assertEqual(self.githash('repo_1', 7), self.gitrevparse(self.root_dir))
+
+
+  def testDoesntRebasePatchMaster(self):
+    """Tests that we can apply a patch without rebasing it.
+    """
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    self.options.rebase_patch_ref = False
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
+
+    # Apply the change on top of that.
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+
+    self.assertCommits([1, 2, 3, 5, 6])
+    self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
+
+  def testDoesntRebasePatchOldCheckout(self):
+    """Tests that we can apply a patch without rebasing it on an old checkout.
+    """
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    # Sync to commit 1
+    self.options.revision = self.githash('repo_1', 1)
+    self.options.rebase_patch_ref = False
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir))
+
+    # Apply the change on top of that.
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+
+    self.assertCommits([1, 2, 3, 5, 6])
+    self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir))
+
+  def testDoesntSoftResetIfNotAskedTo(self):
+    """Test that we can apply a patch without doing a soft reset."""
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    self.options.reset_patch_ref = False
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
+
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+
+    self.assertCommits([1, 2, 3, 4, 5, 6])
+    # The commit hash after cherry-picking is not known, but it must be
+    # different from what the repo was synced at before patching.
+    self.assertNotEqual(self.githash('repo_1', 4),
+                        self.gitrevparse(self.root_dir))
+
+  def testRecoversAfterPatchFailure(self):
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    self.options.revision = 'refs/changes/34/1234/1'
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir))
+
+    # Checkout 'refs/changes/34/1234/1' modifies the 'change' file, so trying to
+    # patch 'refs/changes/36/1236/1' creates a patch failure.
+    with self.assertRaises(subprocess2.CalledProcessError) as cm:
+      scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', self.options,
+                          file_list)
+    self.assertEqual(cm.exception.cmd[:2], ['git', 'cherry-pick'])
+    self.assertIn('error: could not apply', cm.exception.stderr)
+
+    # Try to apply 'refs/changes/35/1235/1', which doesn't have a merge
+    # conflict.
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+    self.assertCommits([1, 2, 3, 5, 6])
+    self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir))
+
+  def testIgnoresAlreadyMergedCommits(self):
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    self.options.revision = 'refs/heads/master-with-5'
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 12),
+                     self.gitrevparse(self.root_dir))
+
+    # When we try 'refs/changes/35/1235/1' on top of 'refs/heads/feature',
+    # 'refs/changes/34/1234/1' will be an empty commit, since the changes were
+    # already present in the tree as commit 11.
+    # Make sure we deal with this gracefully.
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+    self.assertCommits([1, 2, 3, 5, 6, 12])
+    self.assertEqual(self.githash('repo_1', 12),
+                     self.gitrevparse(self.root_dir))
+
+  def testRecoversFromExistingCherryPick(self):
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+
+    self.options.revision = 'refs/changes/34/1234/1'
+    scm.update(self.options, None, file_list)
+    self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir))
+
+    # Checkout 'refs/changes/34/1234/1' modifies the 'change' file, so trying to
+    # cherry-pick 'refs/changes/36/1236/1' raises an error.
+    scm._Run(['fetch', 'origin', 'refs/changes/36/1236/1'], self.options)
+    with self.assertRaises(subprocess2.CalledProcessError) as cm:
+      scm._Run(['cherry-pick', 'FETCH_HEAD'], self.options)
+    self.assertEqual(cm.exception.cmd[:2], ['git', 'cherry-pick'])
+
+    # Try to apply 'refs/changes/35/1235/1', which doesn't have a merge
+    # conflict.
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', self.options,
+                        file_list)
+    self.assertCommits([1, 2, 3, 5, 6])
+    self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir))
+
 
 if __name__ == '__main__':
   level = logging.DEBUG if '-v' in sys.argv else logging.FATAL

tests/scm_unittest.py

@@ -90,6 +90,7 @@ class GitWrapperTestCase(BaseSCMTestCase):
         'GetOldContents',
         'GetPatchName',
         'GetUpstreamBranch',
+        'IsAncestor',
         'IsDirectoryVersioned',
         'IsInsideWorkTree',
         'IsValidRevision',
@@ -170,6 +171,16 @@ class RealGitTest(fake_repos.FakeReposTestBase):
     self.assertTrue(scm.GIT.IsValidRevision(cwd=self.clone_dir, rev=first_rev))
     self.assertTrue(scm.GIT.IsValidRevision(cwd=self.clone_dir, rev='HEAD'))
 
+  def testIsAncestor(self):
+    if not self.enabled:
+      return
+    self.assertTrue(scm.GIT.IsAncestor(
+        self.clone_dir, self.githash('repo_1', 1), self.githash('repo_1', 2)))
+    self.assertFalse(scm.GIT.IsAncestor(
+        self.clone_dir, self.githash('repo_1', 2), self.githash('repo_1', 1)))
+    self.assertFalse(scm.GIT.IsAncestor(
+        self.clone_dir, self.githash('repo_1', 1), 'zebra'))
+
 
 if __name__ == '__main__':
   if '-v' in sys.argv: