Re: [PATCH v12 18/26] stash: convert push to builtin

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> >> > index c77f62c895..3dab488bd6 100644
> >> > --- a/builtin/stash--helper.c
> >> > +++ b/builtin/stash--helper.c
> >> > @@ -231,6 +231,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
> >> >  	struct tree *tree;
> >> >  	struct lock_file lock_file = LOCK_INIT;
> >> >  
> >> > +	discard_cache();
> >> >  	read_cache_preload(NULL);
> >> >  	if (refresh_cache(REFRESH_QUIET))
> >> >  		return -1;
> >> > 
> >
> > So this is working, but it is not the correct spot for that
> > `discard_cache()`, as it forces unnecessary cycles on code paths calling
> > `reset_tree()` (which corresponds to `git read-tree`, admittedly a bit
> > confusing) with a fully up to date index.
> >
> > The real fix, I believe, is this:
> >
> > -- snip --
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index 2d6dfce883..516dee0fa4 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -1372,6 +1372,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
> >  			}
> >  		} else {
> >  			struct child_process cp = CHILD_PROCESS_INIT;
> > +			discard_cache();
> >  			cp.git_cmd = 1;
> >  			argv_array_pushl(&cp.args, "reset", "--hard", "-q",
> >  					 NULL);
> > -- snap --
> >
> > And the reason this is needed: we spawn a `git reset --hard` here, which
> > will change the index, but outside of the current process. So the
> > in-process copy is stale. And when the index' mtime does not help us
> > detect that, we run into that test breakage.
> 
> In non-patch mode with pathspec, there is an invocation of "apply
> --index -R" of a patch that takes the contents of the HEAD to what
> is in the index, updating the on-disk index and making our in-core
> copy stale.  Wouldn't we need to do the same?  Otherwise, the same
> "reset_tree()" you are tryhing to protect with this discard_cache()
> will call read_cache_preload(), no?
> 
> Among the calls to reset_tree() in this file, I think the one that
> follows the "reset --hard" (your fix above) and "apply --index -R"
> (the other side of the same if/else) is the only one that wants to
> read from the result of an external command we just spawned from the
> on-disk index, so perhaps moving discard_cache() to just before that
> call may be a better fix.