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@xxxxxxxxx> wrote: > > Utsav Shah <utsav@xxxxxxxxxxx> 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. > > >
