From ca7d881540e391fc1b5aa44d08c08a67e0b1a797 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Tue, 24 Jul 2018 17:42:45 +0000 Subject: [PATCH] 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 > '..' 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 > > Reviewed-by: Aaron Gable > > Bug: 850812 > Change-Id: I83f38d0a258df3f5cd89e277f0d648badff29a22 > Reviewed-on: https://chromium-review.googlesource.com/1139554 > Reviewed-by: Aaron Gable > Commit-Queue: Edward Lesmes Bug: 850812 Change-Id: Ic65bda67c792bd7af5ec013a62d9615d1498eb3a Reviewed-on: https://chromium-review.googlesource.com/1142805 Reviewed-by: Aaron Gable Commit-Queue: Edward Lesmes --- gclient_scm.py | 83 ++++++++++--- scm.py | 9 ++ tests/gclient_scm_test.py | 248 +++++++++++++++++++++++++++++++++++++- tests/scm_unittest.py | 11 ++ 4 files changed, 330 insertions(+), 21 deletions(-) diff --git a/gclient_scm.py b/gclient_scm.py index 76b1fb543..3ca455778 100644 --- a/gclient_scm.py +++ b/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]) diff --git a/scm.py b/scm.py index 6baf1085a..a8b357914 100644 --- a/scm.py +++ b/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: diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 5fa2c90e6..cb8561287 100755 --- a/tests/gclient_scm_test.py +++ b/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 diff --git a/tests/scm_unittest.py b/tests/scm_unittest.py index 8ee3d811a..29b7767e4 100755 --- a/tests/scm_unittest.py +++ b/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: