From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 15 Apr 2026 09:36:32 GMT
Subject: Re: [PATCH] checkout: add --autostash option for branch switching
Message-ID: <21c205c6-687f-41b4-9f43-22e6ea6928b3@gmail.com>
In-Reply-To: <20260415081659.86783-1-haraldnordgren@gmail.com>

On 15/04/2026 09:16, Harald Nordgren wrote:
>>> +	if (old_branch_info.name)
>>> +		stash_label_base = old_branch_info.name;
>>> +	else if (old_branch_info.commit) {
>>> +		strbuf_add_unique_abbrev(&old_commit_shortname,
>>> +					 &old_branch_info.commit->object.oid,
>>> +					 DEFAULT_ABBREV);
>>> +		stash_label_base = old_commit_shortname.buf;
>>> +	}
>>> +
>>>    	if (do_merge) {
>>>    		ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
>>> +		if (ret && opts->merge) {
>>
>> As we saw above merge_working_tree() can return non-zero for a variety
>> of reasons. We only want to try stashing if the call to unpack_trees()
>> failed. Even then if you look at the list of errors in unpack-trees.h
>> you'll see that only a few of them relate to problems that can be solved
>> by stashing. The old code just tried merging whenever unpack_trees()
>> failed so it probably not so bad to do the same here but we should not
>> be stashing if merge_working_tree() returns before calling unpack_trees().
> 
> What you are saying makes a lot of sense.
> 
> I gave this a shot now, trying to return an error code that only attempts
> the stashing when it has a chance of improving the outcome. Not at all sure
> if it's correct though!

That sounds like the right approach

>>> +						   autostash_msg.buf);
>>> +			created_autostash = 1;
>>> +			ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
>>> +		}
>>>    		if (ret) {
>>
>> I'm confused by this - if we stash then don't we expect the call to
>> unpack_trees() in merge_working_tree() to succeed and therefore return
>> 0? If opts->merge is false then we should not be trying to apply the
>> stash when merge_working_tree() fails.
> 
> I'm attempting to fix this by making call to apply_autostash_ref
> conditional on whether or not the autostash was actually created. Makes
> sense?

Yes, exactly

Thanks

Phillip