Re: [PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2021, 20:44 UTC
Message-ID: <xmqqo8fgry3g.fsf@gitster.g>
In-Reply-To: <75a3c46c405549d1f5127097729c556a7e297587.1616016143.git.gitgitgadget@gmail.com>

"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 21 quoted lines

> From: Nipunn Koorapati <nipunn@dropbox.com>
>
> Teach git to honor fsmonitor rather than issuing an lstat
> when checking for dirty local deletes. Eliminates O(files)
> lstats during `git diff HEAD`
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
>  diff-lib.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index b73cc1859a49..3fb538ad18e9 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -30,7 +30,7 @@
>   */
>  static int check_removed(const struct cache_entry *ce, struct stat *st)
>  {
> -	if (lstat(ce->name, st) < 0) {
> +	if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {

So when the cache entry is marked as VALID, we know it is there and unmodified without asking lstat(). Otherwise we ask lstat() as before. OK.

Show 16 quoted lines

>  		if (!is_missing_file_error(errno))
>  			return -1;
>  		return 1;
> @@ -574,6 +574,7 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
>  	struct object_id oid;
>  	const char *name;
>  	char merge_base_hex[GIT_MAX_HEXSZ + 1];
> +	struct index_state *istate = revs->diffopt.repo->index;
>  
>  	if (revs->pending.nr != 1)
>  		BUG("run_diff_index must be passed exactly one tree");
> @@ -581,6 +582,8 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
>  	trace_performance_enter();
>  	ent = revs->pending.objects;
>  
> +	refresh_fsmonitor(istate);

And the VALID bit is set only for the ones that are untouched? When core_fsmonitor is not set, or istate->fsmonitor_has_run_once is set, refresh_fsmonitor() becomes no-op and does not even drop the VALID bit from the cache entries. As run_diff_index() is rather library-ish part of the system, are we sure no earlier attempts to invoke fsmonitor have touched ce to set the VALID bit on at this point?

Assuming that we won't see stray VALID bit to confuse us, the patch looks good to me, but I am not sure what to base confidence on that assumption.

Thanks.

>  	if (merge_base) {
>  		diff_get_merge_base(revs, &oid);
>  		name = oid_to_hex_r(merge_base_hex, &oid);