archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <>
To: Utsav Shah <>
Cc: Kevin Willford <>,
	Utsav Shah via GitGitGadget <>,
	"git\" <>,
	Utsav Shah <>
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: <> (raw)
In-Reply-To: <> (Utsav Shah's message of "Tue, 29 Oct 2019 16:50:54 -0700")

Utsav Shah <> 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.

  reply	other threads:[~2019-10-30  0:21 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-25 15:23 [PATCH 0/1] unpack-trees: skip lstat on files based on fsmonitor Utsav Shah via GitGitGadget
2019-10-25 15:23 ` [PATCH 1/1] unpack-trees: skip lstat " Utsav Shah via GitGitGadget
2019-10-28  3:37   ` Junio C Hamano
2019-10-28  6:39     ` Utsav Shah
2019-10-28 19:23       ` Kevin Willford
2019-10-29 19:06         ` Utsav Shah
2019-10-29 20:12           ` Kevin Willford
2019-10-29 23:50             ` Utsav Shah
2019-10-30  0:21               ` Junio C Hamano [this message]
2019-10-30 16:41                 ` Utsav Shah
2019-11-04  6:02                   ` Junio C Hamano
2019-11-05 15:27 ` [PATCH v2 0/1] unpack-trees: skip stat on fsmonitor-valid files Utsav Shah via GitGitGadget
2019-11-05 15:27   ` [PATCH v2 1/1] " Utsav Shah via GitGitGadget
2019-11-05 21:40     ` Kevin Willford
2019-11-06  4:36       ` Utsav Shah
2019-11-06 17:24         ` Kevin Willford
2019-11-06  4:54   ` [PATCH v3 0/1] " Utsav Shah via GitGitGadget
2019-11-06  4:54     ` [PATCH v3 1/1] " Utsav Shah via GitGitGadget
2019-11-06 10:46       ` Junio C Hamano
2019-11-06 22:33         ` Utsav Shah
2019-11-08  3:51           ` Utsav Shah
2019-11-08  4:11             ` Junio C Hamano
2019-11-06 10:16     ` [PATCH v3 0/1] " Junio C Hamano
2019-11-20  8:32     ` [PATCH v4 " Utsav Shah via GitGitGadget
2019-11-20  8:32       ` [PATCH v4 1/1] " Utsav Shah via GitGitGadget
2019-11-21  4:15         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).