On Wed, Aug 14, 2024 at 01:22:27PM +0000, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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/
> 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?
> @@ -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
