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

From: Junio C Hamano <gitster@pobox.com>
To: Utsav Shah <utsav@dropbox.com>
Cc: Kevin Willford <Kevin.Willford@microsoft.com>,
	Utsav Shah via GitGitGadget <gitgitgadget@gmail.com>,
	"git\@vger.kernel.org" <git@vger.kernel.org>,
	Utsav Shah <ukshah2@illinois.edu>
Subject: Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor
Date: Wed, 30 Oct 2019 09:21:27 +0900	[thread overview]
Message-ID: <xmqqh83r5bi0.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <CAPYzU3Mv9fHG_WhCOfsA8KGeegdUCoEzfDCt8-DQ+CEjs=V62Q@mail.gmail.com> (Utsav Shah's message of "Tue, 29 Oct 2019 16:50:54 -0700")

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.