gclient_scm: Use cherry-picking instead of rebasing.

We have a problem when in this situation, 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 does merge-base between |patch| and |master|, and cherry-picks only the
changes belonging to the patch.

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

gclient_scm.py

@@ -221,6 +221,7 @@ class GitWrapper(SCMWrapper):
     if self.out_cb:
       filter_kwargs['predicate'] = self.out_cb
     self.filter = gclient_utils.GitFilter(**filter_kwargs)
+    self._used_revision = None
 
   @staticmethod
   def BinaryExists():
@@ -343,26 +344,48 @@ class GitWrapper(SCMWrapper):
   def apply_patch_ref(self, patch_repo, patch_ref, options, file_list):
     base_rev = self._Capture(['rev-parse', 'HEAD'])
     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, self._used_revision,
+        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:
+        # If the revision we were asked to sync is a reference
+        # (e.g. refs/heads/master) we can use it as our "master revision".
+        if self._used_revision.startswith('refs/'):
+          master_rev = self._used_revision
+        else:
+          # Otherwise, fall back to refs/remotes/origin/master.
+          master_rev = 'refs/remotes/origin/master'
+          self._Capture(['fetch', self.remote, 'refs/heads/master'])
+
+        # Find the merge-base between the master_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', master_rev, patch_rev])
+        self.Print('Merge base of %s and %s is %s' % (
+            master_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])
 
@@ -418,6 +441,8 @@ class GitWrapper(SCMWrapper):
       # hash is also a tag, only make a distinction at checkout
       rev_type = "hash"
 
+    self._used_revision = revision
+
     mirror = self._GetMirror(url, options)
     if mirror:
       url = mirror.mirror_path

recipes/trigger_recipe_roller.txt

@@ -8,5 +8,4 @@ the Build and Test Yeti.
 As the CI needs of the browser grew, Batty, the Build and Test Yeti, got
 a new friend:
 
-
-The End.
+The End!!

testing_support/fake_repos.py

@@ -247,7 +247,7 @@ class FakeReposBase(object):
       return False
     for repo in ['repo_%d' % r for r in range(1, self.NB_GIT_REPOS + 1)]:
       subprocess2.check_call(['git', 'init', '-q', join(self.git_root, repo)])
-      self.git_hashes[repo] = [None]
+      self.git_hashes[repo] = [(None, None)]
     self.git_port = find_free_port(self.host, 20000)
     self.git_base = 'git://%s:%d/git/' % (self.host, self.git_port)
     # Start the daemon.
@@ -275,17 +275,28 @@ class FakeReposBase(object):
     return subprocess2.check_output(
         ['git', 'rev-parse', 'HEAD'], cwd=path).strip()
 
-  def _commit_git(self, repo, tree):
+  def _commit_git(self, repo, tree, base=None):
     repo_root = join(self.git_root, repo)
+    if base:
+      base_commit = self.git_hashes[repo][base][0]
+      subprocess2.check_call(
+          ['git', 'checkout', base_commit], cwd=repo_root)
     self._genTree(repo_root, tree)
     commit_hash = commit_git(repo_root)
-    if self.git_hashes[repo][-1]:
-      new_tree = self.git_hashes[repo][-1][1].copy()
+    base = base or -1
+    if self.git_hashes[repo][base][1]:
+      new_tree = self.git_hashes[repo][base][1].copy()
       new_tree.update(tree)
     else:
       new_tree = tree.copy()
     self.git_hashes[repo].append((commit_hash, new_tree))
 
+  def _create_ref(self, repo, ref, revision):
+    repo_root = join(self.git_root, repo)
+    subprocess2.check_call(
+        ['git', 'update-ref', ref, self.git_hashes[repo][revision][0]],
+        cwd=repo_root)
+
   def _fast_import_git(self, repo, data):
     repo_root = join(self.git_root, repo)
     logging.debug('%s: fast-import %s', repo, data)

tests/gclient_scm_test.py

@@ -21,6 +21,7 @@ import unittest
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
+from testing_support import fake_repos
 from testing_support.super_mox import mox, StdoutCheck, SuperMoxTestBase
 from testing_support.super_mox import TestCaseUtils
 
@@ -126,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
@@ -253,6 +255,149 @@ from :3
       gclient_scm.GitWrapper.BinaryExists = self._original_GitBinaryExists
 
 
+class ApplyPatchFakeRepo(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
+    #        /
+    # 1--2--3--4 refs/heads/master
+
+
+    self._commit_git('repo_1', {'commit 1': 'touched'})
+    self._commit_git('repo_1', {'commit 2': 'touched'})
+    self._commit_git('repo_1', {'commit 3': 'touched'})
+    self._commit_git('repo_1', {'commit 4': 'touched'})
+    self._create_ref('repo_1', 'refs/heads/master', 4)
+
+    # Create a change on top of commit 3 that consists of two commits.
+    self._commit_git('repo_1',
+                     {'commit 5': 'touched',
+                      'change': '1234'},
+                     base=3)
+    self._create_ref('repo_1', 'refs/changes/34/1234/1', 5)
+    self._commit_git('repo_1',
+                     {'commit 6': 'touched',
+                      'change': '1235'})
+    self._create_ref('repo_1', 'refs/changes/35/1235/1', 6)
+
+
+class ApplyPatchTestCase(fake_repos.FakeReposTestBase):
+  FAKE_REPOS_CLASS = ApplyPatchFakeRepo
+
+  def setUp(self):
+    super(ApplyPatchTestCase, self).setUp()
+    self.enabled = self.FAKE_REPOS.set_up_git()
+    self.options = BaseGitWrapperTestCase.OptionsObject()
+    self.url = self.git_base + 'repo_1'
+
+  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 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 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))
+
+
 class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
 
   def testRevertMissing(self):