From: Ben Peart Date: Mon, 08 Jan 2018 20:58:17 GMT Subject: Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Message-ID: <6f7b31f7-3532-a7b4-5846-d2994dcb8795@gmail.com> In-Reply-To: On 1/5/2018 5:22 PM, Junio C Hamano wrote: > Johannes Schindelin writes: > >>> 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. > > I do find the logic a bit convoluted with double negative. > It's great to see more use of the fsmonitor data. Thanks for doing this! I agree with the sentiment that the logic as written is confusing. I'll also point out that DIFF_IGNORE_FSMONITOR would be more consistent with the similar CE_MATCH_IGNORE_FSMONITOR flag and logic. I'm also confused why we would not want to use the fsmonitor data in the 'add' case. When would you ever need to add a file that had not been modified?