diff --git a/gclient.py b/gclient.py index 13b15e0870..c68e2c39fa 100755 --- a/gclient.py +++ b/gclient.py @@ -848,7 +848,7 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): # Arguments number differs from overridden method # pylint: disable=arguments-differ def run(self, revision_overrides, command, args, work_queue, options, - patch_refs): + patch_refs, target_branches): """Runs |command| then parse the DEPS file.""" logging.info('Dependency(%s).run()' % self.name) assert self._file_list == [] @@ -875,9 +875,11 @@ class Dependency(gclient_utils.WorkItem, DependencySettings): patch_repo = self.url.split('@')[0] patch_ref = patch_refs.pop(self.FuzzyMatchUrl(patch_refs), None) + target_branch = target_branches.pop( + self.FuzzyMatchUrl(target_branches), None) if command == 'update' and patch_ref is not None: - self._used_scm.apply_patch_ref(patch_repo, patch_ref, options, - file_list) + self._used_scm.apply_patch_ref(patch_repo, patch_ref, target_branch, + options, file_list) if file_list: file_list = [os.path.join(self.name, f.strip()) for f in file_list] @@ -1520,19 +1522,23 @@ it or fix the checkout. index += 1 return revision_overrides - def _EnforcePatchRefs(self): + def _EnforcePatchRefsAndBranches(self): """Checks for patch refs.""" patch_refs = {} + target_branches = {} if not self._options.patch_refs: - return patch_refs + 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: raise gclient_utils.Error( 'Wrong revision format: %s should be of the form ' - 'patch_repo@patch_ref.' % given_patch_ref) + '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 + return patch_refs, target_branches def RunOnDeps(self, command, args, ignore_requirements=False, progress=True): """Runs a command on each dependency in a client and its dependencies. @@ -1546,6 +1552,7 @@ it or fix the checkout. revision_overrides = {} patch_refs = {} + target_branches = {} # It's unnecessary to check for revision overrides for 'recurse'. # Save a few seconds by not calling _EnforceRevisions() in that case. if command not in ('diff', 'recurse', 'runhooks', 'status', 'revert', @@ -1554,7 +1561,7 @@ it or fix the checkout. revision_overrides = self._EnforceRevisions() if command == 'update': - patch_refs = self._EnforcePatchRefs() + patch_refs, target_branches = self._EnforcePatchRefsAndBranches() # Disable progress for non-tty stdout. should_show_progress = ( setup_color.IS_TTY and not self._options.verbose and progress) @@ -1571,7 +1578,7 @@ it or fix the checkout. if s.should_process: work_queue.enqueue(s) work_queue.flush(revision_overrides, command, args, options=self._options, - patch_refs=patch_refs) + patch_refs=patch_refs, target_branches=target_branches) if revision_overrides: print('Please fix your script, having invalid --revision flags will soon ' @@ -1705,7 +1712,8 @@ it or fix the checkout. for s in self.dependencies: if s.should_process: work_queue.enqueue(s) - work_queue.flush({}, None, [], options=self._options, patch_refs=None) + work_queue.flush({}, None, [], options=self._options, patch_refs=None, + target_branches=None) def ShouldPrintRevision(dep): return (not self._options.filter @@ -1845,14 +1853,15 @@ class CipdDependency(Dependency): #override def run(self, revision_overrides, command, args, work_queue, options, - patch_refs): + patch_refs, target_branches): """Runs |command| then parse the DEPS file.""" logging.info('CipdDependency(%s).run()' % self.name) if not self.should_process: return self._CreatePackageIfNecessary() super(CipdDependency, self).run(revision_overrides, command, args, - work_queue, options, patch_refs) + work_queue, options, patch_refs, + target_branches) def _CreatePackageIfNecessary(self): # We lazily create the CIPD package to make sure that only packages @@ -2520,13 +2529,21 @@ def CMDsync(parser, args): 'work even if the src@ part is skipped.') parser.add_option('--patch-ref', action='append', dest='patch_refs', metavar='GERRIT_REF', default=[], - help='Patches the given reference with the format dep@ref. ' - 'For dep, you can specify URLs as well as paths, with ' - 'URLs taking preference. The reference will be ' - 'applied to the necessary path, will be rebased on ' - 'top what the dep was synced to, and then will do a ' - 'soft reset. Use --no-rebase-patch-ref and ' - '--reset-patch-ref to disable this behavior.') + help='Patches the given reference with the format ' + '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 ' + 'of what |dep| was synced to, and a soft reset will ' + 'be done. Use --no-rebase-patch-ref and ' + '--no-reset-patch-ref to disable this behavior. ' + '|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.') 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 3ca455778a..a4c0dfd83b 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -92,7 +92,6 @@ class SCMWrapper(object): This is the abstraction layer to bind to different SCM. """ - def __init__(self, url=None, root_dir=None, relpath=None, out_fh=None, out_cb=None, print_outbuf=False): self.url = url @@ -345,15 +344,22 @@ class GitWrapper(SCMWrapper): self.Print('FAILED to break lock: %s: %s' % (to_break, ex)) raise - def _GetBranchForCommit(self, commit): + # 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.""" - if scm.GIT.IsAncestor(self.checkout_path, commit, - 'refs/remotes/origin/master'): - return 'refs/remotes/origin/master' + _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, + ['for-each-ref', 'refs/remotes/%s' % self.remote, '--format=%(refname)']).splitlines() - for ref in remote_refs: + 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. ' @@ -364,7 +370,42 @@ class GitWrapper(SCMWrapper): # everything in between. return commit - def apply_patch_ref(self, patch_repo, patch_ref, options, file_list): + def apply_patch_ref(self, patch_repo, patch_ref, target_branch, options, + file_list): + """Apply a patch on top of the revision we're synced at. + + The patch ref is given by |patch_repo|@|patch_ref|, and the current revision + is |base_rev|. + We also need the |target_branch| that the patch was uploaded against. We use + it to find a merge base between |patch_rev| and |base_rev|, so we can find + what commits constitute the patch: + + Graphically, it looks like this: + + ... -> merge_base -> [possibly already landed commits] -> target_branch + \ + -> [possibly not yet landed dependent CLs] -> patch_rev + + Next, we apply the commits |merge_base..patch_rev| on top of whatever is + currently checked out, denoted |base_rev|. Typically, it'd be a revision + from |target_branch|, but this is not required. + + Graphically, we cherry pick |merge_base..patch_rev| on top of |base_rev|: + + ... -> base_rev -> [possibly not yet landed dependent CLs] -> patch_rev + + After application, if |options.reset_patch_ref| is specified, we soft reset + the just cherry-picked changes, keeping them in git index only. + + Args: + patch_repo: The patch origin. e.g. 'https://foo.googlesource.com/bar' + patch_ref: The ref to the patch. e.g. 'refs/changes/1234/34/1'. + target_branch: The branch the patch was uploaded against. + e.g. 'refs/heads/master' or 'refs/heads/infra/config'. + options: The options passed to gclient. + file_list: A list where modified files will be appended. + """ + # Abort any cherry-picks in progress. try: self._Capture(['cherry-pick', '--abort']) @@ -372,10 +413,12 @@ class GitWrapper(SCMWrapper): pass base_rev = self._Capture(['rev-parse', 'HEAD']) - branch_rev = self._GetBranchForCommit(base_rev) + target_branch = target_branch or self._GetTargetBranchForCommit(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('Patch ref is %r @ %r. Target branch for patch is %r. ' + 'Current HEAD is %r. Current dir is %r' % ( + patch_repo, patch_ref, target_branch, base_rev, + self.checkout_path)) self._Capture(['reset', '--hard']) self._Capture(['fetch', patch_repo, patch_ref]) patch_rev = self._Capture(['rev-parse', 'FETCH_HEAD']) @@ -386,9 +429,9 @@ class GitWrapper(SCMWrapper): 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]) + merge_base = self._Capture(['merge-base', target_branch, patch_rev]) self.Print('Merge base of %s and %s is %s' % ( - branch_rev, patch_rev, merge_base)) + target_branch, 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. @@ -406,8 +449,11 @@ class GitWrapper(SCMWrapper): 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('Failed to apply patch.') + self.Print('Patch ref is %r @ %r. Target branch for patch is %r. ' + 'Current HEAD is %r. Current dir is %r' % ( + patch_repo, patch_ref, target_branch, 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 diff --git a/recipes/recipe_modules/bot_update/resources/bot_update.py b/recipes/recipe_modules/bot_update/resources/bot_update.py index 37cdb2d107..a9a610c536 100755 --- a/recipes/recipe_modules/bot_update/resources/bot_update.py +++ b/recipes/recipe_modules/bot_update/resources/bot_update.py @@ -55,9 +55,6 @@ COMMIT_ORIGINAL_POSITION_FOOTER_KEY = 'Cr-Original-Commit-Position' # Regular expression to parse gclient's revinfo entries. REVINFO_RE = re.compile(r'^([^:]+):\s+([^@]+)@(.+)$') -# Regular expression to match gclient's patch error message. -PATCH_ERROR_RE = re.compile('Failed to apply .* @ .* to .* at .*') - # Copied from scripts/recipes/chromium.py. GOT_REVISION_MAPPINGS = { CHROMIUM_SRC_URL: { @@ -385,7 +382,7 @@ def gclient_sync( except SubprocessFailed as e: # If gclient sync is handling patching, parse the output for a patch error # message. - if apply_patch_on_gclient and PATCH_ERROR_RE.search(e.output): + if apply_patch_on_gclient and 'Failed to apply patch.' in e.output: raise PatchFailed(e.message, e.code, e.output) # Throw a GclientSyncFailed exception so we can catch this independently. raise GclientSyncFailed(e.message, e.code, e.output) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index cb85612874..123167a948 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -1088,12 +1088,12 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): """ for i in commits: name = os.path.join(self.root_dir, 'commit ' + str(i)) - self.assertTrue(os.path.exists(name)) + self.assertTrue(os.path.exists(name), 'Commit not found: %s' % 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)) + self.assertFalse(os.path.exists(name), 'Unexpected commit: %s' % name) def testCanCloneGerritChange(self): scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.') @@ -1133,7 +1133,7 @@ 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', self.options, + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, file_list) self.assertCommits([1, 2, 3, 4, 5, 6]) @@ -1155,7 +1155,7 @@ 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', self.options, + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, file_list) self.assertCommits([1, 5, 6]) @@ -1172,7 +1172,7 @@ 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', self.options, + scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', None, self.options, file_list) self.assertCommits([1, 2, 7, 8, 9, 10]) @@ -1190,7 +1190,7 @@ 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', self.options, + 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 @@ -1199,6 +1199,24 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): self.assertCommits([1, 2, 7, 10]) self.assertEqual(self.githash('repo_1', 7), self.gitrevparse(self.root_dir)) + def testCheckoutOriginFeaturePatchBranch(self): + scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.') + file_list = [] + + # Sync to the hash instead of 'origin/feature' + self.options.revision = self.githash('repo_1', 9) + scm.update(self.options, None, file_list) + self.assertEqual(self.githash('repo_1', 9), self.gitrevparse(self.root_dir)) + + # Apply refs/changes/34/1234/1, created for branch 'origin/master' on top of + # 'origin/feature'. + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', 'origin/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 'origin/feature'. + self.assertCommits([1, 2, 5, 6, 7, 8, 9]) + self.assertEqual(self.githash('repo_1', 9), self.gitrevparse(self.root_dir)) def testDoesntRebasePatchMaster(self): """Tests that we can apply a patch without rebasing it. @@ -1211,7 +1229,7 @@ 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', self.options, + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, file_list) self.assertCommits([1, 2, 3, 5, 6]) @@ -1230,7 +1248,7 @@ 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', self.options, + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, file_list) self.assertCommits([1, 2, 3, 5, 6]) @@ -1245,7 +1263,7 @@ 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', self.options, + scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options, file_list) self.assertCommits([1, 2, 3, 4, 5, 6]) @@ -1265,14 +1283,14 @@ 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', 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', self.options, + 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)) @@ -1290,7 +1308,7 @@ 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', self.options, + 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), @@ -1313,7 +1331,7 @@ 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', self.options, + 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 c12fef4333..244f0d1415 100755 --- a/tests/gclient_smoketest.py +++ b/tests/gclient_smoketest.py @@ -601,6 +601,30 @@ class GClientSmokeGIT(GClientSmokeBase): 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']) + self.gclient([ + '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)), + ]) + # 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 testSyncPatchRefNoHooks(self): if not self.enabled: return diff --git a/tests/gclient_test.py b/tests/gclient_test.py index 47c41cfbd9..a11a4e3e5f 100755 --- a/tests/gclient_test.py +++ b/tests/gclient_test.py @@ -310,7 +310,8 @@ class GclientTest(trial_dir.TestCase): work_queue = gclient_utils.ExecutionQueue(options.jobs, None, False) for s in client.dependencies: work_queue.enqueue(s) - work_queue.flush({}, None, [], options=options, patch_refs={}) + work_queue.flush({}, None, [], options=options, patch_refs={}, + target_branches={}) self.assertEqual( [h.action for h in client.GetHooks(options)], [tuple(x['action']) for x in hooks]) @@ -361,7 +362,8 @@ class GclientTest(trial_dir.TestCase): work_queue = gclient_utils.ExecutionQueue(options.jobs, None, False) for s in client.dependencies: work_queue.enqueue(s) - work_queue.flush({}, None, [], options=options, patch_refs={}) + work_queue.flush({}, None, [], options=options, patch_refs={}, + target_branches={}) self.assertEqual( [h.action for h in client.GetHooks(options)], [tuple(x['action']) for x in hooks + extra_hooks + sub_hooks])