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

[Denton Liu <liu.denton@gmail.com>]

Hi Phillip,

Thanks for your detailed review on the series. I've updated everything
according to your comments except for the parts below:

On Thu, Apr 02, 2020 at 04:24:54PM +0100, Phillip Wood wrote:
> > diff --git a/branch.c b/branch.c
> > index 579494738a..bf2536c70d 100644
> > --- a/branch.c
> > +++ b/branch.c
> > @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r)
> >   	unlink(git_path_merge_rr(r));
> >   	unlink(git_path_merge_msg(r));
> >   	unlink(git_path_merge_mode(r));
> > +	apply_autostash(git_path_merge_autostash(r));
> >   }
> >   void remove_branch_state(struct repository *r, int verbose)
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 7ba33a3bec..c11894423a 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -1687,6 +1687,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> >   	unlink(git_path_merge_mode(the_repository));
> >   	unlink(git_path_squash_msg(the_repository));
> > +	apply_autostash(git_path_merge_autostash(the_repository));
> > +
> 
> It's hard to tell from the limited context but do we want to run
> commit_index_files() before applying the autostash?

I don't think it really matters which order we run it in. When we run
apply_autostash(), we only ever touch the working tree, not the index so
it doesn't matter if it's run before or after. I'd prefer to keep it
here because if we ever refactor this to use
remove_merge_branch_state(), the person working on this will be able to
perform the refactor more easily without having to worry about implicit
ordering dependencies.

> I wonder if this should
> be using remove_merge_branch_state() instead of duplicating it as well.

We can _almost_ use remove_branch_state() even! Unfortunately,
remove_merge_branch_state() calls `unlink(git_path_merge_rr(r))` which
we cannot do since repo_rerere() relies on it later. Perhaps we can
can move repo_rerere() earlier?
 
> > 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?

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