Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
On 1/5/2018 5:22 PM, Junio C Hamano wrote: Johannes Schindelinwrites: 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(>diffopt, "i/", "w/"); + if (!(option & DIFF_SKIP_FSMONITOR)) + refresh_fsmonitor(_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?
Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
Johannes Schindelinwrites: >> 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(>diffopt, "i/", "w/"); >> >> +if (!(option & DIFF_SKIP_FSMONITOR)) >> +refresh_fsmonitor(_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.
Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
Hi Alex, On Tue, 2 Jan 2018, Alex Vandiver wrote: > 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(>diffopt, "i/", "w/"); > > + if (!(option & DIFF_SKIP_FSMONITOR)) > + refresh_fsmonitor(_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
[PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
With fsmonitor enabled, the first call to match_stat_with_submodule calls refresh_fsmonitor, incurring the overhead of reading the list of updated files -- but run_diff_files does not respect the CE_FSMONITOR_VALID flag. Make use of the fsmonitor extension to skip lstat() calls on files that fsmonitor judged as unmodified. Skip use of the fsmonitor extension when called by "add", as the format_callback in such cases expects to be called even when the file is believed to be "up to date" with the index. Notably, this change improves performance of the git shell prompt when GIT_PS1_SHOWDIRTYSTATE is set. Signed-off-by: Alex Vandiver--- builtin/add.c | 2 +- diff-lib.c| 6 ++ diff.h| 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index bf01d89e2..bba20b46e 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -119,7 +119,7 @@ int add_files_to_cache(const char *prefix, rev.diffopt.format_callback_data = rev.diffopt.flags.override_submodule_config = 1; rev.max_count = 0; /* do not compare unmerged paths with stage #2 */ - run_diff_files(, DIFF_RACY_IS_MODIFIED); + run_diff_files(, DIFF_RACY_IS_MODIFIED | DIFF_SKIP_FSMONITOR); clear_pathspec(_data); return !!data.add_errors; } 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(>diffopt, "i/", "w/"); + if (!(option & DIFF_SKIP_FSMONITOR)) + refresh_fsmonitor(_index); + if (diff_unmerged_stage < 0) diff_unmerged_stage = 2; entries = active_nr; @@ -197,6 +200,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option) if (ce_uptodate(ce) || ce_skip_worktree(ce)) continue; + if (ce->ce_flags & CE_FSMONITOR_VALID && !(option & DIFF_SKIP_FSMONITOR)) + continue; + /* If CE_VALID is set, don't look at workdir for file removal */ if (ce->ce_flags & CE_VALID) { changed = 0; diff --git a/diff.h b/diff.h index 36a09624f..1060bc495 100644 --- a/diff.h +++ b/diff.h @@ -395,6 +395,8 @@ extern const char *diff_aligned_abbrev(const struct object_id *sha1, int); #define DIFF_SILENT_ON_REMOVED 01 /* report racily-clean paths as modified */ #define DIFF_RACY_IS_MODIFIED 02 +/* skip loading the fsmonitor data */ +#define DIFF_SKIP_FSMONITOR 04 extern int run_diff_files(struct rev_info *revs, unsigned int option); extern int run_diff_index(struct rev_info *revs, int cached); -- 2.15.1.626.gc4617b774