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

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

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. "
+            if keep:
-                      "You may resolve it in the working tree now as "
+                printwrap("It is likely that your change has a merge "
-                      "described above and then run 'git review' again, or "
+                          "conflict. You may resolve it in the working tree "
-                      "if you do not want to resolve it yet (note that the "
+                          "now as described above and then run 'git review' "
-                      "change can not merge until the conflict is resolved) "
+                          "again, or if you do not want to resolve it yet "
-                      "you may run 'git rebase --abort' then 'git review -R' "
+                          "(note that the change can not merge until the "
-                      "to upload the change without rebasing.")
+                          "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)

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."""

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.