Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 4, 2018, 22:46 UTC
Message-ID: <alpine.DEB.2.21.1.1801042335130.32@MININT-6BKU6QN.europe.corp.microsoft.com>
In-Reply-To: <121828fc14bc6f3096d16005feffb58bf68f070a.1514948078.git.alexmv@dropbox.com>

Hi Alex,

On Tue, 2 Jan 2018, Alex Vandiver wrote:

Show 13 quoted lines

> diff --git a/diff-lib.c b/diff-lib.c
> index 8104603a3..13ff00d81 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  
>  	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
>  
> +	if (!(option & DIFF_SKIP_FSMONITOR))
> +		refresh_fsmonitor(&the_index);
> +
>  	if (diff_unmerged_stage < 0)
>  		diff_unmerged_stage = 2;

I read over this hunk five times, and only now am I able to wrap my head around this: if we do *not* want to skip the fsmonitor data, we refresh the fsmonitor data in the index.

That feels a bit like an unneeded double negation. Speaking for myself, I would prefore `DIFF_IGNORE_FSMONITOR` instead, it would feel less like a double negation then. But I am not a native speaker, so I might be wrong.

> +               if (ce->ce_flags & CE_FSMONITOR_VALID && !(option & DIFF_SKIP_FSMONITOR))
> +                       continue;

Since we do expect this to be called without the DIFF_SKIP_FSMONITOR flag, I guess it makes sense to order it this way.

I still have troubles to understand why we ignore the fsmonitor data with `git add`, though... we want to add only modified files, right? I thought that the fsmonitor data could help performance exactly there (I am thinking of a certain insanely large code base where a developer might want to change only one or maybe 3 files out of an entire machine workshop of files, and with fsmonitor it should be a really fast operation because it should ignore all but those few files, right?)... Could you maybe try to help me understand that better?

Thanks, Johannes