git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 0/6] Minor fsmonitor bugfixes, use with `git diff`
@ 2018-01-03  3:04 Alex Vandiver
  2018-01-03  3:04 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2018-01-03  3:04 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

Changes in this reroll:
 - Instead of including dir.h from fsmonitor.h, stop inlining the
   functions that made that necessary.

 - test-dump-fsmonitor sets a config variable soas to be able to
   access the fsmonitor state (after it has been un-split if using the
   split index) from the index without modifying it.  I'm least sure
   of this change, but I'm short on ideas for a cleaner
   implementation.

 - test-dump-fsmonitor now outputs a more concise output, with a
   trailing newline, instead of just patching over the lack of
   trailing newline on the old format.


Somehow patch 6/6 didn't get included in Junio's
av/fsmonitor-updates.  That's really the most useful patch of the
series, and the original impetus for it, as it turns out. :)

Best,
 - Alex



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/6] Fix comments to agree with argument name
  2018-01-03  3:04 [PATCH v2 0/6] Minor fsmonitor bugfixes, use with `git diff` Alex Vandiver
@ 2018-01-03  3:04 ` Alex Vandiver
  2018-01-03  3:04   ` [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver
                     ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Alex Vandiver @ 2018-01-03  3:04 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 dir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 7c4b45e30..cf05b1da0 100644
--- a/dir.c
+++ b/dir.c
@@ -790,9 +790,9 @@ static int add_excludes_from_buffer(char *buf, size_t size,
  * an index if 'istate' is non-null), parse it and store the
  * exclude rules in "el".
  *
- * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
+ * If sha1_stat is not NULL, compute SHA-1 of the exclude file and fill
  * stat data from disk (only valid if add_excludes returns zero). If
- * ss_valid is non-zero, "ss" must contain good value as input.
+ * sha1_stat.valid is non-zero, sha1_stat must contain good value as input.
  */
 static int add_excludes(const char *fname, const char *base, int baselen,
 			struct exclude_list *el,
-- 
2.15.1.31.gddce0adfe


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid
  2018-01-03  3:04 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
@ 2018-01-03  3:04   ` Alex Vandiver
  2018-01-04 22:27     ` Johannes Schindelin
  2018-01-03  3:04   ` [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later Alex Vandiver
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2018-01-03  3:04 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

These were inline'd when they were first introduced, presumably as an
optimization for cases when they were called in tight loops.  This
complicates using these functions, as untracked_cache_invalidate_path
is defined in dir.h.

Leave the inline'ing up to the compiler's decision, for ease of use.

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 fsmonitor.c | 18 ++++++++++++++++++
 fsmonitor.h | 17 ++---------------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 0af7c4edb..df084235b 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -194,6 +194,24 @@ void refresh_fsmonitor(struct index_state *istate)
 	istate->fsmonitor_last_update = last_update;
 }
 
+void mark_fsmonitor_valid(struct cache_entry *ce)
+{
+	if (core_fsmonitor) {
+		ce->ce_flags |= CE_FSMONITOR_VALID;
+		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name);
+	}
+}
+
+void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)
+{
+	if (core_fsmonitor) {
+		ce->ce_flags &= ~CE_FSMONITOR_VALID;
+		untracked_cache_invalidate_path(istate, ce->name);
+		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
+	}
+}
+
+
 void add_fsmonitor(struct index_state *istate)
 {
 	int i;
diff --git a/fsmonitor.h b/fsmonitor.h
index cd3cc0ccf..6328745b2 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -46,13 +46,7 @@ extern void refresh_fsmonitor(struct index_state *istate);
  * called any time the cache entry has been updated to reflect the
  * current state of the file on disk.
  */
-static inline void mark_fsmonitor_valid(struct cache_entry *ce)
-{
-	if (core_fsmonitor) {
-		ce->ce_flags |= CE_FSMONITOR_VALID;
-		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name);
-	}
-}
+extern void mark_fsmonitor_valid(struct cache_entry *ce);
 
 /*
  * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate
@@ -61,13 +55,6 @@ static inline void mark_fsmonitor_valid(struct cache_entry *ce)
  * trigger an lstat() or invalidate the untracked cache for the
  * corresponding directory
  */
-static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)
-{
-	if (core_fsmonitor) {
-		ce->ce_flags &= ~CE_FSMONITOR_VALID;
-		untracked_cache_invalidate_path(istate, ce->name);
-		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
-	}
-}
+extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);
 
 #endif
-- 
2.15.1.31.gddce0adfe


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later
  2018-01-03  3:04 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
  2018-01-03  3:04   ` [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver
@ 2018-01-03  3:04   ` Alex Vandiver
  2018-01-04 23:03     ` Johannes Schindelin
  2018-01-03  3:04   ` [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise Alex Vandiver
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2018-01-03  3:04 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

dd9005a0a ("fsmonitor: delay updating state until after split index is
merged", 2017-10-27) began deferring the setting of the
CE_FSMONITOR_VALID flag until later, such that do_read_index() does
not perform those steps.  This means that t/helper/test-dump-fsmonitor
showed all bits as always unset.

Load the index using read_index_from(), which is aware of split
indexes and later fsmonitor ewah inflation, but ensure that we do not
add or remove it, by setting the value to "keep".

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 config.c                       | 9 +++++++--
 t/helper/test-dump-fsmonitor.c | 4 +++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index e617c2018..7c6ed888e 100644
--- a/config.c
+++ b/config.c
@@ -2174,8 +2174,13 @@ int git_config_get_fsmonitor(void)
 	if (core_fsmonitor && !*core_fsmonitor)
 		core_fsmonitor = NULL;
 
-	if (core_fsmonitor)
-		return 1;
+
+	if (core_fsmonitor) {
+		if (!strcasecmp(core_fsmonitor, "keep"))
+			return -1;
+		else
+			return 1;
+	}
 
 	return 0;
 }
diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index ad452707e..48c4bab0b 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -1,12 +1,14 @@
 #include "cache.h"
+#include "config.h"
 
 int cmd_main(int ac, const char **av)
 {
 	struct index_state *istate = &the_index;
 	int i;
 
+	git_config_push_parameter("core.fsmonitor=keep");
 	setup_git_directory();
-	if (do_read_index(istate, get_index_file(), 0) < 0)
+	if (read_index_from(istate, get_index_file()) < 0)
 		die("unable to read index file");
 	if (!istate->fsmonitor_last_update) {
 		printf("no fsmonitor\n");
-- 
2.15.1.31.gddce0adfe


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise
  2018-01-03  3:04 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
  2018-01-03  3:04   ` [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver
  2018-01-03  3:04   ` [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later Alex Vandiver
@ 2018-01-03  3:04   ` Alex Vandiver
  2018-01-04 22:33     ` Johannes Schindelin
  2018-01-03  3:04   ` [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh Alex Vandiver
  2018-01-03  3:04   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver
  4 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2018-01-03  3:04 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

Rather than display one very long line, summarize the contents of that
line.  The tests do not currently rely on any content except the first
line ("no fsmonitor" / "fsmonitor last update").

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 t/helper/test-dump-fsmonitor.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index 48c4bab0b..5d61b0d62 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -4,7 +4,8 @@
 int cmd_main(int ac, const char **av)
 {
 	struct index_state *istate = &the_index;
-	int i;
+	uint64_t now = getnanotime();
+	int i, valid = 0;
 
 	git_config_push_parameter("core.fsmonitor=keep");
 	setup_git_directory();
@@ -14,10 +15,17 @@ int cmd_main(int ac, const char **av)
 		printf("no fsmonitor\n");
 		return 0;
 	}
-	printf("fsmonitor last update %"PRIuMAX"\n", (uintmax_t)istate->fsmonitor_last_update);
+
+	printf("fsmonitor last update %"PRIuMAX", (%.2f seconds ago)\n",
+	       (uintmax_t)istate->fsmonitor_last_update,
+	       (now - istate->fsmonitor_last_update)/1.0e9);
 
 	for (i = 0; i < istate->cache_nr; i++)
-		printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-");
+		if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)
+			valid++;
+
+	printf("  valid: %d\n", valid);
+	printf("  invalid: %d\n", istate->cache_nr - valid);
 
 	return 0;
 }
-- 
2.15.1.31.gddce0adfe


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh
  2018-01-03  3:04 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
                     ` (2 preceding siblings ...)
  2018-01-03  3:04   ` [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise Alex Vandiver
@ 2018-01-03  3:04   ` Alex Vandiver
  2018-01-03  3:04   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver
  4 siblings, 0 replies; 16+ messages in thread
From: Alex Vandiver @ 2018-01-03  3:04 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

These were mistakenly left in when the test was introduced, in
1487372d3 ("fsmonitor: store fsmonitor bitmap before splitting index",
2017-11-09)

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 t/t7519-status-fsmonitor.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index eb2d13bbc..19b2a0a0f 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -307,9 +307,7 @@ test_expect_success 'splitting the index results in the same state' '
 	dirty_repo &&
 	git update-index --fsmonitor  &&
 	git ls-files -f >expect &&
-	test-dump-fsmonitor >&2 && echo &&
 	git update-index --fsmonitor --split-index &&
-	test-dump-fsmonitor >&2 && echo &&
 	git ls-files -f >actual &&
 	test_cmp expect actual
 '
-- 
2.15.1.31.gddce0adfe


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
  2018-01-03  3:04 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
                     ` (3 preceding siblings ...)
  2018-01-03  3:04   ` [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh Alex Vandiver
@ 2018-01-03  3:04   ` Alex Vandiver
  2018-01-04 22:46     ` Johannes Schindelin
  4 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2018-01-03  3:04 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

This makes use of the fsmonitor extension to skip lstat() calls on
files that fsmonitor judged as unmodified.  We skip use of the
fsmonitor extension when called by "add" because the format_callback
in such cases expects to be called even when the file is believed to
be "up to date" with the index.

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 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 = &data;
 	rev.diffopt.flags.override_submodule_config = 1;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
-	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED | DIFF_SKIP_FSMONITOR);
 	clear_pathspec(&rev.prune_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(&revs->diffopt, "i/", "w/");
 
+	if (!(option & DIFF_SKIP_FSMONITOR))
+		refresh_fsmonitor(&the_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 7cf276f07..5cf5866bd 100644
--- a/diff.h
+++ b/diff.h
@@ -392,6 +392,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.31.gddce0adfe


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid
  2018-01-03  3:04   ` [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver
@ 2018-01-04 22:27     ` Johannes Schindelin
  2018-01-08 20:27       ` Ben Peart
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2018-01-04 22:27 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Ben Peart

Hi Alex,

On Tue, 2 Jan 2018, Alex Vandiver wrote:

> These were inline'd when they were first introduced, presumably as an
> optimization for cases when they were called in tight loops.  This
> complicates using these functions, as untracked_cache_invalidate_path
> is defined in dir.h.
> 
> Leave the inline'ing up to the compiler's decision, for ease of use.

As a compromise, you could leave the rather simple mark_fsmonitor_valid()
as inlined function. It should be by far the more-called function, anyway.

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise
  2018-01-03  3:04   ` [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise Alex Vandiver
@ 2018-01-04 22:33     ` Johannes Schindelin
  2018-01-08 20:33       ` Ben Peart
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2018-01-04 22:33 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Ben Peart

Hi Alex,

On Tue, 2 Jan 2018, Alex Vandiver wrote:

> Rather than display one very long line, summarize the contents of that
> line.  The tests do not currently rely on any content except the first
> line ("no fsmonitor" / "fsmonitor last update").

The more interesting part would be the entries with outdated ("invalid")
information. I thought that this information was pretty useful for
debugging. Maybe we could still keep at least that part, or at least
trigger outputting it via a command-line flag?

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
  2018-01-03  3:04   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver
@ 2018-01-04 22:46     ` Johannes Schindelin
  2018-01-05 22:22       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2018-01-04 22:46 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Ben Peart

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(&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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later
  2018-01-03  3:04   ` [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later Alex Vandiver
@ 2018-01-04 23:03     ` Johannes Schindelin
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2018-01-04 23:03 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Ben Peart

Hi Alex,

On Tue, 2 Jan 2018, Alex Vandiver wrote:

> diff --git a/config.c b/config.c
> index e617c2018..7c6ed888e 100644
> --- a/config.c
> +++ b/config.c
> @@ -2174,8 +2174,13 @@ int git_config_get_fsmonitor(void)
>  	if (core_fsmonitor && !*core_fsmonitor)
>  		core_fsmonitor = NULL;
>  
> -	if (core_fsmonitor)
> -		return 1;
> +
> +	if (core_fsmonitor) {
> +		if (!strcasecmp(core_fsmonitor, "keep"))
> +			return -1;
> +		else
> +			return 1;
> +	}

It took me a while to reason about this:

- there is no existing code path that can return -1 from
  git_config_get_fsmonitor(),

- the callers in builtin/update-index.c (testing explicitly for 0 and 1)
  do not matter because they only trigger warnings.

- the remaining two callers are in fsmonitor.c:

  - tweak_fsmonitor() (which handles -1 specifically), and

  - inflate_fsmonitor_ewah(), which only tests whether
    git_config_get_fsmonitor() returned a non-zero value, but that test is
    inside a code block that is only triggered if the index has an
    fsmonitor_dirty array, meaning: it already had fsmonitor enabled.
    Therefore the test is legitimate.

This would take the next reader as much time, I would wager a bet. So
maybe you can include this information (or at least the information about
inflate_fsmonitor_ewah()) in the commit message?

> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
> index ad452707e..48c4bab0b 100644
> --- a/t/helper/test-dump-fsmonitor.c
> +++ b/t/helper/test-dump-fsmonitor.c
> @@ -1,12 +1,14 @@
>  #include "cache.h"
> +#include "config.h"
>  
>  int cmd_main(int ac, const char **av)
>  {
>  	struct index_state *istate = &the_index;
>  	int i;
>  
> +	git_config_push_parameter("core.fsmonitor=keep");

The alternative would be to use an environment variable. We already use
GIT_FSMONITOR_TEST.

However, I wonder why we need this. Do we really update the index anywhere
in the tests, then *toggle* the core.fsmonitor setting, and *then* call
test-dump-fsmonitor?

And if we do, can't we simply avoid it?

Ciao,
Johannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
  2018-01-04 22:46     ` Johannes Schindelin
@ 2018-01-05 22:22       ` Junio C Hamano
  2018-01-08 20:58         ` Ben Peart
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2018-01-05 22:22 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Alex Vandiver, git, Ben Peart

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid
  2018-01-04 22:27     ` Johannes Schindelin
@ 2018-01-08 20:27       ` Ben Peart
  0 siblings, 0 replies; 16+ messages in thread
From: Ben Peart @ 2018-01-08 20:27 UTC (permalink / raw)
  To: Johannes Schindelin, Alex Vandiver; +Cc: git



On 1/4/2018 5:27 PM, Johannes Schindelin wrote:
> Hi Alex,
> 
> On Tue, 2 Jan 2018, Alex Vandiver wrote:
> 
>> These were inline'd when they were first introduced, presumably as an
>> optimization for cases when they were called in tight loops.  This
>> complicates using these functions, as untracked_cache_invalidate_path
>> is defined in dir.h.
>>
>> Leave the inline'ing up to the compiler's decision, for ease of use.
> 

I'm fine with these not being inline.  I was attempting to minimize the 
performance impact of the fsmonitor code when it was not even turned on. 
  Inlineing these functions allowed it to be kept to a simple test but I 
suspect (especially with modern optimizing compilers) that the overhead 
of calling a function to do that test is negligible.

> As a compromise, you could leave the rather simple mark_fsmonitor_valid()
> as inlined function. It should be by far the more-called function, anyway.
> 
> Ciao,
> Johannes
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise
  2018-01-04 22:33     ` Johannes Schindelin
@ 2018-01-08 20:33       ` Ben Peart
  0 siblings, 0 replies; 16+ messages in thread
From: Ben Peart @ 2018-01-08 20:33 UTC (permalink / raw)
  To: Johannes Schindelin, Alex Vandiver; +Cc: git



On 1/4/2018 5:33 PM, Johannes Schindelin wrote:
> Hi Alex,
> 
> On Tue, 2 Jan 2018, Alex Vandiver wrote:
> 
>> Rather than display one very long line, summarize the contents of that
>> line.  The tests do not currently rely on any content except the first
>> line ("no fsmonitor" / "fsmonitor last update").
> 
> The more interesting part would be the entries with outdated ("invalid")
> information. I thought that this information was pretty useful for
> debugging. Maybe we could still keep at least that part, or at least
> trigger outputting it via a command-line flag?
> 

During the development and testing of fsmonitor, I found the '+-' to be 
helpful (especially since it is in index order).  I could touch a file 
and verify that it showed up as invalid and that it was the file I 
expected by its placement in the index.

I'd hate to have to add options to a test program for more/less output. 
I do like your additions of the time since updated and the final counts. 
  I prefer more information rather than less in my test tools - how 
about this?


diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index 5d61b0d621..8503da288d 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -20,11 +20,13 @@ int cmd_main(int ac, const char **av)
                (uintmax_t)istate->fsmonitor_last_update,
                (now - istate->fsmonitor_last_update)/1.0e9);

-       for (i = 0; i < istate->cache_nr; i++)
+       for (i = 0; i < istate->cache_nr; i++) {
+               printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) 
? "+" : "-");
                 if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID)
                         valid++;
+       }

-       printf("  valid: %d\n", valid);
+       printf("\n  valid: %d\n", valid);
         printf("  invalid: %d\n", istate->cache_nr - valid);

         return 0;


> Ciao,
> Johannes
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
  2018-01-05 22:22       ` Junio C Hamano
@ 2018-01-08 20:58         ` Ben Peart
  0 siblings, 0 replies; 16+ messages in thread
From: Ben Peart @ 2018-01-08 20:58 UTC (permalink / raw)
  To: Junio C Hamano, Johannes Schindelin; +Cc: Alex Vandiver, git



On 1/5/2018 5:22 PM, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`
  2017-12-19  0:28 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
@ 2017-12-19  0:28   ` Alex Vandiver
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Vandiver @ 2017-12-19  0:28 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

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 <alexmv@dropbox.com>
---
 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 = &data;
 	rev.diffopt.flags.override_submodule_config = 1;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
-	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED | DIFF_SKIP_FSMONITOR);
 	clear_pathspec(&rev.prune_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(&revs->diffopt, "i/", "w/");
 
+	if (!(option & DIFF_SKIP_FSMONITOR))
+		refresh_fsmonitor(&the_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


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-01-08 20:58 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-03  3:04 [PATCH v2 0/6] Minor fsmonitor bugfixes, use with `git diff` Alex Vandiver
2018-01-03  3:04 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
2018-01-03  3:04   ` [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid Alex Vandiver
2018-01-04 22:27     ` Johannes Schindelin
2018-01-08 20:27       ` Ben Peart
2018-01-03  3:04   ` [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later Alex Vandiver
2018-01-04 23:03     ` Johannes Schindelin
2018-01-03  3:04   ` [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise Alex Vandiver
2018-01-04 22:33     ` Johannes Schindelin
2018-01-08 20:33       ` Ben Peart
2018-01-03  3:04   ` [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh Alex Vandiver
2018-01-03  3:04   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver
2018-01-04 22:46     ` Johannes Schindelin
2018-01-05 22:22       ` Junio C Hamano
2018-01-08 20:58         ` Ben Peart
  -- strict thread matches above, loose matches on Subject: below --
2017-12-19  0:28 [PATCH 0/6] Minor fsmonitor bugfixes, use with " Alex Vandiver
2017-12-19  0:28 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
2017-12-19  0:28   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git