Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

From: Junio C Hamano <gitster@pobox.com>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: Grzegorz Rajchman <rayman17@gmail.com>, git@vger.kernel.org
Subject: Re: [BUG] git stash pop --quiet deletes files in git 2.24.0
Date: Sun, 10 Nov 2019 15:11:48 +0900	[thread overview]
Message-ID: <xmqqk188l0pn.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20191108165929.GB3115@cat> (Thomas Gummerer's message of "Fri, 8 Nov 2019 16:59:29 +0000")

Thomas Gummerer <t.gummerer@gmail.com> writes:

> On 11/08, Junio C Hamano wrote:
>> So, I do not think removing that discard_cache() alone solves the
>> breakage exposed by 34933d0eff.  Discarding and re-reading the
>> on-disk index there would restore correctness, but then you would
>> want to make sure that we are not wasting the overall cost for the
>> I/O and refreshing.
>> 
>> I think the safer immediate short-term fix is to revert the change
>> to the quiet codepath and let it only refresh the in-core index.
>
> Yup, this is certainly my bad, we shouldn't be writing the discarded
> index of course.  I don't think what we were doing here before was
> correct either though.  The only thing that would be called after this
> is 'do_stash_drop()', which only executes external commands.

Right.  Removing discard alone would not be a correct fix exactly
for that reason: the in-core index was stale wrt the on-disk index.

If the program later used in-core index for further processing
(which is not, and that is why the short-term solution of reverting
that hunk would work), we would have been operating on a wrong data.
So for the fix that keeps data we have in-core always up-to-date, we
should be re-reading from the on-disk index there after discard().

And in the longer term, it would likely be the right direction, as
the "git status" invocation on the non-quiet codepath would want to
become an in-core direct calls into wt-status machinery instead of
fork+exec eventually.

> From what you are saying above, and from my testing I think this
> refresh is actually unnecessary, and we could just remove it outright.

Perhaps.  But later it will bite us when somebody wants to rewrite
the "status at the end" part in C.

Besides, if the original was "update-index -q --refresh" in the
scripted Porcelain after an pop was attempted, it would have shown
the unmerged paths as "needs merge", wouldn't it?  For that, we need
to have something (I do not remember if refresh_and_write_cache()
would be the in-core API call to do so offhand).

Thanks.