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

This cleans up some mistakes I introduced in my previous series, and
switches `git diff` to use the fsmonitor data.

I've noticed that `git checkout HEAD` drops the fsmonitor data, which
surprises me -- the following patch "fixes" that but broke tests, so
there's something I clearly don't understand yet going on here:

--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1262,6 +1262,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
        o->result.timestamp.nsec = o->src_index->timestamp.nsec;
        o->result.version = o->src_index->version;
        o->result.split_index = o->src_index->split_index;
+       o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;
        if (o->result.split_index)
                o->result.split_index->refcount++;
        hashcpy(o->result.sha1, o->src_index->sha1);


 - Alex


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

* [PATCH 1/6] Fix comments to agree with argument name
  2017-12-19  0:28 [PATCH 0/6] Minor fsmonitor bugfixes, use with `git diff` Alex Vandiver
@ 2017-12-19  0:28 ` Alex Vandiver
  2017-12-19  0:28   ` [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path Alex Vandiver
                     ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Alex Vandiver @ 2017-12-19  0:28 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.626.gc4617b774


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

* [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path
  2017-12-19  0:28 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
@ 2017-12-19  0:28   ` Alex Vandiver
  2017-12-19 20:17     ` Junio C Hamano
  2017-12-19  0:28   ` [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 @ 2017-12-19  0:28 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

This missing include is currently hidden by dint of the fact that
dir.h is already included by all things that currently include
fsmonitor.h

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 fsmonitor.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fsmonitor.h b/fsmonitor.h
index cd3cc0ccf..5f68ca4d2 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -1,5 +1,6 @@
 #ifndef FSMONITOR_H
 #define FSMONITOR_H
+#include "dir.h"
 
 extern struct trace_key trace_fsmonitor;
 
-- 
2.15.1.626.gc4617b774


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

* [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later
  2017-12-19  0:28 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
  2017-12-19  0:28   ` [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path Alex Vandiver
@ 2017-12-19  0:28   ` Alex Vandiver
  2017-12-20 21:12     ` Alex Vandiver
  2017-12-19  0:28   ` [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor Alex Vandiver
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2017-12-19  0:28 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.

Split out the code which inflates the ewah into CE_FSMONITOR_VALID
bits, and call that from t/helper/test-dump-fsmonitor.  We cannot
simply switch the code to call read_index_from or the more specific
tweak_fsmonitor, because the latter would modify the extension state
by calling add_fsmonitor.

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 fsmonitor.c                    | 9 ++++++++-
 fsmonitor.h                    | 6 ++++++
 t/helper/test-dump-fsmonitor.c | 2 ++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 0af7c4edb..7011dff15 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -227,7 +227,7 @@ void remove_fsmonitor(struct index_state *istate)
 	}
 }
 
-void tweak_fsmonitor(struct index_state *istate)
+void inflate_fsmonitor_ewah(struct index_state *istate)
 {
 	int i;
 	int fsmonitor_enabled = git_config_get_fsmonitor();
@@ -250,6 +250,13 @@ void tweak_fsmonitor(struct index_state *istate)
 		ewah_free(istate->fsmonitor_dirty);
 		istate->fsmonitor_dirty = NULL;
 	}
+}
+
+void tweak_fsmonitor(struct index_state *istate)
+{
+	int fsmonitor_enabled = git_config_get_fsmonitor();
+
+	inflate_fsmonitor_ewah(istate);
 
 	switch (fsmonitor_enabled) {
 	case -1: /* keep: do nothing */
diff --git a/fsmonitor.h b/fsmonitor.h
index 5f68ca4d2..619852d4b 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -28,6 +28,12 @@ extern void write_fsmonitor_extension(struct strbuf *sb, struct index_state *ist
 extern void add_fsmonitor(struct index_state *istate);
 extern void remove_fsmonitor(struct index_state *istate);
 
+/*
+ * Inflate the fsmonitor_dirty ewah into the CE_FSMONITOR_VALID bits.
+ * Called by tweak_fsmonitor.
+ */
+extern void inflate_fsmonitor_ewah(struct index_state *istate);
+
 /*
  * Add/remove the fsmonitor index extension as necessary based on the current
  * core.fsmonitor setting.
diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index ad452707e..53b19b39b 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "fsmonitor.h"
 
 int cmd_main(int ac, const char **av)
 {
@@ -8,6 +9,7 @@ int cmd_main(int ac, const char **av)
 	setup_git_directory();
 	if (do_read_index(istate, get_index_file(), 0) < 0)
 		die("unable to read index file");
+	inflate_fsmonitor_ewah(istate);
 	if (!istate->fsmonitor_last_update) {
 		printf("no fsmonitor\n");
 		return 0;
-- 
2.15.1.626.gc4617b774


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

* [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor
  2017-12-19  0:28 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
  2017-12-19  0:28   ` [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path Alex Vandiver
  2017-12-19  0:28   ` [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later Alex Vandiver
@ 2017-12-19  0:28   ` Alex Vandiver
  2017-12-19 20:28     ` Junio C Hamano
  2017-12-19  0:28   ` [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh Alex Vandiver
  2017-12-19  0:28   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver
  4 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2017-12-19  0:28 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Ben Peart

This makes it more readable when used for debugging from the
commandline.

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

diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index 53b19b39b..4e56929f7 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -19,5 +19,6 @@ int cmd_main(int ac, const char **av)
 	for (i = 0; i < istate->cache_nr; i++)
 		printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-");
 
+	printf("\n");
 	return 0;
 }
-- 
2.15.1.626.gc4617b774


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

* [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh
  2017-12-19  0:28 ` [PATCH 1/6] Fix comments to agree with argument name Alex Vandiver
                     ` (2 preceding siblings ...)
  2017-12-19  0:28   ` [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor Alex Vandiver
@ 2017-12-19  0:28   ` Alex Vandiver
  2017-12-19 20:19     ` Junio C Hamano
  2017-12-19  0:28   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver
  4 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2017-12-19  0:28 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.626.gc4617b774


^ permalink raw reply related	[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
                     ` (3 preceding siblings ...)
  2017-12-19  0:28   ` [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh Alex Vandiver
@ 2017-12-19  0:28   ` Alex Vandiver
  4 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 related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path
  2017-12-19  0:28   ` [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path Alex Vandiver
@ 2017-12-19 20:17     ` Junio C Hamano
  2017-12-20 20:59       ` Alex Vandiver
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-12-19 20:17 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Johannes Schindelin, Ben Peart

Alex Vandiver <alexmv@dropbox.com> writes:

> Subject: Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path

Perhaps

"Subject: fsmonitor.h: include dir.h"

But I am not sure if this is a right direction to go in.  If a .C
user of fsmonitor needs (does not need) things from dir.h, that file
can (does not need to) include dir.h itself.

I think this header does excessive "static inline" as premature
optimization, so a better "fix" to your perceived problem may be to
make them not "static inline".

> This missing include is currently hidden by dint of the fact that
> dir.h is already included by all things that currently include
> fsmonitor.h
>
> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
> ---


>  fsmonitor.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fsmonitor.h b/fsmonitor.h
> index cd3cc0ccf..5f68ca4d2 100644
> --- a/fsmonitor.h
> +++ b/fsmonitor.h
> @@ -1,5 +1,6 @@
>  #ifndef FSMONITOR_H
>  #define FSMONITOR_H
> +#include "dir.h"
>  
>  extern struct trace_key trace_fsmonitor;

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

* Re: [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh
  2017-12-19  0:28   ` [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh Alex Vandiver
@ 2017-12-19 20:19     ` Junio C Hamano
  2017-12-20 17:35       ` Johannes Schindelin
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-12-19 20:19 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Johannes Schindelin, Ben Peart

Alex Vandiver <alexmv@dropbox.com> writes:

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

Hmph, by default the standard output and standard error streams are
not shown in the test output, and it would help while debugging test
failures, so I am not sure if this is a good change.  With the
previous step [4/6], we can lose the "echo", of course, and I do not
think we need >&2 redirection there, either.

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

* Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor
  2017-12-19  0:28   ` [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor Alex Vandiver
@ 2017-12-19 20:28     ` Junio C Hamano
  2017-12-21  1:55       ` Alex Vandiver
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-12-19 20:28 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Johannes Schindelin, Ben Peart

Alex Vandiver <alexmv@dropbox.com> writes:

> Subject: Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor

"Subject: fsmonitor: complete the last line of test-dump-fsmonitor output"

perhaps.

> This makes it more readable when used for debugging from the
> commandline.
>
> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
> ---
>  t/helper/test-dump-fsmonitor.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
> index 53b19b39b..4e56929f7 100644
> --- a/t/helper/test-dump-fsmonitor.c
> +++ b/t/helper/test-dump-fsmonitor.c
> @@ -19,5 +19,6 @@ int cmd_main(int ac, const char **av)
>  	for (i = 0; i < istate->cache_nr; i++)
>  		printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-");
>  
> +	printf("\n");

That (and existing) uses of printf() all feel a bit overkill ;-)
Perhaps putchar() would suffice.

I am not sure if the above wants to become something like

	for (i = 0; i < istate->cache_nr; i++) {
        	putchar(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID ? '+' : '-');
		quote_c_style(istate->cache[i]->name, NULL, stdout, 0);
		putchar('\n');
	}

instead of "a single long incomplete line" in the first place.  Your
"fix" merely turns it into "a single long complete line", which does
not quite feel big enough an improvement, at least to me.

>  	return 0;
>  }

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

* Re: [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh
  2017-12-19 20:19     ` Junio C Hamano
@ 2017-12-20 17:35       ` Johannes Schindelin
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2017-12-20 17:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Alex Vandiver, git, Ben Peart

Hi Junio,

On Tue, 19 Dec 2017, Junio C Hamano wrote:

> Alex Vandiver <alexmv@dropbox.com> writes:
> 
> > 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
> >  '
> 
> Hmph, by default the standard output and standard error streams are
> not shown in the test output, and it would help while debugging test
> failures, so I am not sure if this is a good change.  With the
> previous step [4/6], we can lose the "echo", of course, and I do not
> think we need >&2 redirection there, either.

I think you got it backwards. Sure, this helps debugging, but it hurts
runtime of the entire test suite (which I might have happened to mention a
couple of times takes way too long on Windows, thanks to our choice of
test "framework").

And in the bigger picture, I think that it is very, very easy to insert
those debugging statements when something breaks (we have to do that with
other tests, anyways).

So I am in favor of this patch, and disagree with your assessment, Junio.

Ciao,
Dscho

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

* Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path
  2017-12-19 20:17     ` Junio C Hamano
@ 2017-12-20 20:59       ` Alex Vandiver
  2017-12-21 21:47         ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2017-12-20 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin, Ben Peart

On Tue, 19 Dec 2017, Junio C Hamano wrote:
> Alex Vandiver <alexmv@dropbox.com> writes:
> 
> > Subject: Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path
> 
> Perhaps
> 
> "Subject: fsmonitor.h: include dir.h"

Certainly more concise.

> But I am not sure if this is a right direction to go in.  If a .C
> user of fsmonitor needs (does not need) things from dir.h, that file
> can (does not need to) include dir.h itself.

Hm; I was patterning based on existing .h files, which don't seem shy
about pulling in other .h files.

> I think this header does excessive "static inline" as premature
> optimization, so a better "fix" to your perceived problem may be to
> make them not "static inline".

Yeah, quite possibly.  Ben, do you recall your rationale for inlining
them in 6a6da08f6 ("fsmonitor: teach git to optionally utilize a file
system monitor to speed up detecting new or changed files.",
2017-09-22) ?

 - Alex

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

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

On Mon, 18 Dec 2017, Alex Vandiver wrote:
> 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.

This isn't the right fix, actually.  With split indexes, this puts us
right back into "only shows a few bits" territory, because
do_read_index doesn't know about split indexes.

Which means we need a way to do the whole index load but _not_
add/remove the fsmonitor cache, even if the config says to do so.

The best I'm coming up with is the below -- but I'm not happy with
it, because 'keep' is meaningless as a configuration value outside of
testing, since it's normally treated as an executable path.  This uses
the fact that fsmonitor.c currently has a:

        switch (fsmonitor_enabled) {
        case -1: /* keep: do nothing */
                break;

...despite get_config_get_fsmonitor() havong no way to return -1 at
present.

Is this sort of testing generally done via environment variables,
rather than magic config values?
 - Alex

---------------------8<----------------
diff --git a/config.c b/config.c
index 6fb06c213..75fcf1a52 100644
--- a/config.c
+++ b/config.c
@@ -2164,8 +2164,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..12e131530 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -5,8 +5,9 @@ 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");
-----------------8<---------------------

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

* Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor
  2017-12-19 20:28     ` Junio C Hamano
@ 2017-12-21  1:55       ` Alex Vandiver
  2017-12-21 21:49         ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Vandiver @ 2017-12-21  1:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin, Ben Peart


On Tue, 19 Dec 2017, Junio C Hamano wrote:
> That (and existing) uses of printf() all feel a bit overkill ;-)
> Perhaps putchar() would suffice.
> 
> I am not sure if the above wants to become something like
> 
> 	for (i = 0; i < istate->cache_nr; i++) {
>         	putchar(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID ? '+' : '-');
> 		quote_c_style(istate->cache[i]->name, NULL, stdout, 0);
> 		putchar('\n');
> 	}
> 
> instead of "a single long incomplete line" in the first place.  Your
> "fix" merely turns it into "a single long complete line", which does
> not quite feel big enough an improvement, at least to me.

The more user-digestable form like you describe already exists by way
of `git ls-files -f`.  I am not sure it is worth replicating it.

The only current uses of this tool are in tests, which only examine
the first ("no fsmonitor" / "fsmonitor last update ...") line.  I find
it useful as a brief summary view of the fsmonitor bits, but I suppose
I'd also be happy with just presence/absence and a count of set/unset
bits.

Barring objections from Dscho or Ben, I'll reroll with a version that
shows something like:

    fsmonitor last update 1513821151547101894 (5 seconds ago)
    5 files valid / 10 files invalid

 - Alex

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

* Re: [PATCH 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path
  2017-12-20 20:59       ` Alex Vandiver
@ 2017-12-21 21:47         ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-12-21 21:47 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Johannes Schindelin, Ben Peart

Alex Vandiver <alexmv@dropbox.com> writes:

>> But I am not sure if this is a right direction to go in.  If a .C
>> user of fsmonitor needs (does not need) things from dir.h, that file
>> can (does not need to) include dir.h itself.
>
> Hm; I was patterning based on existing .h files, which don't seem shy
> about pulling in other .h files.

IIUC, existing X.h do pull in Y.h when X.h uses a structure or a
typedef defined in Y.h (but using pointer to such a structure or a
type does not count) defined in Y.h; in such a case, a user of X.h
that wants to use what is defined in X.h would not be able to use
it without somehow knowing the shape of such a structure or type and
would be forced to pull in Y.h itself.  "static inline" falls into
the same category---as it stands, anybody that includes fsmonitor.h
and wants to use one of these static inline functions would need to
have definitions from dir.h, which I agree is wrong and I understand
that you want to include dir.h there.


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

* Re: [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor
  2017-12-21  1:55       ` Alex Vandiver
@ 2017-12-21 21:49         ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-12-21 21:49 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git, Johannes Schindelin, Ben Peart

Alex Vandiver <alexmv@dropbox.com> writes:

> The only current uses of this tool are in tests, which only examine
> the first ("no fsmonitor" / "fsmonitor last update ...") line.  I find
> it useful as a brief summary view of the fsmonitor bits, but I suppose
> I'd also be happy with just presence/absence and a count of set/unset
> bits.
>
> Barring objections from Dscho or Ben, I'll reroll with a version that
> shows something like:
>
>     fsmonitor last update 1513821151547101894 (5 seconds ago)
>     5 files valid / 10 files invalid

As I agree that this is test/debug only, I do not care too deeply
either way, as long as it does not end with an incomplete line ;-)

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

end of thread, other threads:[~2017-12-21 21:50 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-19  0:28 [PATCH 0/6] Minor fsmonitor bugfixes, use with `git diff` 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 2/6] fsmonitor: Add dir.h include, for untracked_cache_invalidate_path Alex Vandiver
2017-12-19 20:17     ` Junio C Hamano
2017-12-20 20:59       ` Alex Vandiver
2017-12-21 21:47         ` Junio C Hamano
2017-12-19  0:28   ` [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later Alex Vandiver
2017-12-20 21:12     ` Alex Vandiver
2017-12-19  0:28   ` [PATCH 4/6] fsmonitor: Add a trailing newline to test-dump-fsmonitor Alex Vandiver
2017-12-19 20:28     ` Junio C Hamano
2017-12-21  1:55       ` Alex Vandiver
2017-12-21 21:49         ` Junio C Hamano
2017-12-19  0:28   ` [PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh Alex Vandiver
2017-12-19 20:19     ` Junio C Hamano
2017-12-20 17:35       ` Johannes Schindelin
2017-12-19  0:28   ` [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff` Alex Vandiver

Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).