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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 14, 2024, 14:36 UTC
Message-ID: <ZrzA0yp45w9NuTp2@tanuki>
In-Reply-To: <pull.1772.git.1723641747309.gitgitgadget@gmail.com>

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/

Show 20 quoted lines

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

Show 15 quoted lines

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