threads / patch / 47526

v2, 6 partsMinor fsmonitor bugfixes, use with `git diff`

Subject: [PATCH v2 0/6] Minor fsmonitor bugfixes, use with `git diff`

## tl;dr

15 messages between Jan 3, 2018 and Jan 8, 2018. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Alex Vandiver· Jan 3, 2018, 03:04 UTC · lore
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
Alex Vandiver· Jan 3, 2018, 03:04 UTC · re: Alex Vandiver · lore

[PATCH 1/6] Fix comments to agree with argument name

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 dir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to dir.c +2 −2
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
Alex Vandiver· Jan 3, 2018, 03:04 UTC · re: Alex Vandiver · lore

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

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(-)
Show changes to 3 files +9 −1

builtin/add.c, diff-lib.c, diff.h

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
Johannes Schindelin· Jan 4, 2018, 22:46 UTC · re: Alex Vandiver · lore

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

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

Junio C Hamano· Jan 5, 2018, 22:22 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 21 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.
I do find the logic a bit convoluted with double negative.
Ben Peart· Jan 8, 2018, 20:58 UTC · re: Junio C Hamano · lore

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

On 1/5/2018 5:22 PM, Junio C Hamano wrote:
Show 26 quoted lines
> 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?

Alex Vandiver· Jan 3, 2018, 03:04 UTC · re: Alex Vandiver · lore

[PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh

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(-)
Show changes to t/t7519-status-fsmonitor.sh +0 −2
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
Alex Vandiver· Jan 3, 2018, 03:04 UTC · re: Alex Vandiver · lore

[PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid

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(-)
Show changes to 2 files +20 −15

fsmonitor.c, fsmonitor.h

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
Johannes Schindelin· Jan 4, 2018, 22:27 UTC · re: Alex Vandiver · lore

Re: [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid

Hi Alex,
On Tue, 2 Jan 2018, Alex Vandiver wrote:
Show 6 quoted lines
> 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

Ben Peart· Jan 8, 2018, 20:27 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/6] fsmonitor: Stop inline'ing mark_fsmonitor_valid / _invalid

On 1/4/2018 5:27 PM, Johannes Schindelin wrote:
Show 11 quoted lines
> 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.
Show 6 quoted lines
> 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
>
Alex Vandiver· Jan 3, 2018, 03:04 UTC · re: Alex Vandiver · lore

[PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise

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(-)
Show changes to t/helper/test-dump-fsmonitor.c +11 −3
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
Johannes Schindelin· Jan 4, 2018, 22:33 UTC · re: Alex Vandiver · lore

Re: [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise

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

Ben Peart· Jan 8, 2018, 20:33 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/6] fsmonitor: Make output of test-dump-fsmonitor more concise

On 1/4/2018 5:33 PM, Johannes Schindelin wrote:
Show 13 quoted lines
> 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?
Show changes to t/helper/test-dump-fsmonitor.c +4 −2
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
>
Alex Vandiver· Jan 3, 2018, 03:04 UTC · re: Alex Vandiver · lore

[PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later

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(-)
Show changes to 2 files +10 −3

config.c, t/helper/test-dump-fsmonitor.c

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
Johannes Schindelin· Jan 4, 2018, 23:03 UTC · re: Alex Vandiver · lore

Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later

Hi Alex,
On Tue, 2 Jan 2018, Alex Vandiver wrote:
Show 17 quoted lines
> 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?

Show 14 quoted lines
> 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

← back to recent threads