Re: [PATCH v12 4/4] checkout: -m (--merge) uses autostash when switching branches

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Harald
>
> For the subject line I think
>
>      checkout -m: autostash when switching branches
>
> would be more in keeping with our usual style.

Thanks.  I agree.  Alternatively, 

    checkout: autostash when switching branches with -m

would also work.

>> When switching branches with "git checkout -m", local modifications
>> can block the switch. 
>
> Really? Isn't the point of "checkout -m" to merge the local 
> modifications into the branch that's being checked out?

Yeah, either "git checkout -m" -> "git checkout" (without "-m")
or "can block" -> "can cause conflict during".  Also, I think a bit
more description of what happens in the current system without this
patch series would clarify the motivation.  Perhaps something like...

    When switching branches with "git checkout -m", the attempted
    merge of local modifications may cause conflicts with the
    changes made on the other branch, which the user may not want to
    (or may not be able to) resolve right now.  Because there is no
    easy way to recover from this situation, we discouraged users from
    using "checkout -m" unless they are certain their changes are
    trivial and within their ability to resolve conflicts.

... would contrast well with "the user can resolve or reset and
postpone 'stash pop' to some later time" we will give at the end of
the message.

>> Teach the -m flow to create a temporary stash
>> before switching and reapply it after.  On success, only "Applied
>> autostash." is shown. 
>
> and a diff of the local changes?

I noticed that we show the same output as a successful "git checkout
other-branch" without "-m" shows, i.e., short list of modified
paths.  I am not sure what it is officially called but calling it
"diff" is probably misleading.

>> If reapplying causes conflicts, the stash is
>> kept and the user is told they can resolve and run "git stash drop",
>> or run "git reset --hard" and later "git stash pop" to recover their
>> changes.

Good.

>> @@ -814,6 +820,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>>   		refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
>>   
>>   		if (unmerged_index(the_repository->index)) {
>> +			rollback_lock_file(&lock_file);
>>   			error(_("you need to resolve your current index first"));
>>   			return 1;
>
> The changes up to here look like fixes for an existing bug and so would 
> be better in a separate patch.

Good point.

> Sometimes we return "1" and sometimes "-1" what does that signal to the 
> caller?

This too.

>> @@ -846,82 +853,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
>>   		ret = unpack_trees(2, trees, &topts);
>>   		clear_unpack_trees_porcelain(&topts);
>>   		if (ret == -1) {
>> [lots of deletions]
>> -			if (ret)
>> -				return ret;
>> +			rollback_lock_file(&lock_file);
>> +			return 1;
>>   		}
>>   	}
>>   
>> @@ -1166,6 +1099,10 @@ static int switch_branches(const struct checkout_opts *opts,
>>   	struct object_id rev;
>>   	int flag, writeout_error = 0;
>>   	int do_merge = 1;
>> +	int created_autostash = 0;
>
> This can be a bool

So are many things (like do_merge, writeout_error).  I wouldn't
worry too much about the difference between int and bool unless it
is a function parameter.

>> +			strbuf_addf(&autostash_msg,
>> +				    "autostash while switching to '%s'",
>> +				    new_branch_info->name);
>> +			create_autostash_ref_with_msg_silent(the_repository,
>> +						   "CHECKOUT_AUTOSTASH_HEAD",
>
> It's a shame we have to create a ref here. MERGE_AUTOSTASH exists so 
> that "git merge --continue" can apply the stash once the user has 
> resolved any merge conflicts. We don't have that problem here because 
> there is no user interaction and we could just hold onto the stash oid 
> in a variable.

Hmph, so it is not a shame and we can do without it?