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

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

[Alex Vandiver]

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

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

[Alex Vandiver]

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

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

[Alex Vandiver]

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

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

[Johannes Schindelin]

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

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

[Ben Peart]

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
> 

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

[Alex Vandiver]

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

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

[Johannes Schindelin]

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

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

[Alex Vandiver]

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

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

[Johannes Schindelin]

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

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

[Ben Peart]

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
> 

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

[Alex Vandiver]

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

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

[Alex Vandiver]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Ben Peart]

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?

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

[Alex Vandiver]

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

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

[Alex Vandiver]

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

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

[Alex Vandiver]

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

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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