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

This reverts commit c912114140.

Reason for revert: broke patch application on infra/config https://crbug.com/853032

Original change's description:
> 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>

TBR=agable@chromium.org,ehmaldonado@chromium.org

Change-Id: Ib3feeee2f44f5441713383f1dbf08db16fae4717
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 850812, 853032
Reviewed-on: https://chromium-review.googlesource.com/1101977
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

gclient_scm.py

@@ -221,7 +221,6 @@ 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():
@@ -344,48 +343,26 @@ 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 (%r), root is %r' % (
-        patch_repo, patch_ref, base_rev, self._used_revision,
-        self.checkout_path))
+    self.Print('Repo is %r @ %r, ref is %r, root is %r' % (
+        patch_repo, patch_ref, base_rev, self.checkout_path))
     self._Capture(['reset', '--hard'])
     self._Capture(['fetch', patch_repo, patch_ref])
-    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 file_list is not None:
+      file_list.extend(self._GetDiffFilenames('FETCH_HEAD'))
+    self._Capture(['checkout', 'FETCH_HEAD'])
 
+    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])
 
@@ -441,8 +418,6 @@ 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,4 +8,5 @@ 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, None)]
+      self.git_hashes[repo] = [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,28 +275,17 @@ class FakeReposBase(object):
     return subprocess2.check_output(
         ['git', 'rev-parse', 'HEAD'], cwd=path).strip()
 
-  def _commit_git(self, repo, tree, base=None):
+  def _commit_git(self, repo, tree):
     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)
-    base = base or -1
-    if self.git_hashes[repo][base][1]:
-      new_tree = self.git_hashes[repo][base][1].copy()
+    if self.git_hashes[repo][-1]:
+      new_tree = self.git_hashes[repo][-1][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,7 +21,6 @@ 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
 
@@ -127,7 +126,6 @@ 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
@@ -255,149 +253,6 @@ 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):