git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] Fix fsmonitor after discard_index()
@ 2019-03-16  9:49 Johannes Schindelin via GitGitGadget
  2019-03-16  9:49 ` [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-03-16  9:49 UTC (permalink / raw)
  To: git; +Cc: Ben Peart, Ævar Arnfjörð Bjarmason, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()
  2019-03-16  9:49 [PATCH 0/2] Fix fsmonitor after discard_index() Johannes Schindelin via GitGitGadget
@ 2019-03-16  9:49 ` Johannes Schindelin via GitGitGadget
  2019-03-16 10:26   ` Johannes Sixt
  2019-03-16  9:49 ` [PATCH 2/2] fsmonitor: force a refresh after the index was discarded Johannes Schindelin via GitGitGadget
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-03-16  9:49 UTC (permalink / raw)
  To: git
  Cc: Ben Peart, Ævar Arnfjörð Bjarmason,
	Junio C Hamano, Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/2] fsmonitor: force a refresh after the index was discarded
  2019-03-16  9:49 [PATCH 0/2] Fix fsmonitor after discard_index() Johannes Schindelin via GitGitGadget
  2019-03-16  9:49 ` [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
@ 2019-03-16  9:49 ` Johannes Schindelin via GitGitGadget
  2019-03-18 11:05 ` [PATCH 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
  2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
  3 siblings, 0 replies; 17+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-03-16  9:49 UTC (permalink / raw)
  To: git
  Cc: Ben Peart, Ævar Arnfjörð Bjarmason,
	Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()
  2019-03-16  9:49 ` [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
@ 2019-03-16 10:26   ` Johannes Sixt
  2019-03-21 12:32     ` Johannes Schindelin
  0 siblings, 1 reply; 17+ messages in thread
From: Johannes Sixt @ 2019-03-16 10:26 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Ben Peart,
	Ævar Arnfjörð Bjarmason, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] Fix fsmonitor after discard_index()
  2019-03-16  9:49 [PATCH 0/2] Fix fsmonitor after discard_index() Johannes Schindelin via GitGitGadget
  2019-03-16  9:49 ` [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
  2019-03-16  9:49 ` [PATCH 2/2] fsmonitor: force a refresh after the index was discarded Johannes Schindelin via GitGitGadget
@ 2019-03-18 11:05 ` Ævar Arnfjörð Bjarmason
  2019-03-21 14:36   ` Johannes Schindelin
  2019-05-23 20:49   ` Johannes Schindelin
  2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
  3 siblings, 2 replies; 17+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-03-18 11:05 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Ben Peart, Junio C Hamano


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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()
  2019-03-16 10:26   ` Johannes Sixt
@ 2019-03-21 12:32     ` Johannes Schindelin
  0 siblings, 0 replies; 17+ messages in thread
From: Johannes Schindelin @ 2019-03-21 12:32 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Johannes Schindelin via GitGitGadget, git, Ben Peart,
	Ævar Arnfjörð Bjarmason, Junio C Hamano

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
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 0/2] Fix fsmonitor after discard_index()
  2019-03-16  9:49 [PATCH 0/2] Fix fsmonitor after discard_index() Johannes Schindelin via GitGitGadget
                   ` (2 preceding siblings ...)
  2019-03-18 11:05 ` [PATCH 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
@ 2019-03-21 13:57 ` Johannes Schindelin via GitGitGadget
  2019-03-21 13:57   ` [PATCH v2 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
                     ` (4 more replies)
  3 siblings, 5 replies; 17+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-03-21 13:57 UTC (permalink / raw)
  To: git
  Cc: Ben Peart, Ævar Arnfjörð Bjarmason, Johannes Sixt,
	Junio C Hamano

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()
  2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
@ 2019-03-21 13:57   ` Johannes Schindelin via GitGitGadget
  2019-03-21 13:57   ` [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded Johannes Schindelin via GitGitGadget
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 17+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-03-21 13:57 UTC (permalink / raw)
  To: git
  Cc: Ben Peart, Ævar Arnfjörð Bjarmason, Johannes Sixt,
	Junio C Hamano, Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded
  2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
  2019-03-21 13:57   ` [PATCH v2 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
@ 2019-03-21 13:57   ` Johannes Schindelin via GitGitGadget
  2019-03-22 11:04     ` SZEDER Gábor
  2019-05-07 11:10   ` [PATCH v3 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-03-21 13:57 UTC (permalink / raw)
  To: git
  Cc: Ben Peart, Ævar Arnfjörð Bjarmason, Johannes Sixt,
	Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] Fix fsmonitor after discard_index()
  2019-03-18 11:05 ` [PATCH 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
@ 2019-03-21 14:36   ` Johannes Schindelin
  2019-05-23 20:49   ` Johannes Schindelin
  1 sibling, 0 replies; 17+ messages in thread
From: Johannes Schindelin @ 2019-03-21 14:36 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Schindelin via GitGitGadget, git, Ben Peart, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded
  2019-03-21 13:57   ` [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded Johannes Schindelin via GitGitGadget
@ 2019-03-22 11:04     ` SZEDER Gábor
  0 siblings, 0 replies; 17+ messages in thread
From: SZEDER Gábor @ 2019-03-22 11:04 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Ben Peart, Ævar Arnfjörð Bjarmason,
	Johannes Sixt, Junio C Hamano, Johannes Schindelin

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?


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 0/2] Fix fsmonitor after discard_index()
  2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
  2019-03-21 13:57   ` [PATCH v2 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
  2019-03-21 13:57   ` [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded Johannes Schindelin via GitGitGadget
@ 2019-05-07 11:10   ` Ævar Arnfjörð Bjarmason
  2019-05-07 14:00     ` Junio C Hamano
  2019-05-07 11:10   ` [PATCH v3 1/2] fsmonitor: demonstrate that it is not refreshed " Ævar Arnfjörð Bjarmason
  2019-05-07 11:10   ` [PATCH v3 2/2] fsmonitor: force a refresh after the index was discarded Ævar Arnfjörð Bjarmason
  4 siblings, 1 reply; 17+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-07 11:10 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Ben Peart, Johannes Sixt, SZEDER Gábor,
	Æ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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()
  2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
                     ` (2 preceding siblings ...)
  2019-05-07 11:10   ` [PATCH v3 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
@ 2019-05-07 11:10   ` Ævar Arnfjörð Bjarmason
  2019-05-07 11:10   ` [PATCH v3 2/2] fsmonitor: force a refresh after the index was discarded Ævar Arnfjörð Bjarmason
  4 siblings, 0 replies; 17+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-07 11:10 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Ben Peart, Johannes Sixt, SZEDER Gábor,
	Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 2/2] fsmonitor: force a refresh after the index was discarded
  2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
                     ` (3 preceding siblings ...)
  2019-05-07 11:10   ` [PATCH v3 1/2] fsmonitor: demonstrate that it is not refreshed " Ævar Arnfjörð Bjarmason
@ 2019-05-07 11:10   ` Ævar Arnfjörð Bjarmason
  4 siblings, 0 replies; 17+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-07 11:10 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Ben Peart, Johannes Sixt, SZEDER Gábor,
	Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] Fix fsmonitor after discard_index()
  2019-05-07 11:10   ` [PATCH v3 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
@ 2019-05-07 14:00     ` Junio C Hamano
  2019-05-08  3:12       ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2019-05-07 14:00 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Ben Peart, Johannes Sixt, SZEDER Gábor

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] Fix fsmonitor after discard_index()
  2019-05-07 14:00     ` Junio C Hamano
@ 2019-05-08  3:12       ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2019-05-08  3:12 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Ben Peart, Johannes Sixt, SZEDER Gábor

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] Fix fsmonitor after discard_index()
  2019-03-18 11:05 ` [PATCH 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
  2019-03-21 14:36   ` Johannes Schindelin
@ 2019-05-23 20:49   ` Johannes Schindelin
  1 sibling, 0 replies; 17+ messages in thread
From: Johannes Schindelin @ 2019-05-23 20:49 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Schindelin via GitGitGadget, git, Ben Peart, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-05-23 20:49 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-16  9:49 [PATCH 0/2] Fix fsmonitor after discard_index() Johannes Schindelin via GitGitGadget
2019-03-16  9:49 ` [PATCH 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
2019-03-16 10:26   ` Johannes Sixt
2019-03-21 12:32     ` Johannes Schindelin
2019-03-16  9:49 ` [PATCH 2/2] fsmonitor: force a refresh after the index was discarded Johannes Schindelin via GitGitGadget
2019-03-18 11:05 ` [PATCH 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
2019-03-21 14:36   ` Johannes Schindelin
2019-05-23 20:49   ` Johannes Schindelin
2019-03-21 13:57 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
2019-03-21 13:57   ` [PATCH v2 1/2] fsmonitor: demonstrate that it is not refreshed " Johannes Schindelin via GitGitGadget
2019-03-21 13:57   ` [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded Johannes Schindelin via GitGitGadget
2019-03-22 11:04     ` SZEDER Gábor
2019-05-07 11:10   ` [PATCH v3 0/2] Fix fsmonitor after discard_index() Ævar Arnfjörð Bjarmason
2019-05-07 14:00     ` Junio C Hamano
2019-05-08  3:12       ` Junio C Hamano
2019-05-07 11:10   ` [PATCH v3 1/2] fsmonitor: demonstrate that it is not refreshed " Ævar Arnfjörð Bjarmason
2019-05-07 11:10   ` [PATCH v3 2/2] fsmonitor: force a refresh after the index was discarded Ævar Arnfjörð Bjarmason

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git