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

[Utsav Shah <utsav@dropbox.com>]

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 <gitster@pobox.com> wrote:
>
> Utsav Shah <utsav@dropbox.com> writes:
>
> > 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.
>
>
>