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>

gclient_scm.py

@@ -345,29 +345,61 @@ 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):
     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:
+          self._Capture(['cherry-pick', merge_base + '..' + patch_rev])
+
+      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))
+      raise
 
-    if options.rebase_patch_ref:
-      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
     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,16 @@ 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
-
+    #    |
+    #    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 +1040,20 @@ 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)
+
 
 class GerritChangesTest(fake_repos.FakeReposTestBase):
   FAKE_REPOS_CLASS = GerritChangesFakeRepo
@@ -1052,6 +1071,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 +1111,137 @@ 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))
+
 
 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: