Re: [PATCH] checkout: add --autostash option for branch switching

[Harald Nordgren <haraldnordgren@gmail.com>]

> > +	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!

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


Harald