From 690d8d437bdf6da7a722c537c64f7a1a7b682590 Mon Sep 17 00:00:00 2001 From: Andrii Shyshkalov Date: Thu, 14 Jun 2018 22:57:17 +0000 Subject: [PATCH] Revert "gclient_scm: Use cherry-picking instead of rebasing." This reverts commit c912114140868b880bd23382d3569d5394678ff4. 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 > Reviewed-by: Aaron Gable 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 Commit-Queue: Andrii Shyshkalov --- gclient_scm.py | 59 ++++-------- recipes/trigger_recipe_roller.txt | 3 +- testing_support/fake_repos.py | 19 +--- tests/gclient_scm_test.py | 145 ------------------------------ 4 files changed, 23 insertions(+), 203 deletions(-) diff --git a/gclient_scm.py b/gclient_scm.py index 7e31dc6ea..c7a035e7f 100644 --- a/gclient_scm.py +++ b/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 diff --git a/recipes/trigger_recipe_roller.txt b/recipes/trigger_recipe_roller.txt index 4d0e5c5c4..31d563eb2 100644 --- a/recipes/trigger_recipe_roller.txt +++ b/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. diff --git a/testing_support/fake_repos.py b/testing_support/fake_repos.py index 9630f7301..63ea9912f 100755 --- a/testing_support/fake_repos.py +++ b/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) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 3176aa7f0..4742310c1 100755 --- a/tests/gclient_scm_test.py +++ b/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):