rebase: apply and cleanup autostash when rebase fails to start

If "git rebase" fails to start after stashing the user's uncommitted
changes then it forgets to restore the stashed changes and remove the
state directory. To make matters worse, running "git rebase --abort" to
apply the stashed changes and cleanup the state directory fails because
the state directory only contains the "autostash" file and is missing
the "head-name" and "onto" files required by read_basic_state().

Fix this by applying the autostash and removing the state directory if
the pre-rebase hook or initial checkout fail. This matches what
finish_rebase() does at the end of a successful rebase. If the user
modifies any files after the autostash is created it is possible there
will be conflicts when the autostash is applied. In that case
apply_autostash() saves the stash in a new entry under refs/stash and so
it is safe to remove the state directory containing the autostash file.

New tests are added to check the autostash is applied and the state
directory is removed if the rebase fails to start. Checks are also added
to some existing tests in order to ensure there is no state directory
left behind when a rebase fails to start and no autostash has been
created.

Reported-by: Brian Lyles <brianmlyles@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e3a8e74cfc25c89243397f62e3b09b8165c3ed6b..ac23c4ddbb00bc17975aa68941f7c6866c9fbf2f 100644 (file)
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -526,6 +526,23 @@ static int rebase_write_basic_state(struct rebase_options *opts)
        return 0;
 }
 
+static int cleanup_autostash(struct rebase_options *opts)
+{
+       int ret;
+       struct strbuf dir = STRBUF_INIT;
+       const char *path = state_dir_path("autostash", opts);
+
+       if (!file_exists(path))
+               return 0;
+       ret = apply_autostash(path);
+       strbuf_addstr(&dir, opts->state_dir);
+       if (remove_dir_recursively(&dir, 0))
+               ret = error_errno(_("could not remove '%s'"), opts->state_dir);
+       strbuf_release(&dir);
+
+       return ret;
+}
+
 static int finish_rebase(struct rebase_options *opts)
 {
        struct strbuf dir = STRBUF_INIT;
@@ -1726,7 +1743,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
        if (require_clean_work_tree(the_repository, "rebase",
                                    _("Please commit or stash them."), 1, 1)) {
                ret = -1;
-               goto cleanup;
+               goto cleanup_autostash;
        }
 
        /*
@@ -1749,7 +1766,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                        if (options.switch_to) {
                                ret = checkout_up_to_date(&options);
                                if (ret)
-                                       goto cleanup;
+                                       goto cleanup_autostash;
                        }
 
                        if (!(options.flags & REBASE_NO_QUIET))
@@ -1775,8 +1792,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
        /* If a hook exists, give it a chance to interrupt*/
        if (!ok_to_skip_pre_rebase &&
            run_hooks_l("pre-rebase", options.upstream_arg,
-                       argc ? argv[0] : NULL, NULL))
-               die(_("The pre-rebase hook refused to rebase."));
+                       argc ? argv[0] : NULL, NULL)) {
+               ret = error(_("The pre-rebase hook refused to rebase."));
+               goto cleanup_autostash;
+       }
 
        if (options.flags & REBASE_DIFFSTAT) {
                struct diff_options opts;
@@ -1821,9 +1840,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
                        RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
        ropts.head_msg = msg.buf;
        ropts.default_reflog_action = options.reflog_action;
-       if (reset_head(the_repository, &ropts))
-               die(_("Could not detach HEAD"));
-       strbuf_release(&msg);
+       if (reset_head(the_repository, &ropts)) {
+               ret = error(_("Could not detach HEAD"));
+               goto cleanup_autostash;
+       }
 
        /*
         * If the onto is a proper descendant of the tip of the branch, then
@@ -1851,9 +1871,14 @@ run_rebase:
 
 cleanup:
        strbuf_release(&buf);
+       strbuf_release(&msg);
        strbuf_release(&revisions);
        rebase_options_release(&options);
        free(squash_onto_name);
        free(keep_base_onto_name);
        return !!ret;
+
+cleanup_autostash:
+       ret |= !!cleanup_autostash(&options);
+       goto cleanup;
 }

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ae34bfad6071fbb40973eb3b01f1d28e360aa0d9..972ea85febcc2f8340f50a06a5ba122d2dab8b27 100755 (executable)
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -145,7 +145,9 @@ test_expect_success 'Show verbose error when HEAD could not be detached' '
        test_when_finished "rm -f B" &&
        test_must_fail git rebase topic 2>output.err >output.out &&
        test_grep "The following untracked working tree files would be overwritten by checkout:" output.err &&
-       test_grep B output.err
+       test_grep B output.err &&
+       test_must_fail git rebase --quit 2>err &&
+       test_grep "no rebase in progress" err
 '
 
 test_expect_success 'fail when upstream arg is missing and not on branch' '
@@ -422,7 +424,9 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '
        git checkout main &&
        git worktree add wt &&
        test_must_fail git -C wt rebase main main 2>err &&
-       test_grep "already used by worktree at" err
+       test_grep "already used by worktree at" err &&
+       test_must_fail git -C wt rebase --quit 2>err &&
+       test_grep "no rebase in progress" err
 '
 
 test_expect_success 'rebase when inside worktree subdirectory' '

diff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh
index e8456831e8ba1a3ff2009ff6339459f6c00055da..426ff098e1dc738a57299a6499bf89174979db37 100755 (executable)
--- a/t/t3413-rebase-hook.sh
+++ b/t/t3413-rebase-hook.sh
@@ -110,7 +110,9 @@ test_expect_success 'pre-rebase hook stops rebase (1)' '
        git reset --hard side &&
        test_must_fail git rebase main &&
        test "z$(git symbolic-ref HEAD)" = zrefs/heads/test &&
-       test 0 = $(git rev-list HEAD...side | wc -l)
+       test 0 = $(git rev-list HEAD...side | wc -l) &&
+       test_must_fail git rebase --quit 2>err &&
+       test_grep "no rebase in progress" err
 '
 
 test_expect_success 'pre-rebase hook stops rebase (2)' '

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 63e400b89f225a48db6552540195d472a142547e..b43046b3b0dfb89740692a16f0e365d091590657 100755 (executable)
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -82,6 +82,46 @@ testrebase () {
        type=$1
        dotest=$2
 
+       test_expect_success "rebase$type: restore autostash when pre-rebase hook fails" '
+               git checkout -f feature-branch &&
+               test_hook pre-rebase <<-\EOF &&
+               exit 1
+               EOF
+
+               echo changed >file0 &&
+               test_must_fail git rebase $type --autostash -f HEAD^ &&
+               test_must_fail git rebase --quit 2>err &&
+               test_grep "no rebase in progress" err &&
+               echo changed >expect &&
+               test_cmp expect file0
+       '
+
+       test_expect_success "rebase$type: restore autostash when checkout onto fails" '
+               git checkout -f --detach feature-branch &&
+               echo uncommitted-content >file0 &&
+               echo untracked >file4 &&
+               test_when_finished "rm file4" &&
+               test_must_fail git rebase $type --autostash \
+                                                       unrelated-onto-branch &&
+               test_must_fail git rebase --quit 2>err &&
+               test_grep "no rebase in progress" err &&
+               echo uncommitted-content >expect &&
+               test_cmp expect file0
+       '
+
+       test_expect_success "rebase$type: restore autostash when branch checkout fails" '
+               git checkout -f unrelated-onto-branch^ &&
+               echo uncommitted-content >file0 &&
+               echo untracked >file4 &&
+               test_when_finished "rm file4" &&
+               test_must_fail git rebase $type --autostash HEAD \
+                                                       unrelated-onto-branch &&
+               test_must_fail git rebase --quit 2>err &&
+               test_grep "no rebase in progress" err &&
+               echo uncommitted-content >expect &&
+               test_cmp expect file0
+       '
+
        test_expect_success "rebase$type: dirty worktree, --no-autostash" '
                test_config rebase.autostash true &&
                git reset --hard &&