Revert "Reland "gclient: Require a target ref when applying patches.""

This reverts commit 822dbc51a2.

Reason for revert:
Skia autoroller is complaining
https://skia-review.googlesource.com/c/buildbot/+/219338

Original change's description:
> Reland "gclient: Require a target ref when applying patches."
> 
> This is a reland of 1d6478a5ff
> 
> Original change's description:
> > gclient: Require a target ref when applying patches.
> > 
> > Bug: 956807
> > Change-Id: Icfffe965f9f4651f22e8ba32c60133a5620bb350
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1616804
> > Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
> > Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
> 
> Bug: 956807
> Change-Id: I3de8475a091ce6a2a14ff7dcfb92507a205ef78c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1623594
> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

TBR=borenet@chromium.org,tandrii@chromium.org,ehmaldonado@chromium.org

Change-Id: I14a70206a34f7024abc21039806f23dc7cf56bb6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 956807
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1648321
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

gclient.py

@@ -1574,12 +1574,13 @@ it or fix the checkout.
       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 or ':' not in patch_ref:
+      if not patch_repo or not patch_ref:
         raise gclient_utils.Error(
             'Wrong revision format: %s should be of the form '
-            'patch_repo@target_branch:patch_ref.' % given_patch_ref)
-      target_branch, _, patch_ref = patch_ref.partition(':')
-      target_branches[patch_repo] = target_branch
+            '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, target_branches
 
@@ -2597,7 +2598,7 @@ def CMDsync(parser, args):
   parser.add_option('--patch-ref', action='append',
                     dest='patch_refs', metavar='GERRIT_REF', default=[],
                     help='Patches the given reference with the format '
-                         'dep@target-ref:patch-ref. '
+                         '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 '
@@ -2607,7 +2608,10 @@ def CMDsync(parser, args):
                          '|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.')
+                         '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

@@ -351,6 +351,28 @@ class GitWrapper(SCMWrapper):
               self.Print('FAILED to break lock: %s: %s' % (to_break, ex))
               raise
 
+  # 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."""
+    _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,
+         '--format=%(refname)']).splitlines()
+    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. '
+               'Candidate refs were %s.' % (commit, remote_refs))
+    return None
+
   def apply_patch_ref(self, patch_repo, patch_rev, target_rev, options,
                       file_list):
     """Apply a patch on top of the revision we're synced at.
@@ -397,7 +419,8 @@ class GitWrapper(SCMWrapper):
     base_rev = self._Capture(['rev-parse', 'HEAD'])
 
     if not target_rev:
-      raise gclient_utils.Error('A target revision for the patch must be given')
+      # TODO(ehmaldonado): Raise an error once |target_rev| is mandatory.
+      target_rev = self._GetTargetBranchForCommit(base_rev) or base_rev
     elif target_rev.startswith('refs/heads/'):
       # If |target_rev| is in refs/heads/**, try first to find the corresponding
       # remote ref for it, since |target_rev| might point to a local ref which

tests/gclient_scm_test.py

@@ -1168,9 +1168,8 @@ 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', 'refs/heads/master', self.options,
-        file_list)
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options,
+                        file_list)
 
     self.assertCommits([1, 2, 3, 4, 5, 6])
     self.assertEqual(self.githash('repo_1', 4), self.gitrevparse(self.root_dir))
@@ -1191,9 +1190,8 @@ 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', 'refs/heads/master', self.options,
-        file_list)
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options,
+                        file_list)
 
     self.assertCommits([1, 5, 6])
     self.assertEqual(self.githash('repo_1', 1), self.gitrevparse(self.root_dir))
@@ -1209,9 +1207,8 @@ 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', 'refs/heads/feature', self.options,
-        file_list)
+    scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', None, self.options,
+                        file_list)
 
     self.assertCommits([1, 2, 7, 8, 9, 10])
     self.assertEqual(self.githash('repo_1', 9), self.gitrevparse(self.root_dir))
@@ -1228,9 +1225,8 @@ 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', 'refs/heads/feature', self.options,
-        file_list)
+    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
     # remote's master branch and the change) but on top of 7 (which is the
@@ -1249,9 +1245,8 @@ class GerritChangesTest(fake_repos.FakeReposTestBase):
 
     # Apply refs/changes/34/1234/1, created for remote's master branch on top of
     # remote's feature branch.
-    scm.apply_patch_ref(
-        self.url, 'refs/changes/35/1235/1', 'refs/heads/master', self.options,
-        file_list)
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', 'refs/heads/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 remote's feature branch.
@@ -1269,9 +1264,8 @@ 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', 'refs/heads/master', self.options,
-        file_list)
+    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', 4), self.gitrevparse(self.root_dir))
@@ -1289,9 +1283,8 @@ 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', 'refs/heads/master', self.options,
-        file_list)
+    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', 1), self.gitrevparse(self.root_dir))
@@ -1305,9 +1298,8 @@ 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', 'refs/heads/master', self.options,
-        file_list)
+    scm.apply_patch_ref(self.url, 'refs/changes/35/1235/1', None, self.options,
+                        file_list)
 
     self.assertCommits([1, 2, 3, 4, 5, 6])
     # The commit hash after cherry-picking is not known, but it must be
@@ -1326,17 +1318,15 @@ 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', 'refs/heads/master', 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', 'refs/heads/master', self.options,
-        file_list)
+    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))
 
@@ -1353,9 +1343,8 @@ 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', 'refs/heads/feature', self.options,
-        file_list)
+    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),
                      self.gitrevparse(self.root_dir))
@@ -1377,9 +1366,8 @@ 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', 'refs/heads/master', self.options,
-        file_list)
+    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

@@ -589,6 +589,29 @@ class GClientSmokeGIT(GClientSmokeBase):
     self.assertTree(tree)
 
   def testSyncPatchRef(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@%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 testSyncPatchRefBranch(self):
     if not self.enabled:
       return
     self.gclient(['config', self.git_base + 'repo_1', '--name', 'src'])
@@ -620,8 +643,7 @@ class GClientSmokeGIT(GClientSmokeBase):
         '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)),
+        '%srepo_2@%s' % (self.git_base, self.githash('repo_2', 2)),
         '--nohooks',
     ])
     # Assert that repo_2 files coincide with revision @2 (the patch ref)