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

[Junio C Hamano <gitster@pobox.com>]

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

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

>  		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);