Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start
From: Patrick Steinhardt <hidden>
Date: 2024-08-14 14:36:09
On Wed, Aug 14, 2024 at 01:22:27PM +0000, Phillip Wood via GitGitGadget wrote:
From: Phillip Wood <redacted> If "git rebase" fails to start after stashing the user's uncommitted changes then it forgets to restore the stashed changes and remove state
s/remove/& the/
directory. To make matters worse running "git rebase --abort" to apply
s/worse/&,/
the stashed changes and cleanup the state directory fails because the
s/cleanup/& of/
quoted hunk ↗ jump to hunk
diff --git a/builtin/rebase.c b/builtin/rebase.c index e3a8e74cfc2..ac23c4ddbb0 100644 --- 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);
This seems somewhat dangerous to me though. Should we really delete the autostash directory in case applying the autostash has failed?
quoted hunk ↗ jump to hunk
@@ -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; }
It's somewhat curious of a code flow to jump backwards. It might be easier to reason about the flow if we kept track of a variable `autostash_needs_cleanup` that we set such that all jumps can go to the `cleanup` label instead. Patrick