Thomas Gummerer <t.gummerer@xxxxxxxxx> 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.
