From 63343b07f3471586cba9446d7d16833b49f133f7 Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Fri, 18 Feb 2022 17:34:29 +0000 Subject: [PATCH] Revert "Add support for Gerrit topics in gclient syncs" This reverts commit 0f13273f1f235a8a416e9ddd5f35c20bac268e55. Reason for revert: breaks codesearch recipes and autorollers crbug.com/1298961 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 Bug: chromium:1298961 Change-Id: I88c56cd68372bad09b612de7de1a45f9a0c6c681 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3474793 Bot-Commit: Rubber Stamper Commit-Queue: Josip Sokcevic --- gclient.py | 9 --- gclient_scm.py | 141 +++++++++++++------------------------- tests/gclient_scm_test.py | 32 --------- 3 files changed, 48 insertions(+), 134 deletions(-) diff --git a/gclient.py b/gclient.py index 83fcbbd9dd..e75b94ef21 100755 --- a/gclient.py +++ b/gclient.py @@ -2699,11 +2699,6 @@ 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 ' @@ -2771,10 +2766,6 @@ 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 959d3dc79e..41e8e926a3 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -26,7 +26,6 @@ except ImportError: # For Py3 compatibility import urllib.parse as urlparse import gclient_utils -import gerrit_util import git_cache import scm import shutil @@ -405,101 +404,57 @@ class GitWrapper(SCMWrapper): self._UpdateMirrorIfNotContains(mirror, options, rev_type, target_rev) self._Fetch(options, refspec=target_rev) - 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) - + 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']) - 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)) + 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']) + + 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'])) try: - 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 + 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 c4da411900..f9a60f1593 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -1358,38 +1358,6 @@ 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 = []