[PATCH 0/2] Fix fsmonitor after discard_index()

[PATCH 0/2] Fix fsmonitor after discard_index()

[Johannes Schindelin via GitGitGadget]

It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/nycvar.QRO.7.76.6.1903142058130.41@tvgsbejvaqbjf.bet/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

[PATCH 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh | 10 ++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d674c88ba0..7e79b555de 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -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;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 3e0a61db23..918bc323ab 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

[PATCH 2/2] fsmonitor: force a refresh after the index was discarded

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 cache.h                     | 3 ++-
 fsmonitor.c                 | 5 ++---
 read-cache.c                | 1 +
 t/t7519-status-fsmonitor.sh | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index abd518a9a2..8d07c29c2a 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
diff --git a/fsmonitor.c b/fsmonitor.c
index 665bd2d425..1dee0aded1 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -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");
 	/*
diff --git a/read-cache.c b/read-cache.c
index 4dc6de1b55..0bf39e177a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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);
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 918bc323ab..72b9ed3e45 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

Re: [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()

[Johannes Sixt]

Am 16.03.19 um 10:49 schrieb Johannes Schindelin via GitGitGadget:
> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> index 3e0a61db23..918bc323ab 100755
> --- a/t/t7519-status-fsmonitor.sh
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -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" &&

Did you mean
		"git config core.fsmonitor ...
?

> +	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&

And then, does this not unset core.fsmonitor after the test anyway?

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

-- Hannes

Re: [PATCH 0/2] Fix fsmonitor after discard_index()

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 16 2019, Johannes Schindelin via GitGitGadget wrote:

> It was reported by Ævar Arnfjörð Bjarmason
> [https://public-inbox.org/git/nycvar.QRO.7.76.6.1903142058130.41@tvgsbejvaqbjf.bet/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.

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

> (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.)

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.

Re: [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()

[Johannes Schindelin]

Hi Hannes,

On Sat, 16 Mar 2019, Johannes Sixt wrote:

> Am 16.03.19 um 10:49 schrieb Johannes Schindelin via GitGitGadget:
> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> > index 3e0a61db23..918bc323ab 100755
> > --- a/t/t7519-status-fsmonitor.sh
> > +++ b/t/t7519-status-fsmonitor.sh
> > @@ -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" &&
>
> Did you mean
> 		"git config core.fsmonitor ...
> ?

D'oh, yes.

> > +	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
>
> And then, does this not unset core.fsmonitor after the test anyway?

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

>
> > +	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
>
> -- Hannes
>

[PATCH v2 0/2] Fix fsmonitor after discard_index()

[Johannes Schindelin via GitGitGadget]

It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/nycvar.QRO.7.76.6.1903142058130.41@tvgsbejvaqbjf.bet/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

[PATCH v2 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh |  8 ++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d674c88ba0..7e79b555de 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -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;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 3e0a61db23..afd8fa7532 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

[PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 cache.h                     | 3 ++-
 fsmonitor.c                 | 5 ++---
 read-cache.c                | 1 +
 t/t7519-status-fsmonitor.sh | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index ac92421f3a..7434740c99 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
diff --git a/fsmonitor.c b/fsmonitor.c
index 665bd2d425..1dee0aded1 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -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");
 	/*
diff --git a/read-cache.c b/read-cache.c
index 4dc6de1b55..0bf39e177a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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);
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index afd8fa7532..81a375fa0f 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

Re: [PATCH 0/2] Fix fsmonitor after discard_index()

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

Hi Ævar,

On Mon, 18 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

> On Sat, Mar 16 2019, Johannes Schindelin via GitGitGadget wrote:
>
> > It was reported by Ævar Arnfjörð Bjarmason
> > [https://public-inbox.org/git/nycvar.QRO.7.76.6.1903142058130.41@tvgsbejvaqbjf.bet/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.
>
> I've added this to my internal build & now the test suite passes in the
> fsmonitor mode without any test skipping.

Awesome.

> > (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.)
>
> 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.

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

Re: [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded

[SZEDER Gábor]

On Thu, Mar 21, 2019 at 06:57:34AM -0700, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

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

[PATCH v3 0/2] Fix fsmonitor after discard_index()

[Ævar Arnfjörð Bjarmason]

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.
     
         Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
     
    @@ -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

[PATCH v3 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()

[Ævar Arnfjörð Bjarmason]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/helper/test-read-cache.c  | 24 +++++++++++++++++++++++-
 t/t7519-status-fsmonitor.sh |  8 ++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d674c88ba0..7e79b555de 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -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;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 3e0a61db23..afd8fa7532 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

[PATCH v3 2/2] fsmonitor: force a refresh after the index was discarded

[Ævar Arnfjörð Bjarmason]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 cache.h                     | 3 ++-
 fsmonitor.c                 | 5 ++---
 read-cache.c                | 1 +
 t/t7519-status-fsmonitor.sh | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index e928fe9d3b..08607ca7f2 100644
--- a/cache.h
+++ b/cache.h
@@ -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;
diff --git a/fsmonitor.c b/fsmonitor.c
index 665bd2d425..1dee0aded1 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -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");
 	/*
diff --git a/read-cache.c b/read-cache.c
index d5a74b1861..c66b5c6b26 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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);
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index afd8fa7532..81a375fa0f 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

Re: [PATCH v3 0/2] Fix fsmonitor after discard_index()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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

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.

Re: [PATCH v3 0/2] Fix fsmonitor after discard_index()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
> 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.

Re: [PATCH 0/2] Fix fsmonitor after discard_index()

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

Hi Ævar,

On Mon, 18 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

>
> On Sat, Mar 16 2019, Johannes Schindelin via GitGitGadget wrote:
>
> > It was reported by Ævar Arnfjörð Bjarmason
> > [https://public-inbox.org/git/nycvar.QRO.7.76.6.1903142058130.41@tvgsbejvaqbjf.bet/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.
>
> I've added this to my internal build & now the test suite passes in the
> fsmonitor mode without any test skipping.
>
> > (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.)
>
> 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.

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