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

From: Denton Liu <[email-masked]>
Date: Fri, 3 Apr 2020 17:14:58 -0400

Hi Phillip,

On Fri, Apr 03, 2020 at 02:09:26PM +0100, Phillip Wood wrote:
> 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 <[email-masked]>
> > 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.

Great, I'll use this approach then.

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

This makes me a little bit sad because we basically have to duplicate
the file-handling portion of apply_save_autostash() but I agree that
doing your way should be better.

> > +		}
> > +
> >   		/* 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?

This is the message that is printed:

	$ git reset --hard
	HEAD is now at c4c4222 commit 1
	Autostash exists; creating a new stash entry.
	Your changes are safe in the stash.
	You can run "git stash pop" or "git stash drop" at any time.

Thanks,

Denton

> 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;
> > > > >    }