git-cl-patch: Return to cherry-pick, but hard reset with --force

This is a partial revert of https://chromium-review.googlesource.com/c/527345/ Turns out more people were confused by the new behavior than expected, so we're returning to "cherry-pick" being the default, but supporting the collaboration workflow is important, so we're adding a warning message and support for "reset --hard" behind a pre-existing flag. Bug: 723787 Change-Id: Ib6038a42e3bdcc0db93c1f32d759e9ff0e91a065 Reviewed-on: https://chromium-review.googlesource.com/538137 Commit-Queue: Aaron Gable <agable@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
8 years ago · 62619a3811
parent 9df9e9f8f9
commit 62619a3811
2 changed files with 29 additions and 66 deletions
--- a/git_cl.py
+++ b/git_cl.py
@ -1581,7 +1581,7 @@ class Changelist(object):
      DieWithError('Failed to parse issue argument "%s". '
                   'Must be an issue number or a valid URL.' % issue_arg)
    return self._codereview_impl.CMDPatchWithParsedIssue(
-        parsed_issue_arg, reject, nocommit, directory)
+        parsed_issue_arg, reject, nocommit, directory, False)

  def CMDUpload(self, options, git_diff_args, orig_args):
    """Uploads a change to codereview."""
@ -1832,7 +1832,7 @@ class _ChangelistCodereviewBase(object):
    raise NotImplementedError()

  def CMDPatchWithParsedIssue(self, parsed_issue_arg, reject, nocommit,
-                              directory):
+                              directory, force):
    """Fetches and applies the issue.

    Arguments:
@ -1842,6 +1842,7 @@ class _ChangelistCodereviewBase(object):
      nocommit: do not commit the patch, thus leave the tree dirty. Rietveld
        only.
      directory: switch to directory before applying the patch. Rietveld only.
+      force: if true, overwrites existing local state.
    """
    raise NotImplementedError()

@ -2128,7 +2129,7 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
      self.SetFlags({'commit': '1', 'cq_dry_run': '1'})

  def CMDPatchWithParsedIssue(self, parsed_issue_arg, reject, nocommit,
-                              directory):
+                              directory, force):
    # PatchIssue should never be called with a dirty tree.  It is up to the
    # caller to check this, but just in case we assert here since the
    # consequences of the caller not checking this could be dire.
@ -2765,7 +2766,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
    return 0

  def CMDPatchWithParsedIssue(self, parsed_issue_arg, reject, nocommit,
-                              directory):
+                              directory, force):
    assert not reject
    assert not nocommit
    assert not directory
@ -2798,29 +2799,24 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
    fetch_info = revision_info['fetch']['http']
    RunGit(['fetch', fetch_info['url'], fetch_info['ref']])

-    clean, _ = RunGitWithCode(
-        ['merge-base', '--is-ancestor', 'HEAD', 'origin/master'])
-    if clean != 0:
-      clean, _ = RunGitWithCode(
-          ['merge-base', '--is-ancestor', 'HEAD', 'FETCH_HEAD'])
-    if clean != 0:
-      confirm_or_exit(
-          'It looks like you\'re on a branch with some local commits.\n'
-          'If you apply this patch on top of your local content, you will not '
-          'be able to easily upload further changes based on it.\n'
-          'Would you like to proceed with applying this patch anyway?\n')
+    if force:
+      RunGit(['reset', '--hard', 'FETCH_HEAD'])
+      print('Checked out commit for change %i patchset %i locally' %
+            (parsed_issue_arg.issue, patchset))
+    else:
      RunGit(['cherry-pick', 'FETCH_HEAD'])
      print('Committed patch for change %i patchset %i locally.' %
-            (self._changelist.issue, patchset))
-    else:
-      RunGit(['reset', '--hard', 'FETCH_HEAD'])
-      self.SetIssue(self.GetIssue())
+            (parsed_issue_arg.issue, patchset))
+      print('Note: this created a local commit which does not have '
+            'the same hash as the one uploaded for review. This will make '
+            'uploading changes based on top of this branch difficult.\n'
+            'If you want to do that, use "git cl patch --force" instead.')
+
+    self.SetIssue(parsed_issue_arg.issue)
    self.SetPatchset(patchset)
    fetched_hash = RunGit(['rev-parse', 'FETCH_HEAD']).strip()
    self._GitSetBranchConfigValue('last-upload-hash', fetched_hash)
    self._GitSetBranchConfigValue('gerritsquashhash', fetched_hash)
-      print('Checked out commit for change %i patchset %i locally' %
-            (self.GetIssue(), self.GetPatchset()))
    return 0

  @staticmethod
@ -5256,9 +5252,9 @@ def CMDpatch(parser, args):
  parser.add_option('-b', dest='newbranch',
                    help='create a new branch off trunk for the patch')
  parser.add_option('-f', '--force', action='store_true',
-                    help='with -b, clobber any existing branch')
+                    help='overwrite state on the current or chosen branch')
  parser.add_option('-d', '--directory', action='store', metavar='DIR',
-                    help='Change to the directory DIR immediately, '
+                    help='change to the directory DIR immediately, '
                         'before doing anything else. Rietveld only.')
  parser.add_option('--reject', action='store_true',
                    help='failed patches spew .rej files rather than '
@ -5356,7 +5352,8 @@ def CMDpatch(parser, args):
          (cl.GetIssueURL(), target_issue_arg.codereview))

  return cl.CMDPatchWithParsedIssue(target_issue_arg, options.reject,
-                                    options.nocommit, options.directory)
+                                    options.nocommit, options.directory,
+                                    options.force)


 def GetTreeStatus(url=None):
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@ -2127,8 +2127,7 @@ class TestGitCl(TestCase):
    self.calls += [
      ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
         'refs/changes/56/123456/7'],), ''),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'origin/master'],), ''),
-      ((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
      ((['git', 'config', 'branch.master.gerritissue', '123456'],),
       ''),
      ((['git', 'config', 'branch.master.gerritserver',
@ -2146,7 +2145,6 @@ class TestGitCl(TestCase):
    self.calls += [
      ((['git', 'fetch', 'https://host.googlesource.com/my/repo',
         'refs/changes/56/123456/7'],), ''),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'origin/master'],), ''),
      ((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
      ((['git', 'config', 'branch.master.gerritissue', '123456'],),
       ''),
@ -2157,7 +2155,7 @@ class TestGitCl(TestCase):
      ((['git', 'config', 'branch.master.last-upload-hash', 'deadbeef'],), ''),
      ((['git', 'config', 'branch.master.gerritsquashhash', 'deadbeef'],), ''),
    ]
-    self.assertEqual(git_cl.main(['patch', '--gerrit', '123456']), 0)
+    self.assertEqual(git_cl.main(['patch', '--gerrit', '123456', '--force']), 0)

  def test_patch_gerrit_guess_by_url(self):
    self._patch_common(
@ -2167,8 +2165,7 @@ class TestGitCl(TestCase):
    self.calls += [
      ((['git', 'fetch', 'https://else.googlesource.com/my/repo',
         'refs/changes/56/123456/1'],), ''),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'origin/master'],), ''),
-      ((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
      ((['git', 'config', 'branch.master.gerritissue', '123456'],),
       ''),
@ -2190,8 +2187,7 @@ class TestGitCl(TestCase):
    self.calls += [
      ((['git', 'fetch', 'https://else.googlesource.com/my/repo',
         'refs/changes/56/123456/1'],), ''),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'origin/master'],), ''),
-      ((['git', 'reset', '--hard', 'FETCH_HEAD'],), ''),
+      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
      ((['git', 'config', 'branch.master.gerritissue', '123456'],),
       ''),
@ -2205,42 +2201,12 @@ class TestGitCl(TestCase):
    self.assertEqual(git_cl.main(
      ['patch', 'https://else-review.googlesource.com/123456/1']), 0)

-  def test_patch_gerrit_local_content(self):
-    self._patch_common(default_codereview='gerrit', detect_gerrit_server=True,
-                       git_short_host='chromium')
-    self.calls += [
-      ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
-         'refs/changes/56/123456/7'],), ''),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'origin/master'],),
-       CERR1),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'FETCH_HEAD'],),
-       CERR1),
-      (('ask_for_data',
-        'It looks like you\'re on a branch with some local commits.\n'
-        'If you apply this patch on top of your local content, you will not be '
-        'able to easily upload further changes based on it.\n'
-        'Would you like to proceed with applying this patch anyway?\n'
-        'Press Enter to confirm, or Ctrl+C to abort'), 'y'),
-      ((['git', 'cherry-pick', 'FETCH_HEAD'],), ''),
-    ]
-    self.assertEqual(git_cl.main(['patch', '123456']), 0)
-
  def test_patch_gerrit_conflict(self):
    self._patch_common(default_codereview='gerrit', detect_gerrit_server=True,
                       git_short_host='chromium')
    self.calls += [
      ((['git', 'fetch', 'https://chromium.googlesource.com/my/repo',
         'refs/changes/56/123456/7'],), ''),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'origin/master'],),
-       CERR1),
-      ((['git', 'merge-base', '--is-ancestor', 'HEAD', 'FETCH_HEAD'],),
-       CERR1),
-      (('ask_for_data',
-        'It looks like you\'re on a branch with some local commits.\n'
-        'If you apply this patch on top of your local content, you will not be '
-        'able to easily upload further changes based on it.\n'
-        'Would you like to proceed with applying this patch anyway?\n'
-        'Press Enter to confirm, or Ctrl+C to abort'), 'y'),
      ((['git', 'cherry-pick', 'FETCH_HEAD'],), CERR1),
      ((['DieWithError', 'Command "git cherry-pick FETCH_HEAD" failed.\n'],),
       SystemExitMock()),