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 <sokcevic@google.com>
> > Commit-Queue: Ravi Mistry <rmistry@chromium.org>
>
> Bug: chromium:1298922
> Change-Id: I80747d797234bba06c17ef5c5e85b310281922c4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3484976
> Reviewed-by: Josip Sokcevic <sokcevic@google.com>
> Commit-Queue: Ravi Mistry <rmistry@chromium.org>

Bug: chromium:1298922
Change-Id: I21d7251bafff808b1144d6e522fa9f384f4541bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3490488
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Commit-Queue: Ravi Mistry <rmistry@chromium.org>

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.')
 

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])

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 = []