From ecda7828113fc880f1e3ee0b390f0d4feb893279 Mon Sep 17 00:00:00 2001 From: Ravi Mistry Date: Mon, 28 Feb 2022 16:22:20 +0000 Subject: [PATCH] Reland "Reland "Add support for Gerrit topics in gclient syncs"" This is a reland of fc9a40e3c62158938bbba7b4b497bcf352472385 Hopefully the cause of the 2nd revert was fixed in https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3489327 Original change's description: > Reland "Add support for Gerrit topics in gclient syncs" > > This is a reland of 0f13273f1f235a8a416e9ddd5f35c20bac268e55 > > Hopefully the cause of the original revert was fixed in https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3480835 > > Original change's description: > > Add support for Gerrit topics in gclient syncs > > > > > > If the new flag "--download-topics" is specified with a "--patch-ref" then: > > * Finds the topic of the Gerrit change. > > * Finds all open changes in the same repo as the Gerrit change. > > * Cherrypicks all changes locally. > > > > This functionality can be used by developers and bots to apply all changes with the same topic in the checkout to be tested at the same time (similar to how Android's TreeHugger handles topics). > > > > > > Tested by: > > > > * Running the new unit test with `python gclient_scm_test.py GerritChangesTest.testDownloadsTopics` from the `tests/` directory. > > > > * Running an end-to-end test with `DEPOT_TOOLS_UPDATE=0 gclient sync --patch-ref "skia@d831da5b8ac2d257c5b0cf2ec6645a148f05e662:refs/changes/17/505217/2" --download-topics` in a skia checkout. > > > > > > Bug: chromium:1298922 > > Change-Id: Ieace5e27fbc9c5d0ea90a037bf80a95062c1b164 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3444003 > > Reviewed-by: Josip Sokcevic > > Commit-Queue: Ravi Mistry > > Bug: chromium:1298922 > Change-Id: I80747d797234bba06c17ef5c5e85b310281922c4 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3484976 > Reviewed-by: Josip Sokcevic > Commit-Queue: Ravi Mistry Bug: chromium:1298922 Change-Id: I21d7251bafff808b1144d6e522fa9f384f4541bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3490488 Reviewed-by: Josip Sokcevic Commit-Queue: Ravi Mistry --- gclient.py | 9 +++ gclient_scm.py | 141 +++++++++++++++++++++++++------------- tests/gclient_scm_test.py | 32 +++++++++ 3 files changed, 134 insertions(+), 48 deletions(-) diff --git a/gclient.py b/gclient.py index e75b94ef2..83fcbbd9d 100755 --- a/gclient.py +++ b/gclient.py @@ -2699,6 +2699,11 @@ def CMDsync(parser, args): 'patch was created, it is used to determine which ' 'commits from the |patch-ref| actually constitute a ' 'patch.') + parser.add_option('-t', '--download-topics', action='store_true', + help='Downloads and patches locally changes from all open ' + 'Gerrit CLs that have the same topic as the changes ' + 'in the specified patch_refs. Only works if atleast ' + 'one --patch-ref is specified.') 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 ' @@ -2766,6 +2771,10 @@ def CMDsync(parser, args): if not client: raise gclient_utils.Error('client not configured; see \'gclient config\'') + if options.download_topics and not options.rebase_patch_ref: + raise gclient_utils.Error( + 'Warning: You cannot download topics and not rebase each patch ref') + if options.ignore_locks: print('Warning: ignore_locks is no longer used. Please remove its usage.') diff --git a/gclient_scm.py b/gclient_scm.py index 41e8e926a..959d3dc79 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -26,6 +26,7 @@ except ImportError: # For Py3 compatibility import urllib.parse as urlparse import gclient_utils +import gerrit_util import git_cache import scm import shutil @@ -404,57 +405,101 @@ class GitWrapper(SCMWrapper): self._UpdateMirrorIfNotContains(mirror, options, rev_type, target_rev) self._Fetch(options, refspec=target_rev) - self.Print('===Applying patch===') - self.Print('Revision to patch is %r @ %r.' % (patch_repo, patch_rev)) - self.Print('Current dir is %r' % self.checkout_path) - self._Capture(['reset', '--hard']) - self._Capture(['fetch', '--no-tags', patch_repo, patch_rev]) - patch_rev = self._Capture(['rev-parse', 'FETCH_HEAD']) - - if not options.rebase_patch_ref: - self._Capture(['checkout', patch_rev]) - # Adjust base_rev to be the first parent of our checked out patch ref; - # This will allow us to correctly extend `file_list`, and will show the - # correct file-list to programs which do `git diff --cached` expecting to - # see the patch diff. - base_rev = self._Capture(['rev-parse', patch_rev+'~']) - - else: - self.Print('Will cherrypick %r .. %r on top of %r.' % ( - target_rev, patch_rev, base_rev)) - try: - if scm.GIT.IsAncestor(self.checkout_path, patch_rev, target_rev): - # If |patch_rev| is an ancestor of |target_rev|, 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', target_rev + '..' + patch_rev, - '--keep-redundant-commits']) + patch_revs_to_process = [patch_rev] + + if hasattr(options, 'download_topics') and options.download_topics: + # We will now: + # 1. Find the topic of the Gerrit change specified in the patch_rev. + # 2. Find all changes with that topic. + # 3. Append patch_rev of the changes with the same topic to the patch_revs + # to process. + + # Parse the patch_Rev to extract the CL and patchset. + patch_rev_tokens = patch_rev.split('/') + change = patch_rev_tokens[-2] + # Parse the gerrit host out of self.url. + host = self.url.split(os.path.sep)[-1].rstrip('.git') + gerrit_host_url = '%s-review.googlesource.com' % host + + # 1. Find the topic of the Gerrit change specified in the patch_rev. + change_object = gerrit_util.GetChange(gerrit_host_url, change) + topic = change_object.get('topic') + if topic: + # 2. Find all changes with that topic. + changes_with_same_topic = gerrit_util.QueryChanges( + gerrit_host_url, + [('topic', topic), ('status', 'open'), ('repo', host)], + o_params=['ALL_REVISIONS']) + for c in changes_with_same_topic: + if str(c['_number']) == change: + # This change is already in the patch_rev. + continue + self.Print('Found CL %d with the topic name %s' % ( + c['_number'], topic)) + # 3. Append patch_rev of the changes with the same topic to the + # patch_revs to process. + curr_rev = c['current_revision'] + new_patch_rev = c['revisions'][curr_rev]['ref'] + patch_revs_to_process.append(new_patch_rev) - except subprocess2.CalledProcessError as e: - self.Print('Failed to apply patch.') - self.Print('Revision to patch was %r @ %r.' % (patch_repo, patch_rev)) - self.Print('Tried to cherrypick %r .. %r on top of %r.' % ( - target_rev, patch_rev, base_rev)) - self.Print('Current dir is %r' % self.checkout_path) - self.Print('git returned non-zero exit status %s:\n%s' % ( - e.returncode, e.stderr.decode('utf-8'))) - # 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(['reset', '--hard']) + for pr in patch_revs_to_process: + self.Print('===Applying patch===') + self.Print('Revision to patch is %r @ %r.' % (patch_repo, pr)) + self.Print('Current dir is %r' % self.checkout_path) + self._Capture(['fetch', '--no-tags', patch_repo, pr]) + pr = self._Capture(['rev-parse', 'FETCH_HEAD']) + + if not options.rebase_patch_ref: + self._Capture(['checkout', pr]) + # Adjust base_rev to be the first parent of our checked out patch ref; + # This will allow us to correctly extend `file_list`, and will show the + # correct file-list to programs which do `git diff --cached` expecting + # to see the patch diff. + base_rev = self._Capture(['rev-parse', pr+'~']) + else: + self.Print('Will cherrypick %r .. %r on top of %r.' % ( + target_rev, pr, base_rev)) try: - self._Capture(['cherry-pick', '--abort']) - except subprocess2.CalledProcessError: - pass - raise + if scm.GIT.IsAncestor(self.checkout_path, pr, target_rev): + if len(patch_revs_to_process) > 1: + # If there are multiple patch_revs_to_process then we do not want + # want to invalidate a previous patch so throw an error. + raise gclient_utils.Error( + 'patch_rev %s is an ancestor of target_rev %s. This ' + 'situation is unsupported when we need to apply multiple ' + 'patch_revs: %s' % (pr, target_rev, patch_revs_to_process)) + # If |patch_rev| is an ancestor of |target_rev|, check it out. + self._Capture(['checkout', pr]) + 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', target_rev + '..' + pr, + '--keep-redundant-commits']) + + except subprocess2.CalledProcessError as e: + self.Print('Failed to apply patch.') + self.Print('Revision to patch was %r @ %r.' % (patch_repo, pr)) + self.Print('Tried to cherrypick %r .. %r on top of %r.' % ( + target_rev, pr, base_rev)) + self.Print('Current dir is %r' % self.checkout_path) + self.Print('git returned non-zero exit status %s:\n%s' % ( + e.returncode, e.stderr.decode('utf-8'))) + # 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: + self._Capture(['cherry-pick', '--abort']) + except subprocess2.CalledProcessError: + pass + raise - if file_list is not None: - file_list.extend(self._GetDiffFilenames(base_rev)) + if file_list is not None: + file_list.extend(self._GetDiffFilenames(base_rev)) if options.reset_patch_ref: self._Capture(['reset', '--soft', base_rev]) diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index f9a60f159..c4da41190 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -1358,6 +1358,38 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): self.assertNotEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir)) + @mock.patch('gerrit_util.GetChange', return_value={'topic': 'test_topic'}) + @mock.patch('gerrit_util.QueryChanges', return_value=[ + {'_number': 1234}, + {'_number': 1235, 'current_revision': 'abc', + 'revisions': {'abc': {'ref': 'refs/changes/35/1235/1'}}}]) + def testDownloadTopics(self, query_changes_mock, get_change_mock): + 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)) + + # pylint: disable=attribute-defined-outside-init + self.options.download_topics = True + scm.apply_patch_ref( + self.url, 'refs/changes/34/1234/1', 'refs/heads/main', self.options, + file_list) + + get_change_mock.assert_called_once_with( + mock.ANY, '1234') + query_changes_mock.assert_called_once_with( + mock.ANY, + [('topic', 'test_topic'), ('status', 'open'), ('repo', 'repo_1')], + o_params=['ALL_REVISIONS']) + + self.assertCommits([1, 2, 3, 5, 6]) + # The commit hash after the two cherry-picks 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 = []