Fix fsmonitor after discard_index()

It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/[email protected]/T/#mb8718fe52e4721dacd3b143a09187ff9090ef4e3]
that there were problems with the fsmonitor feature in conjunction with the newly built-in git stash/git rebase.

The culprit really is that the fsmonitor flag that says whether it was queried already was not re-set after discard_index() was called by mistake.

This fixes that, and apparently also other long-standing fsmonitor issues.

(Note that there is still a flakiness around t7519
[https://github.com/git-for-windows/git/pull/2127#pullrequestreview-215010574]
where it tries to make sure that the fsmonitor hook can prevent unnecessary lstat() calls, but that seems to be unrelated to this here bug.)

Johannes Schindelin (2):
fsmonitor: demonstrate that it is not refreshed after discard_index()
fsmonitor: force a refresh after the index was discarded

 cache.h                     |  3 ++-
 fsmonitor.c                 |  5 ++---
 read-cache.c                |  1 +
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh | 10 ++++++++++
 5 files changed, 38 insertions(+), 5 deletions(-)

base-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-165%2Fdscho%2Ffix-fsmonitor-after-discard-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-165/dscho/fix-fsmonitor-after-discard-index-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/165

gitgitgadget

From: Johannes Schindelin <[email protected]>

This one is tricky.

When core.fsmonitor is set, a refresh_index() will not perform a full scan of files that might be modified, but will query the fsmonitor and refresh only the ones that have been actually touched.

Due to implementation details, the fsmonitor is queried in refresh_cache_ent(), but of course it only has to be queried once, so we set a flag when we did that. But when the index was discarded, we did not re-set that flag.

So far, this is only covered by our test suite when running with GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all, and only due to the way the built-in stash interacts with the recursive merge machinery.

Let's introduce a straight-forward regression test for this.

We simply extend the "read & discard index" loop in test-tool read-cache to optionally refresh the index, report on a given file's status, and then modify that file. Due to the bug described above, only the first refresh will actually query the fsmonitor; subsequent loop iterations will not.

This problem was reported by Ævar Arnfjörð Bjarmason.

---
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh | 10 ++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

index d674c88ba0..7e79b555de 100644
@@ -1,14 +1,36 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "config.h"
 
 int cmd__read_cache(int argc, const char **argv)
 {
-	int i, cnt = 1;
+	int i, cnt = 1, namelen;
+	const char *name = NULL;
+
+	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
+		namelen = strlen(name);
+		argc--;
+		argv++;
+	}
+
 	if (argc == 2)
 		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
+	git_config(git_default_config, NULL);
 	for (i = 0; i < cnt; i++) {
 		read_cache();
+		if (name) {
+			int pos;
+
+			refresh_index(&the_index, REFRESH_QUIET,
+				      NULL, NULL, NULL);
+			pos = index_name_pos(&the_index, name, namelen);
+			if (pos < 0)
+				die("%s not in index", name);
+			printf("%s is%s up to date\n", name,
+			       ce_uptodate(the_index.cache[pos]) ? "" : " not");
+			write_file(name, "%d\n", i);
+		}
 		discard_cache();
 	}
 	return 0;

index 3e0a61db23..918bc323ab 100755
@@ -346,4 +346,14 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 	test_cmp before after
 '
 
+test_expect_failure 'discard_index() also discards fsmonitor info' '
+	test_when_finished \
+		"git config core.monitor .git/hooks/fsmonitor-test" &&
+	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
+	test_might_fail git update-index --refresh &&
+	test-tool read-cache --print-and-refresh=tracked 2 >actual &&
+	printf "tracked is%s up to date\n" "" " not" >expect &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

With this change, the index_state struct becomes the new home for the flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.

It also gets re-set when the index is discarded, fixing the bug where fsmonitor-enabled Git would miss updates under certain circumstances.

---
 cache.h                     | 3 ++-
 fsmonitor.c                 | 5 ++---
 read-cache.c                | 1 +
 t/t7519-status-fsmonitor.sh | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

index abd518a9a2..8d07c29c2a 100644
@@ -339,7 +339,8 @@ struct index_state {
 	struct cache_time timestamp;
 	unsigned name_hash_initialized : 1,
 		 initialized : 1,
-		 drop_cache_tree : 1;
+		 drop_cache_tree : 1,
+		 fsmonitor_has_run_once : 1;
 	struct hashmap name_hash;
 	struct hashmap dir_hash;
 	struct object_id oid;

index 665bd2d425..1dee0aded1 100644
@@ -129,7 +129,6 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n
 
 void refresh_fsmonitor(struct index_state *istate)
 {
-	static int has_run_once = 0;
 	struct strbuf query_result = STRBUF_INIT;
 	int query_success = 0;
 	size_t bol; /* beginning of line */

@@ -137,9 +136,9 @@ void refresh_fsmonitor(struct index_state *istate)
 	char *buf;
 	int i;
 
-	if (!core_fsmonitor || has_run_once)
+	if (!core_fsmonitor || istate->fsmonitor_has_run_once)
 		return;
-	has_run_once = 1;
+	istate->fsmonitor_has_run_once = 1;
 
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
 	/*

index 4dc6de1b55..0bf39e177a 100644
@@ -2335,6 +2335,7 @@ int discard_index(struct index_state *istate)
 	free_name_hash(istate);
 	cache_tree_free(&(istate->cache_tree));
 	istate->initialized = 0;
+	istate->fsmonitor_has_run_once = 0;
 	FREE_AND_NULL(istate->cache);
 	istate->cache_alloc = 0;
 	discard_split_index(istate);

index 918bc323ab..72b9ed3e45 100755
@@ -346,7 +346,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 	test_cmp before after
 '
 
-test_expect_failure 'discard_index() also discards fsmonitor info' '
+test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_when_finished \
 		"git config core.monitor .git/hooks/fsmonitor-test" &&
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&

gitgitgadget

Am 16.03.19 um 10:49 schrieb Johannes Schindelin via GitGitGadget:

I've added this to my internal build & now the test suite passes in the fsmonitor mode without any test skipping.

FWIW Since February 1st, 2018 I've run my builds on CentOS [67] through an GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all test and have never encountered this flakyness, and I built pretty much on every "next" push-out.

The fix sounds good, just one data point on the rarity of the race in practice. I hadn't noticed this being flaky.

Hi Hannes,

D'oh, yes.

It will unset it, as you point out, not *re-set* it to the value it had before.

But I guess that's par for the course. I am just worried about side effects in our test suite. I run into those all the time, it is not even
funny: when a prereq is not met, or when you just want to --run=<this-one>, you can't, because it is almost as if more than half of our test cases depend on the output of *some* previous test case.

But you're right, I should not worry so much. After all, I am adding a test case to the *end* of the test script.

Will send out a fixed version soon.

Thanks,
Dscho

It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/[email protected]/T/#mb8718fe52e4721dacd3b143a09187ff9090ef4e3]
that there were problems with the fsmonitor feature in conjunction with the newly built-in git stash/git rebase.

The culprit really is that the fsmonitor flag that says whether it was queried already was not re-set after discard_index() was called by mistake.

This fixes that, and apparently also other long-standing fsmonitor issues.

(Note that there is still a flakiness around t7519
[https://github.com/git-for-windows/git/pull/2127#pullrequestreview-215010574]
where it tries to make sure that the fsmonitor hook can prevent unnecessary lstat() calls, but that seems to be unrelated to this here bug.)

Changes since v1:

* Removed unnecessary test_when_finished "test_config ..." line, pointed out by Hannes Sixt.

Johannes Schindelin (2):
fsmonitor: demonstrate that it is not refreshed after discard_index()
fsmonitor: force a refresh after the index was discarded

 cache.h                     |  3 ++-
 fsmonitor.c                 |  5 ++---
 read-cache.c                |  1 +
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh |  8 ++++++++
 5 files changed, 36 insertions(+), 5 deletions(-)

base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-165%2Fdscho%2Ffix-fsmonitor-after-discard-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-165/dscho/fix-fsmonitor-after-discard-index-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/165

Range-diff vs v1:

1: f451752656 ! 1: 51a7edf22a fsmonitor: demonstrate that it is not refreshed after discard_index()

     @@ -79,8 +79,6 @@
       '
       
      +test_expect_failure 'discard_index() also discards fsmonitor info' '
     -+	test_when_finished \
     -+		"git config core.monitor .git/hooks/fsmonitor-test" &&
      +	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
      +	test_might_fail git update-index --refresh &&
      +	test-tool read-cache --print-and-refresh=tracked 2 >actual &&

2: 1d62623776 ! 2: 79fdd0d586 fsmonitor: force a refresh after the index was discarded
@@ -70,6 +70,6 @@

-test_expect_failure 'discard_index() also discards fsmonitor info' ' +test_expect_success 'discard_index() also discards fsmonitor info' '
- test_when_finished \
- "git config core.monitor .git/hooks/fsmonitor-test" &&
test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
+ test_might_fail git update-index --refresh &&
+ test-tool read-cache --print-and-refresh=tracked 2 >actual &&

gitgitgadget

From: Johannes Schindelin <[email protected]>

This one is tricky.

When core.fsmonitor is set, a refresh_index() will not perform a full scan of files that might be modified, but will query the fsmonitor and refresh only the ones that have been actually touched.

Due to implementation details, the fsmonitor is queried in refresh_cache_ent(), but of course it only has to be queried once, so we set a flag when we did that. But when the index was discarded, we did not re-set that flag.

So far, this is only covered by our test suite when running with GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all, and only due to the way the built-in stash interacts with the recursive merge machinery.

Let's introduce a straight-forward regression test for this.

We simply extend the "read & discard index" loop in test-tool read-cache to optionally refresh the index, report on a given file's status, and then modify that file. Due to the bug described above, only the first refresh will actually query the fsmonitor; subsequent loop iterations will not.

This problem was reported by Ævar Arnfjörð Bjarmason.

---
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh |  8 ++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

index d674c88ba0..7e79b555de 100644
@@ -1,14 +1,36 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "config.h"
 
 int cmd__read_cache(int argc, const char **argv)
 {
-	int i, cnt = 1;
+	int i, cnt = 1, namelen;
+	const char *name = NULL;
+
+	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
+		namelen = strlen(name);
+		argc--;
+		argv++;
+	}
+
 	if (argc == 2)
 		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
+	git_config(git_default_config, NULL);
 	for (i = 0; i < cnt; i++) {
 		read_cache();
+		if (name) {
+			int pos;
+
+			refresh_index(&the_index, REFRESH_QUIET,
+				      NULL, NULL, NULL);
+			pos = index_name_pos(&the_index, name, namelen);
+			if (pos < 0)
+				die("%s not in index", name);
+			printf("%s is%s up to date\n", name,
+			       ce_uptodate(the_index.cache[pos]) ? "" : " not");
+			write_file(name, "%d\n", i);
+		}
 		discard_cache();
 	}
 	return 0;

index 3e0a61db23..afd8fa7532 100755
@@ -346,4 +346,12 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 	test_cmp before after
 '
 
+test_expect_failure 'discard_index() also discards fsmonitor info' '
+	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
+	test_might_fail git update-index --refresh &&
+	test-tool read-cache --print-and-refresh=tracked 2 >actual &&
+	printf "tracked is%s up to date\n" "" " not" >expect &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

With this change, the index_state struct becomes the new home for the flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.

It also gets re-set when the index is discarded, fixing the bug where fsmonitor-enabled Git would miss updates under certain circumstances.

---
 cache.h                     | 3 ++-
 fsmonitor.c                 | 5 ++---
 read-cache.c                | 1 +
 t/t7519-status-fsmonitor.sh | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

index ac92421f3a..7434740c99 100644
@@ -339,7 +339,8 @@ struct index_state {
 	struct cache_time timestamp;
 	unsigned name_hash_initialized : 1,
 		 initialized : 1,
-		 drop_cache_tree : 1;
+		 drop_cache_tree : 1,
+		 fsmonitor_has_run_once : 1;
 	struct hashmap name_hash;
 	struct hashmap dir_hash;
 	struct object_id oid;

index 665bd2d425..1dee0aded1 100644
@@ -129,7 +129,6 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n
 
 void refresh_fsmonitor(struct index_state *istate)
 {
-	static int has_run_once = 0;
 	struct strbuf query_result = STRBUF_INIT;
 	int query_success = 0;
 	size_t bol; /* beginning of line */

@@ -137,9 +136,9 @@ void refresh_fsmonitor(struct index_state *istate)
 	char *buf;
 	int i;
 
-	if (!core_fsmonitor || has_run_once)
+	if (!core_fsmonitor || istate->fsmonitor_has_run_once)
 		return;
-	has_run_once = 1;
+	istate->fsmonitor_has_run_once = 1;
 
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
 	/*

index 4dc6de1b55..0bf39e177a 100644
@@ -2335,6 +2335,7 @@ int discard_index(struct index_state *istate)
 	free_name_hash(istate);
 	cache_tree_free(&(istate->cache_tree));
 	istate->initialized = 0;
+	istate->fsmonitor_has_run_once = 0;
 	FREE_AND_NULL(istate->cache);
 	istate->cache_alloc = 0;
 	discard_split_index(istate);

index afd8fa7532..81a375fa0f 100755
@@ -346,7 +346,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 	test_cmp before after
 '
 
-test_expect_failure 'discard_index() also discards fsmonitor info' '
+test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
 	test_might_fail git update-index --refresh &&
 	test-tool read-cache --print-and-refresh=tracked 2 >actual &&

gitgitgadget

Hi Ævar,

Awesome.

That makes me think that my bug fix may bring this racy problem to the light of day. After all, now we will query the fsmonitor a lot more times than before, and in quicker succession (because there are no slow shell commands running in between fsmonitor calls, i.e. the test suite).

I guess I will see more of that now, and maybe eventually the pain in my notification mails will be high enough that I'll allocate time to investigate.

Thank you for your feedback, though!
Dscho

Under what circumstances would a fsmonitor-enabled Git miss updates?

This v3 is all Johannes's patches. The outstanding review on v2 could be clarified with a commit message change, which I've addressed, and v2 conflicted with a cache.h change that's since landed in "master", which I've rebased this on.

Junio: We're getting closer to the release so it would be great to have this. It's been broken for a long time, but having this finaly fixed in v2.22 would be great. The functional code changes are also isolated to the fsmonitor code path, which reduces the risk and makes this easier to review/reason about.

Johannes Schindelin (2):
fsmonitor: demonstrate that it is not refreshed after discard_index()
fsmonitor: force a refresh after the index was discarded

 cache.h                     |  3 ++-
 fsmonitor.c                 |  5 ++---
 read-cache.c                |  1 +
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh |  8 ++++++++
 5 files changed, 36 insertions(+), 5 deletions(-)

Range-diff:

1:  51a7edf22a = 1:  c31f834b07 fsmonitor: demonstrate that it is not refreshed after discard_index()
2:  79fdd0d586 ! 2:  7bf5f9f610 fsmonitor: force a refresh after the index was discarded

@@ -6,8 +6,10 @@
flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.

- It also gets re-set when the index is discarded, fixing the bug where
- fsmonitor-enabled Git would miss updates under certain circumstances.
+ It also gets re-set when the index is discarded, fixing the bug
+ demonstrated by the "test_expect_failure" test added in the preceding
+ commit. In that case fsmonitor-enabled Git would miss updates under
+ certain circumstances, see that preceding commit for details.

@@ -15,11 +17,11 @@
--- a/cache.h
+++ b/cache.h
@@
- struct cache_time timestamp;
- unsigned name_hash_initialized : 1,
initialized : 1,
-- drop_cache_tree : 1;
-+ drop_cache_tree : 1,
+ drop_cache_tree : 1,
+ updated_workdir : 1,
+- updated_skipworktree : 1;
++ updated_skipworktree : 1,
+ fsmonitor_has_run_once : 1;
struct hashmap name_hash;
struct hashmap dir_hash;

2.21.0.593.g511ec345e18

From: Johannes Schindelin <[email protected]>

This one is tricky.

When core.fsmonitor is set, a refresh_index() will not perform a full scan of files that might be modified, but will query the fsmonitor and refresh only the ones that have been actually touched.

Due to implementation details, the fsmonitor is queried in refresh_cache_ent(), but of course it only has to be queried once, so we set a flag when we did that. But when the index was discarded, we did not re-set that flag.

So far, this is only covered by our test suite when running with GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all, and only due to the way the built-in stash interacts with the recursive merge machinery.

Let's introduce a straight-forward regression test for this.

We simply extend the "read & discard index" loop in test-tool read-cache to optionally refresh the index, report on a given file's status, and then modify that file. Due to the bug described above, only the first refresh will actually query the fsmonitor; subsequent loop iterations will not.

This problem was reported by Ævar Arnfjörð Bjarmason.

---
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh |  8 ++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

index d674c88ba0..7e79b555de 100644
@@ -1,14 +1,36 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "config.h"
 
 int cmd__read_cache(int argc, const char **argv)
 {
-	int i, cnt = 1;
+	int i, cnt = 1, namelen;
+	const char *name = NULL;
+
+	if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
+		namelen = strlen(name);
+		argc--;
+		argv++;
+	}
+
 	if (argc == 2)
 		cnt = strtol(argv[1], NULL, 0);
 	setup_git_directory();
+	git_config(git_default_config, NULL);
 	for (i = 0; i < cnt; i++) {
 		read_cache();
+		if (name) {
+			int pos;
+
+			refresh_index(&the_index, REFRESH_QUIET,
+				      NULL, NULL, NULL);
+			pos = index_name_pos(&the_index, name, namelen);
+			if (pos < 0)
+				die("%s not in index", name);
+			printf("%s is%s up to date\n", name,
+			       ce_uptodate(the_index.cache[pos]) ? "" : " not");
+			write_file(name, "%d\n", i);
+		}
 		discard_cache();
 	}
 	return 0;

index 3e0a61db23..afd8fa7532 100755
@@ -346,4 +346,12 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 	test_cmp before after
 '
 
+test_expect_failure 'discard_index() also discards fsmonitor info' '
+	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
+	test_might_fail git update-index --refresh &&
+	test-tool read-cache --print-and-refresh=tracked 2 >actual &&
+	printf "tracked is%s up to date\n" "" " not" >expect &&
+	test_cmp expect actual
+'
+
 test_done

2.21.0.593.g511ec345e18

From: Johannes Schindelin <[email protected]>

With this change, the index_state struct becomes the new home for the flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.

It also gets re-set when the index is discarded, fixing the bug demonstrated by the "test_expect_failure" test added in the preceding commit. In that case fsmonitor-enabled Git would miss updates under certain circumstances, see that preceding commit for details.

---
 cache.h                     | 3 ++-
 fsmonitor.c                 | 5 ++---
 read-cache.c                | 1 +
 t/t7519-status-fsmonitor.sh | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

index e928fe9d3b..08607ca7f2 100644
@@ -341,7 +341,8 @@ struct index_state {
 		 initialized : 1,
 		 drop_cache_tree : 1,
 		 updated_workdir : 1,
-		 updated_skipworktree : 1;
+		 updated_skipworktree : 1,
+		 fsmonitor_has_run_once : 1;
 	struct hashmap name_hash;
 	struct hashmap dir_hash;
 	struct object_id oid;

index 665bd2d425..1dee0aded1 100644
@@ -129,7 +129,6 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n
 
 void refresh_fsmonitor(struct index_state *istate)
 {
-	static int has_run_once = 0;
 	struct strbuf query_result = STRBUF_INIT;
 	int query_success = 0;
 	size_t bol; /* beginning of line */

@@ -137,9 +136,9 @@ void refresh_fsmonitor(struct index_state *istate)
 	char *buf;
 	int i;
 
-	if (!core_fsmonitor || has_run_once)
+	if (!core_fsmonitor || istate->fsmonitor_has_run_once)
 		return;
-	has_run_once = 1;
+	istate->fsmonitor_has_run_once = 1;
 
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
 	/*

index d5a74b1861..c66b5c6b26 100644
@@ -2323,6 +2323,7 @@ int discard_index(struct index_state *istate)
 	free_name_hash(istate);
 	cache_tree_free(&(istate->cache_tree));
 	istate->initialized = 0;
+	istate->fsmonitor_has_run_once = 0;
 	FREE_AND_NULL(istate->cache);
 	istate->cache_alloc = 0;
 	discard_split_index(istate);

index afd8fa7532..81a375fa0f 100755
@@ -346,7 +346,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
 	test_cmp before after
 '
 
-test_expect_failure 'discard_index() also discards fsmonitor info' '
+test_expect_success 'discard_index() also discards fsmonitor info' '
 	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
 	test_might_fail git update-index --refresh &&
 	test-tool read-cache --print-and-refresh=tracked 2 >actual &&

2.21.0.593.g511ec345e18

Thanks. With this many topic backlog in flight (and not yet in 'pu'), a resend like this, which is much easier to handle than mere reference to the previous discussion thread, is really appreciated.

Hmph, surprisingly, 1/2 alone did not reproduce any breakage.

    ... goes and looks ...

Ah, that's false alarm.

As I use prove, "make test" treats known breakage as a non-event and does not show, which made me lose a few minutes scratching my head wondering what I did wrong. It would have been nicer if it were easy to use 1/2 alone with the new test marked to expect success, but with a patch split into two like this, it requires going in to the test script and changing s/_failure/_success/, which is a bit suboptimial.

Anyway, will queue this to 'pu' and hopefully we can merge it down soonish.

Thanks.

Hi Ævar,

I finally found some time to track this down (it only took me two days...): it is a racy condition that *hides* the bug.

Details and a fix in https://github.com/gitgitgadget/git/pull/223

Ciao,
Dscho