gclient_scm: Add option to specify the branch the patch is based on.

Also add shortcuts, so we don't have to look through all the branches
if the patch is based on a common branch (i.e. master, infra/config, lkgr)


Bug: 870279
Change-Id: I625a8481dccac9a475b096b926e6fab7efe676b0
Reviewed-on: https://chromium-review.googlesource.com/1161094
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

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 '

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

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)

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

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

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