Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2019, 00:21 UTC
Message-ID: <xmqqh83r5bi0.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPYzU3Mv9fHG_WhCOfsA8KGeegdUCoEzfDCt8-DQ+CEjs=V62Q@mail.gmail.com>

Utsav Shah <utsav@dropbox.com> writes:

Show 9 quoted lines

> Thanks for testing it out. The unpack_trees bugfix is especially useful.
>
> There's tons of places where we're using ce_uptodate(ce) that could be
> optimized by checking CE_FSMONITOR_VALID. One example is in
> run_diff_files in diff-lib.c
>
> Should we add a check for CE_FSMONITOR_VALID in all of them? Should we
> do that in this patch? Or should we take the time to refactor and
> flesh out bugs in unifying it with CE_UPTODATE?

If we rephrase the first question slightly, i.e. "should these places all be avoiding lstat() based check when fsmonitor says the path is up to date?", I would imagine the answer is absolutely yes.

I would further imagine that the implementation of the interface to external fsmonitor itself may have to distinguish "we know/have known this path is clean" vs "we just got told by fsmonitor that this path is clean", so losing FSMONITOR_VALID bit might not be an easy or clean conversion, in which case my earlier "can we perhaps lose it and have fsmonitor interfacing code to directly set UPTODATE bit?" would lead us in a wrong direction.

But ce_uptodate(ce) being the primary way for the callers that care about "is the path known to be up to date?", it is unsatisfying that all of them have to ask

	if (!ce_uptodate(ce) && !(ce->ce_flags & CE_FSMONITOR_VALID))
		... process ce that is not up-to-date ...

So I would say that the longer term goal should be to let them ask ce_uptodate(ce) and have that macro automatically take FSMONITOR bit into account (in other words, those who want to know if ce is fresh should not have to even know about what fsmonitor is).

Perhaps we can take a polished version of this "'reset --hard' can and should notice paths known-to-be-uptodate via fsmonitor" as an independent patch (to reduce the number of things we have to worry by one) for now? Taking this patch means we would now have one more place that checks both ce_uptodate() and FSMONITOR_VALID bit, but if we would be auditing all such places before we can decide what the best way to reach the goal of allowing them to just say ce_uptodate() without having to spell FSMONITOR_VALID, that probably is a cost worth paying.

Thanks for working on this topic.