From 62619a38110f1a49f7ed40128f843cf4bf803dc1 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 16 Jun 2017 08:22:09 -0700 Subject: [PATCH] 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 Reviewed-by: Dirk Pranke Reviewed-by: Andrii Shyshkalov --- git_cl.py | 53 +++++++++++++++++++++----------------------- tests/git_cl_test.py | 42 ++++------------------------------- 2 files changed, 29 insertions(+), 66 deletions(-) diff --git a/git_cl.py b/git_cl.py index 073410b7e..b80c7fc2a 100755 --- 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') - RunGit(['cherry-pick', 'FETCH_HEAD']) - print('Committed patch for change %i patchset %i locally.' % - (self._changelist.issue, patchset)) - else: + if force: RunGit(['reset', '--hard', 'FETCH_HEAD']) - self.SetIssue(self.GetIssue()) - 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())) + (parsed_issue_arg.issue, patchset)) + else: + RunGit(['cherry-pick', 'FETCH_HEAD']) + print('Committed patch for change %i patchset %i locally.' % + (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) 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): diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 4ae5f17a5..ddb4080af 100755 --- 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()),