From 42b03c34539b29f2018295486724904f5549e2a4 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Thu, 6 Jun 2019 16:16:44 +0000 Subject: [PATCH] Revert "Reland "gclient: Require a target ref when applying patches."" This reverts commit 822dbc51a298688c61e4dd8c40fbc714e1460663. Reason for revert: Skia autoroller is complaining https://skia-review.googlesource.com/c/buildbot/+/219338 Original change's description: > Reland "gclient: Require a target ref when applying patches." > > This is a reland of 1d6478a5ffed7d272a7910d678e0a35d2f9fd69b > > Original change's description: > > gclient: Require a target ref when applying patches. > > > > Bug: 956807 > > Change-Id: Icfffe965f9f4651f22e8ba32c60133a5620bb350 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1616804 > > Reviewed-by: Andrii Shyshkalov > > Commit-Queue: Edward Lesmes > > Bug: 956807 > Change-Id: I3de8475a091ce6a2a14ff7dcfb92507a205ef78c > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1623594 > Reviewed-by: Andrii Shyshkalov > Commit-Queue: Edward Lesmes TBR=borenet@chromium.org,tandrii@chromium.org,ehmaldonado@chromium.org Change-Id: I14a70206a34f7024abc21039806f23dc7cf56bb6 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 956807 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1648321 Reviewed-by: Edward Lesmes Commit-Queue: Edward Lesmes --- gclient.py | 16 ++++++---- gclient_scm.py | 25 +++++++++++++++- tests/gclient_scm_test.py | 60 +++++++++++++++----------------------- tests/gclient_smoketest.py | 26 +++++++++++++++-- 4 files changed, 82 insertions(+), 45 deletions(-) diff --git a/gclient.py b/gclient.py index fd67a29be..71ec2b199 100755 --- a/gclient.py +++ b/gclient.py @@ -1574,12 +1574,13 @@ it or fix the checkout. return patch_refs, target_branches for given_patch_ref in self._options.patch_refs: patch_repo, _, patch_ref = given_patch_ref.partition('@') - if not patch_repo or not patch_ref or ':' not in patch_ref: + if not patch_repo or not patch_ref: raise gclient_utils.Error( 'Wrong revision format: %s should be of the form ' - 'patch_repo@target_branch:patch_ref.' % given_patch_ref) - target_branch, _, patch_ref = patch_ref.partition(':') - target_branches[patch_repo] = target_branch + 'patch_repo@[target_branch:]patch_ref.' % given_patch_ref) + if ':' in patch_ref: + target_branch, _, patch_ref = patch_ref.partition(':') + target_branches[patch_repo] = target_branch patch_refs[patch_repo] = patch_ref return patch_refs, target_branches @@ -2597,7 +2598,7 @@ def CMDsync(parser, args): parser.add_option('--patch-ref', action='append', dest='patch_refs', metavar='GERRIT_REF', default=[], help='Patches the given reference with the format ' - 'dep@target-ref:patch-ref. ' + 'dep@[target-ref:]patch-ref. ' 'For |dep|, you can specify URLs as well as paths, ' 'with URLs taking preference. ' '|patch-ref| will be applied to |dep|, rebased on top ' @@ -2607,7 +2608,10 @@ def CMDsync(parser, args): '|target-ref| is the target branch against which a ' 'patch was created, it is used to determine which ' 'commits from the |patch-ref| actually constitute a ' - 'patch.') + 'patch. If not given, we will iterate over all remote ' + 'branches and select one that contains the revision ' + '|dep| is synced at. ' + 'WARNING: |target-ref| will be mandatory soon.') parser.add_option('--with_branch_heads', action='store_true', help='Clone git "branch_heads" refspecs in addition to ' 'the default refspecs. This adds about 1/2GB to a ' diff --git a/gclient_scm.py b/gclient_scm.py index 72a5dd50a..ef7dbae4b 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -351,6 +351,28 @@ class GitWrapper(SCMWrapper): self.Print('FAILED to break lock: %s: %s' % (to_break, ex)) raise + # TODO(ehmaldonado): Remove after bot_update is modified to pass the patch's + # branch. + def _GetTargetBranchForCommit(self, commit): + """Get the remote branch a commit is part of.""" + _WELL_KNOWN_BRANCHES = [ + 'refs/remotes/origin/master', + 'refs/remotes/origin/infra/config', + 'refs/remotes/origin/lkgr', + ] + for branch in _WELL_KNOWN_BRANCHES: + if scm.GIT.IsAncestor(self.checkout_path, commit, branch): + return branch + remote_refs = self._Capture( + ['for-each-ref', 'refs/remotes/%s' % self.remote, + '--format=%(refname)']).splitlines() + for ref in sorted(remote_refs, reverse=True): + 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)) + return None + def apply_patch_ref(self, patch_repo, patch_rev, target_rev, options, file_list): """Apply a patch on top of the revision we're synced at. @@ -397,7 +419,8 @@ class GitWrapper(SCMWrapper): base_rev = self._Capture(['rev-parse', 'HEAD']) if not target_rev: - raise gclient_utils.Error('A target revision for the patch must be given') + # TODO(ehmaldonado): Raise an error once |target_rev| is mandatory. + target_rev = self._GetTargetBranchForCommit(base_rev) or base_rev elif target_rev.startswith('refs/heads/'): # If |target_rev| is in refs/heads/**, try first to find the corresponding # remote ref for it, since |target_rev| might point to a local ref which diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 1e99f52f5..f1b712e78 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -1168,9 +1168,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, + file_list) self.assertCommits([1, 2, 3, 4, 5, 6]) self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir)) @@ -1191,9 +1190,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, + file_list) self.assertCommits([1, 5, 6]) self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir)) @@ -1209,9 +1207,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): 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', 'refs/heads/feature', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', None, self.options, + file_list) self.assertCommits([1, 2, 7, 8, 9, 10]) self.assertEqual(self.githash('repo_1', 9), self.gitrevparse(self.root_dir)) @@ -1228,9 +1225,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): 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', 'refs/heads/feature', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', None, self.options, + file_list) # We shouldn't have rebased on top of 2 (which is the merge base between # remote's master branch and the change) but on top of 7 (which is the @@ -1249,9 +1245,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): # Apply refs/changes/34/1234/1, created for remote's master branch on top of # remote's feature branch. - scm.apply_patch_ref( - self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', 'refs/heads/master', + self.options, file_list) # Commits 5 and 6 are part of the patch, and commits 1, 2, 7, 8 and 9 are # part of remote's feature branch. @@ -1269,9 +1264,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, + file_list) self.assertCommits([1, 2, 3, 5, 6]) self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir)) @@ -1289,9 +1283,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, + file_list) self.assertCommits([1, 2, 3, 5, 6]) self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir)) @@ -1305,9 +1298,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, 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 @@ -1326,17 +1318,15 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): # 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', None, + 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, + file_list) self.assertCommits([1, 2, 3, 5, 6]) self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir)) @@ -1353,9 +1343,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): # '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', 'refs/heads/feature', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, + file_list) self.assertCommits([1, 2, 3, 5, 6, 12]) self.assertEqual(self.githash('repo_1', 12), self.gitrevparse(self.root_dir)) @@ -1377,9 +1366,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): # 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', 'refs/heads/master', self.options, - file_list) + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, + file_list) self.assertCommits([1, 2, 3, 5, 6]) self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir)) diff --git a/tests/gclient_smoketest.py b/tests/gclient_smoketest.py index fdf99e5e4..f286bc228 100755 --- a/tests/gclient_smoketest.py +++ b/tests/gclient_smoketest.py @@ -589,6 +589,29 @@ class GClientSmokeGIT(GClientSmokeBase): self.assertTree(tree) def testSyncPatchRef(self): + if not self.enabled: + return + self.gclient(['config', self.git_base + 'repo_1', '--name', 'src']) + self.gclient([ + 'sync', '-v', '-v', '-v', + '--revision', 'src/repo2@%s' % self.githash('repo_2', 1), + '--patch-ref', + '%srepo_2@%s' % (self.git_base, self.githash('repo_2', 2)), + ]) + # Assert that repo_2 files coincide with revision @2 (the patch ref) + tree = self.mangle_git_tree(('repo_1@2', 'src'), + ('repo_2@2', 'src/repo2'), + ('repo_3@2', 'src/repo2/repo_renamed')) + tree['src/git_hooked1'] = 'git_hooked1' + tree['src/git_hooked2'] = 'git_hooked2' + self.assertTree(tree) + # Assert that HEAD revision of repo_2 is @1 (the base we synced to) since we + # should have done a soft reset. + self.assertEqual( + self.githash('repo_2', 1), + self.gitrevparse(os.path.join(self.root_dir, 'src/repo2'))) + + def testSyncPatchRefBranch(self): if not self.enabled: return self.gclient(['config', self.git_base + 'repo_1', '--name', 'src']) @@ -620,8 +643,7 @@ class GClientSmokeGIT(GClientSmokeBase): 'sync', '-v', '-v', '-v', '--revision', 'src/repo2@%s' % self.githash('repo_2', 1), '--patch-ref', - '%srepo_2@refs/heads/master:%s' % ( - self.git_base, self.githash('repo_2', 2)), + '%srepo_2@%s' % (self.git_base, self.githash('repo_2', 2)), '--nohooks', ]) # Assert that repo_2 files coincide with revision @2 (the patch ref)