Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start

[Patrick Steinhardt <ps@pks.im>]

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