Commit 1d6478a5 authored by Edward Lemur's avatar Edward Lemur Committed by Commit Bot

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/+/1616804Reviewed-by: 's avatarAndrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
parent b2d7725d
......@@ -1574,13 +1574,12 @@ 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:
if not patch_repo or not patch_ref or ':' not in patch_ref:
raise gclient_utils.Error(
'Wrong revision format: %s should be of the form '
'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_repo@target_branch:patch_ref.' % given_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
......@@ -2598,7 +2597,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 '
......@@ -2608,10 +2607,7 @@ 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. 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.')
'patch.')
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 '
......
......@@ -351,28 +351,6 @@ 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_ref, target_ref, options,
file_list):
"""Apply a patch on top of the revision we're synced at.
......@@ -417,13 +395,12 @@ class GitWrapper(SCMWrapper):
base_rev = self._Capture(['rev-parse', 'HEAD'])
if not target_ref:
target_ref = self._GetTargetBranchForCommit(base_rev) or base_rev
elif not target_ref.startswith(('refs/heads/', 'refs/branch-heads/')):
if (not target_ref
or not target_ref.startswith(('refs/heads/', 'refs/branch-heads/'))):
# TODO(ehmaldonado): Support all refs.
raise gclient_utils.Error(
'target_ref must be in refs/heads/** or refs/branch-heads/**. '
'Got %s instead.' % target_ref)
'target_ref must be given and must be in refs/heads/** or '
'refs/branch-heads/**. Got %s instead.' % target_ref)
elif target_ref == 'refs/heads/master':
# We handle refs/heads/master separately because bot_update treats it
# differently than other refs: it will fetch refs/heads/foo to
......
......@@ -1168,8 +1168,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options,
file_list)
self.assertCommits([1, 2, 3, 4, 5, 6])
self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
......@@ -1190,8 +1191,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options,
file_list)
self.assertCommits([1, 5, 6])
self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir))
......@@ -1207,8 +1209,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/36/1236/1', 'refs/heads/feature', self.options,
file_list)
self.assertCommits([1, 2, 7, 8, 9, 10])
self.assertEqual(self.githash('repo_1', 9), self.gitrevparse(self.root_dir))
......@@ -1225,8 +1228,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/36/1236/1', 'refs/heads/feature', 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
......@@ -1245,8 +1249,9 @@ 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.
......@@ -1264,8 +1269,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options,
file_list)
self.assertCommits([1, 2, 3, 5, 6])
self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
......@@ -1283,8 +1289,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options,
file_list)
self.assertCommits([1, 2, 3, 5, 6])
self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir))
......@@ -1298,8 +1305,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/master', 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
......@@ -1318,15 +1326,17 @@ 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', None,
self.options, file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/36/1236/1', 'refs/heads/master', 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options,
file_list)
self.assertCommits([1, 2, 3, 5, 6])
self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir))
......@@ -1343,8 +1353,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/feature', self.options,
file_list)
self.assertCommits([1, 2, 3, 5, 6, 12])
self.assertEqual(self.githash('repo_1', 12),
self.gitrevparse(self.root_dir))
......@@ -1366,8 +1377,9 @@ 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', None, self.options,
file_list)
scm.apply_patch_ref(
self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options,
file_list)
self.assertCommits([1, 2, 3, 5, 6])
self.assertEqual(self.githash('repo_1', 5), self.gitrevparse(self.root_dir))
......
......@@ -589,29 +589,6 @@ 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'])
......@@ -643,7 +620,8 @@ class GClientSmokeGIT(GClientSmokeBase):
'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)),
'%srepo_2@refs/heads/master:%s' % (
self.git_base, self.githash('repo_2', 2)),
'--nohooks',
])
# Assert that repo_2 files coincide with revision @2 (the patch ref)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment