threads / patch / 50754

patch, 2 partsFix fsmonitor after discard_index()

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

## tl;dr

17 messages between Mar 16, 2019 and May 23, 2019. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

Johannes Schindelin via GitGitGadget· Mar 16, 2019, 09:49 UTC · lore

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
Johannes Schindelin via GitGitGadget· Mar 16, 2019, 09:49 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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

t/helper/test-read-cache.c, t/t7519-status-fsmonitor.sh

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
Johannes Sixt· Mar 16, 2019, 10:26 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

Am 16.03.19 um 10:49 schrieb Johannes Schindelin via GitGitGadget:
Show 11 quoted lines
> 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?
Show 7 quoted lines
> +	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
Johannes Schindelin· Mar 21, 2019, 12:32 UTC · re: Johannes Sixt · lore

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

Hi Hannes,
On Sat, 16 Mar 2019, Johannes Sixt wrote:
Show 16 quoted lines
> 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

Show 11 quoted lines
>
> > +	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
>
Johannes Schindelin via GitGitGadget· Mar 16, 2019, 09:49 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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(-)
Show changes to 4 files +6 −5

cache.h, fsmonitor.c, read-cache.c, t/t7519-status-fsmonitor.sh

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
Ævar Arnfjörð Bjarmason· Mar 18, 2019, 11:05 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

On Sat, Mar 16 2019, Johannes Schindelin via GitGitGadget wrote:
Show 9 quoted lines
> 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.

Johannes Schindelin· Mar 21, 2019, 14:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Hi Ævar,
On Mon, 18 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
Show 14 quoted lines
> 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.
Show 12 quoted lines
> > (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

Johannes Schindelin· May 23, 2019, 20:49 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Hi Ævar,
On Mon, 18 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
Show 30 quoted lines
>
> 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

Johannes Schindelin via GitGitGadget· Mar 21, 2019, 13:57 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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
Johannes Schindelin via GitGitGadget· Mar 21, 2019, 13:57 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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

t/helper/test-read-cache.c, t/t7519-status-fsmonitor.sh

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
Johannes Schindelin via GitGitGadget· Mar 21, 2019, 13:57 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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(-)
Show changes to 4 files +6 −5

cache.h, fsmonitor.c, read-cache.c, t/t7519-status-fsmonitor.sh

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
SZEDER Gábor· Mar 22, 2019, 11:04 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

On Thu, Mar 21, 2019 at 06:57:34AM -0700, Johannes Schindelin via GitGitGadget wrote:
Show 8 quoted lines
> 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?
Ævar Arnfjörð Bjarmason· May 7, 2019, 11:10 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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
Junio C Hamano· May 7, 2019, 14:00 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Junio C Hamano· May 8, 2019, 03:12 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> Æ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.
Ævar Arnfjörð Bjarmason· May 7, 2019, 11:10 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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

t/helper/test-read-cache.c, t/t7519-status-fsmonitor.sh

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
Ævar Arnfjörð Bjarmason· May 7, 2019, 11:10 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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(-)
Show changes to 4 files +6 −5

cache.h, fsmonitor.c, read-cache.c, t/t7519-status-fsmonitor.sh

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

← back to recent threads