Re: [PATCH v3 17/19] merge: teach --autostash option

From: Phillip Wood <phillip.wood123@gmail.com>
To: Denton Liu <liu.denton@gmail.com>, phillip.wood@dunelm.org.uk
Cc: Git Mailing List <git@vger.kernel.org>,
	Alban Gruin <alban.gruin@gmail.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3 17/19] merge: teach --autostash option
Date: Fri, 3 Apr 2020 14:09:26 +0100	[thread overview]
Message-ID: <faa275d4-ca5d-9ab9-cbcb-aebd5e53e20b@gmail.com> (raw)
In-Reply-To: <20200403105639.GA3193506@generichostname>

Hi Denton

On 03/04/2020 11:56, Denton Liu wrote:
> On Fri, Apr 03, 2020 at 06:31:26AM -0400, Denton Liu wrote:
>>>> diff --git a/builtin/reset.c b/builtin/reset.c
>>>> index 18228c312e..038c8532eb 100644
>>>> --- a/builtin/reset.c
>>>> +++ b/builtin/reset.c
>>>> @@ -25,6 +25,7 @@
>>>>    #include "cache-tree.h"
>>>>    #include "submodule.h"
>>>>    #include "submodule-config.h"
>>>> +#include "sequencer.h"
>>>>    #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
>>>> @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>>>>    		if (reset_type == HARD && !update_ref_status && !quiet)
>>>>    			print_new_head_line(lookup_commit_reference(the_repository, &oid));
>>>>    	}
>>>> -	if (!pathspec.nr)
>>>> +	if (!pathspec.nr) {
>>>> +		if (reset_type == HARD)
>>>> +			save_autostash(git_path_merge_autostash(the_repository));
>>>> +
>>>>    		remove_branch_state(the_repository, 0);
>>>
>>> This removes the autostash file for all reset types but we only keep the
>>> stash in the case of 'reset --hard' which is confusing.
>>
>> I was worried that this change would be controversial... The rationale
>> behind this change was that with `reset --hard`, we want to leave a
>> clean working tree behind so we save it into the stash reflog. In all
>> other cases, remove_branch_state() will apply the saved stash entry
>> which should be fine since users don't expect a clean worktree with the
>> other reset types.
>>
>> I considered saving the autostash in all cases of reset but
>> `git merge --abort` invokes `git reset --merge` behind the scenes so
>> we'd have to consider that. Perhaps we can make all resets save the
>> stash entry and, in the case of `merge --abort`, we can add some extra
>> logic to subvert this so that the stash entry is applied?
> 
> Perhaps something like this?
> 
> -- >8 --
> commit 14d0b569cb7675f00d32d3d7fad7564fcaeca458
> Author: Denton Liu <liu.denton@gmail.com>
> Date:   Fri Apr 3 06:50:34 2020 -0400
> 
>      squash! merge: teach --autostash option
>      
>      Stash is saved when any reset is run, when git merge --abort is run,
>      stash is applied.

I think this is the easiest behavior to understand, it avoids changing 
the behavior of reset, in particular it stops 'reset --mixed/--soft' 
from suddenly starting to touch the working tree.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 9573d77096..31b82d614c 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1242,6 +1242,8 @@ static int merging_a_throwaway_tag(struct commit *commit)
>   	return is_throwaway_tag;
>   }
>   
> +static GIT_PATH_FUNC(git_path_merge_autostash_saved, "MERGE_AUTOSTASH_SAVED")
> +
>   int cmd_merge(int argc, const char **argv, const char *prefix)
>   {
>   	struct object_id result_tree, stash, head_oid;
> @@ -1295,9 +1297,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>   		if (!file_exists(git_path_merge_head(the_repository)))
>   			die(_("There is no merge to abort (MERGE_HEAD missing)."));
>   
> +		if (file_exists(git_path_merge_autostash(the_repository))) {
> +			if (rename(git_path_merge_autostash(the_repository),
> +						git_path_merge_autostash_saved()))
> +				die_errno(_("failed to rename autostash"));

This is a bit of a performance, can't we just remember the stash oid in 
a variable and tweak the apply code?

> +		}
> +
>   		/* Invoke 'git reset --merge' */
>   		ret = cmd_reset(nargc, nargv, prefix);
> -		apply_autostash(git_path_merge_autostash(the_repository));
> +
> +		apply_autostash(git_path_merge_autostash_saved());

Calling cmd_reset() was already a bit dodgy by our normal rules, now 
we're calling other functions after it though I guess given the current 
autostash implementation it's mostly in a separate process.

I think this is a good direction to go in
BTW what message gets printed when the stash is saved?

Best Wishes

Phillip

>   		goto done;
>   	}
>   
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 038c8532eb..060470c455 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -439,9 +439,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>   			print_new_head_line(lookup_commit_reference(the_repository, &oid));
>   	}
>   	if (!pathspec.nr) {
> -		if (reset_type == HARD)
> -			save_autostash(git_path_merge_autostash(the_repository));
> -
> +		save_autostash(git_path_merge_autostash(the_repository));
>   		remove_branch_state(the_repository, 0);
>   	}
>   
>>
>> I'm not sure about what the most intuitive behaviour is. I thought that
>> this implementation would be the best but I'm not entirely sure. I'd
>> appreciate some more discussion on this.
>>
>> Thanks,
>>
>> Denton
>>
>>> Best Wishes
>>>
>>> Phillip
>>>
>>>> +	}
>>>>    	return update_ref_status;
>>>>    }