From 29f598d554d4fa405f2c2a35dbe4933cfa9a2161 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Fri, 15 Jul 2022 18:21:39 +0000 Subject: [PATCH] Don't keep incomplete rebase state by default Dumping the user into a dirtied working tree after a failed rebase attempt can be confusing, no matter how much contextual explanation we provide when doing so. By default, run `git rebase --abort` automatically so as to clean up from a failed test rebase, and then let the user rebase again on their own if that's the state they want to be in. Add a -K/--keep-rebase option to get the old behavior, and mention it when we automatically abort in case the user wants to just have git-review redo the rebase for them and leave things in that incomplete state. Change-Id: I7d7bfca1623a71a9b4fe445360d94fd6b039f040 --- git-review.1 | 7 +++- git_review/cmd.py | 42 +++++++++++++++---- git_review/tests/test_git_review.py | 17 ++++++++ .../notes/abort-rebase-8bd29d8328818660.yaml | 7 ++++ 4 files changed, 63 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/abort-rebase-8bd29d8328818660.yaml diff --git a/git-review.1 b/git-review.1 index 15a4bd83..14edf801 100644 --- a/git-review.1 +++ b/git-review.1 @@ -131,7 +131,9 @@ If the master branch is different enough, the rebase can produce merge conflicts If that happens rebasing will be aborted and diff displayed for not\-rebased branches. You can also use .Ar \-\-no\-rebase ( Ar \-R ) -to always skip the test rebase. +to always skip the test rebase or +.Ar \-\-keep\-rebase ( Ar \-K ) +to keep the incomplete state rather than having it automatically aborted. .It Fl f , Fl \-finish Close down the local branch and switch back to the target branch on successful submission. @@ -188,6 +190,9 @@ Use the this option to skip the merge conflict test, allowing you to push merge Also can be used for .Fl \-compare to skip automatic rebase of fetched reviews. +.It Fl K , Fl \-keep\-rebase +If the test rebase before pushing fails, keep the incomplete rebase state in +the worktree rather than automatically aborting it to clean up. .It Fl \-no-thin Disable thin pushes when pushing to Gerrit. This should only be used if you are currently experiencing unpack failures due to missing trees. It should diff --git a/git_review/cmd.py b/git_review/cmd.py index e4b2dd5b..094abbae 100644 --- a/git_review/cmd.py +++ b/git_review/cmd.py @@ -900,7 +900,7 @@ def check_remote(branch, remote, scheme, hostname, port, project, raise -def rebase_changes(branch, remote, interactive=True): +def rebase_changes(branch, remote, interactive=True, keep=False): global _orig_head @@ -959,17 +959,35 @@ def rebase_changes(branch, remote, interactive=True): print("Errors running %s" % cmd) if interactive: print(output) - printwrap("It is likely that your change has a merge conflict. " - "You may resolve it in the working tree now as " - "described above and then run 'git review' again, or " - "if you do not want to resolve it yet (note that the " - "change can not merge until the conflict is resolved) " - "you may run 'git rebase --abort' then 'git review -R' " - "to upload the change without rebasing.") + if keep: + printwrap("It is likely that your change has a merge " + "conflict. You may resolve it in the working tree " + "now as described above and then run 'git review' " + "again, or if you do not want to resolve it yet " + "(note that the change can not merge until the " + "conflict is resolved) you may run 'git rebase " + "--abort' then 'git review -R' to upload the change " + "without rebasing.") + else: + printwrap("It is likely that your change has a merge " + "conflict, but the result of the test rebase has " + "been discarded. You may rebase it yourself and " + "review again, or use 'git review -R' to upload the " + "change without resolving the conflict, or use the " + "--keep-rebase option to leave the incomplete " + "rebase result in your working tree.") return False return True +def abort_rebase(): + cmd = "git rebase --abort" + (status, output) = run_command_status(cmd) + if status != 0: + print("Errors running %s" % cmd) + print(output) + + def undo_rebase(): global _orig_head if not _orig_head: @@ -1546,6 +1564,10 @@ additional information: action="store_true", help="Force and push a rebase even when not" " needed.") + rebase_group.add_argument("-K", "--keep-rebase", dest="keep_rebase", + action="store_true", + help="Keep the unfinished test rebase if a " + " merge conflict is detected.") track_group = parser.add_mutually_exclusive_group() track_group.add_argument("--track", dest="track", @@ -1768,7 +1790,9 @@ additional information: return if options.rebase or options.force_rebase: - if not rebase_changes(branch, remote): + if not rebase_changes(branch, remote, keep=options.keep_rebase): + if not options.keep_rebase: + abort_rebase() sys.exit(1) if not options.force_rebase and not undo_rebase(): sys.exit(1) diff --git a/git_review/tests/test_git_review.py b/git_review/tests/test_git_review.py index dcb92323..2bf83804 100644 --- a/git_review/tests/test_git_review.py +++ b/git_review/tests/test_git_review.py @@ -330,6 +330,23 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase): self.assertTrue(rebased) self.assertIn("It is likely that your change has a merge conflict.", exc.args[0]) + exc = self.assertRaises(Exception, self._run_git, 'rebase', '--abort') + self.assertIn("fatal: No rebase in progress?", exc.args[0]) + + def test_keep_aborted_rebase_state(self): + """Test git-review -K behavior when rebase is needed.""" + self._run_git_review('-s') + head_1 = self._run_git('rev-parse', 'HEAD^1') + + self._run_git('checkout', '-b', 'test_branch', head_1) + + self._simple_change('some other message', + 'create conflict with master') + + self.assertRaises(Exception, self._run_git_review, '-K') + output = self._run_git('status') + self.assertIn( + "You are currently editing a commit while rebasing", output) def test_upload_without_rebase(self): """Test change not needing a rebase can upload without rebasing.""" diff --git a/releasenotes/notes/abort-rebase-8bd29d8328818660.yaml b/releasenotes/notes/abort-rebase-8bd29d8328818660.yaml new file mode 100644 index 00000000..d08e4c81 --- /dev/null +++ b/releasenotes/notes/abort-rebase-8bd29d8328818660.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + By default, `git rebase --abort` is run automatically following a failed + test rebase, letting the user rebase again on their own if that's the state + they want to be in. A -K/--keep-rebase option is added to get the old + behavior.