Thanks, that makes a lot of sense. ce_uptodate doesn't have too many
callers either, so modifying it and checking CE_FSMONITOR_VALID there
should not be hard to audit.



On Tue, Oct 29, 2019 at 5:21 PM Junio C Hamano [off-list ref] wrote:

Utsav Shah [off-list ref] writes:

quoted

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.