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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 02/19, Johannes Schindelin wrote:
> Hi Gábor & Thomas,
> 
> On Tue, 19 Feb 2019, SZEDER Gábor wrote:
> 
> > > I'll hopefully be in a position to
> > > send a patch with a proper log message why this is the right fix in
> > > the next couple of days.
> > > 
> > > 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.

Right, I agree with this assessment, and also agree that this is a
better place to discard the cache, rather than doing it in
'reset_tree()'.

> Now, I seriously believe that we missed the best time to move
> ps/stash-in-c into `next` for cooking. The best time would have been just
> after Paul submitted the latest patch series: we know for a fact that he
> is too busy to really take care of this patch series, so keeping it in
> `pu` puts everybody into that awkward spot where nobody wants to step on
> Paul's toes messing with his patch series, but where Paul also lacks the
> time to push it further, so everything is stuck in a limbo and is *so very
> much* not cooking at all. You might say that it has turned bad because we
> failed to stoke the fire appropriately.
> 
> Since it is now way too late in the v2.21.0 process, this problem is only
> exacerbated, because it won't even enter `next` "better late than never".
>
> To address this unfortunate situation, my current plan is to take over
> from Paul (we had been chatting about this privately in the past, and he
> is okay with this because of University eating all his time).
> 
> I will open the whole bag again, most likely squashing the late fixups
> into the patches that introduced the problems, re-review with a much finer
> comb than the patch series has enjoyed on the Git mailing list (even just
> a quick look at `do_apply_stash()` revealed an unnecessary `reset_tree()`
> call that *no* reviewer spotted, even I myself, but then, I am hardly
> solely responsible for that review), and most likely I'll even take my
> sweet little time changing the code to avoid more spawned Git processes.
> 
> It will take a long time, and the `stash` project that has been discussed
> recently to be given to GSoC students is no longer available, as I will
> take care of it before GSoC even starts, and I won't spend much time
> reviewing other people's code in the meantime.
>
> I will start that only after v2.21.0 final is out, obviously.
> 
> Once I submit a new iteration, it will look quite a bit different from
> before, and reviewers will have to re-review *everything*, wasting
> everybody's time even more. It will have to be re-reviewed in its entirety
> anyway because it has been *such* a long time since the latest review, and
> that's just the price we all have to pay for missing the right moment to
> advance this to `next`. Thomas, I will ask you to review, and Gábor, I
> will expect you to review that iteration, too, as you are now a bit
> familiar with the code, and I will really need your help here.
> 
> Anyway, that's my plan for now.

I must say I am not very happy about this plan.  The series has been
marked as "Will merge to 'next'" in previous iterations, but then we
found some issues that prevented that.  However I thought we were fine
fixing those on top at this point, rather than starting with a new
iteration again.

I was always under the impression that once the problem that was
discovered here was fixed we'd advance the series to 'next' with the
patch that comes out of this discussion on top.  Whether it's in next
shortly before 2.21 or not doesn't seem to make much of a difference
to me, as this wasn't going to make the 2.21 release anyway.  My hope
was that we could get it into 'next' shortly after 2.21 is released to
get the series some further exposure (which may well turn up some
other issues that we are not aware of yet, but such is the life of
software).

Sure there are some things that no reviewer spotted, but I'd think so
will there be next time and the time after that.  I don't believe that
code review can eliminate all issues, but I think we all did our best
to avoid a good chunk of them and unfortunately missed some in the
process.

Reviewing this series again in a slightly new form is not something I
am personally looking forward to.  I'd be more than happy to review
patches on top of this series, but after reading it many times over,
in slightly different versions, it gets harder and harder to review
and to find the motivation to review this.  Now if we really think
this series has to be completely overhauled, so be it, but I don't
think we would end up with a better end result than if we were to
continue to work on top of this.

> Ciao,
> Dscho