diff --git a/gclient_scm.py b/gclient_scm.py index f1a786253..76b1fb543 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -345,71 +345,29 @@ 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 (%r), root is %r' % ( - patch_repo, patch_ref, base_rev, branch_rev, 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: - # 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 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'])) - self._Capture(['cherry-pick', '--abort']) - 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]) diff --git a/scm.py b/scm.py index a8b357914..6baf1085a 100644 --- a/scm.py +++ b/scm.py @@ -255,15 +255,6 @@ 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 2a30e01fb..5fa2c90e6 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -127,7 +127,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 @@ -1012,18 +1011,12 @@ 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'}) @@ -1042,30 +1035,6 @@ 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 @@ -1083,18 +1052,6 @@ 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 = [] @@ -1123,179 +1080,6 @@ 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)) - 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 29b7767e4..8ee3d811a 100755 --- a/tests/scm_unittest.py +++ b/tests/scm_unittest.py @@ -90,7 +90,6 @@ class GitWrapperTestCase(BaseSCMTestCase): 'GetOldContents', 'GetPatchName', 'GetUpstreamBranch', - 'IsAncestor', 'IsDirectoryVersioned', 'IsInsideWorkTree', 'IsValidRevision', @@ -171,16 +170,6 @@ 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: