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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Patrick

On 14/08/2024 15:36, Patrick Steinhardt wrote:
> 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/

Thanks for catching those typos

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

Applying the stash should not fail because the rebase has not started 
and so HEAD, the index and the worktree are unchanged since the stash 
was created. If it does fail for some reason then apply_autostash() 
creates a new entry under refs/stash. We definitely do want to remove 
the directory otherwise we're left with the inconsistent state we're 
tying to fix.

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

It is slightly odd, but in the end I decided it was simpler to say "goto 
cleanup_autostash" than try and keep track of what needs cleaning up 
when saying "goto cleanup". There are a couple of similar examples in 
builtin/stash.c:show_stash() and 
config.c:git_config_set_multivar_in_file_gently()

Thanks for the review

Phillip


> Patrick
>