Fix fsmonitor after discard_index()
cover Fix fsmonitor after discard_index()
It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/[email protected]/T/#mb8718fe52e4721dacd3b143a09187ff9090ef4e3]
that there were problems with the fsmonitor feature in conjunction with the newly built-in git stash/git rebase.
The culprit really is that the fsmonitor flag that says whether it was queried already was not re-set after discard_index() was called by mistake.
This fixes that, and apparently also other long-standing fsmonitor issues.
(Note that there is still a flakiness around t7519
[https://github.com/git-for-windows/git/pull/2127#pullrequestreview-215010574]
where it tries to make sure that the fsmonitor hook can prevent unnecessary lstat() calls, but that seems to be unrelated to this here bug.)
Johannes Schindelin (2):
fsmonitor: demonstrate that it is not refreshed after discard_index()
fsmonitor: force a refresh after the index was discarded
cache.h | 3 ++- fsmonitor.c | 5 ++--- read-cache.c | 1 + t/helper/test-read-cache.c | 24 +++++++++++++++++++++++- t/t7519-status-fsmonitor.sh | 10 ++++++++++ 5 files changed, 38 insertions(+), 5 deletions(-)
base-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-165%2Fdscho%2Ffix-fsmonitor-after-discard-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-165/dscho/fix-fsmonitor-after-discard-index-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/165
Signature · 1 line
gitgitgadget
PATCH 1/2 fsmonitor: demonstrate that it is not refreshed after discard_index()
From: Johannes Schindelin <[email protected]>
This one is tricky.
When core.fsmonitor is set, a refresh_index() will not perform a full scan of files that might be modified, but will query the fsmonitor and refresh only the ones that have been actually touched.
Due to implementation details, the fsmonitor is queried in refresh_cache_ent(), but of course it only has to be queried once, so we set a flag when we did that. But when the index was discarded, we did not re-set that flag.
So far, this is only covered by our test suite when running with GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all, and only due to the way the built-in stash interacts with the recursive merge machinery.
Let's introduce a straight-forward regression test for this.
We simply extend the "read & discard index" loop in test-tool read-cache to optionally refresh the index, report on a given file's status, and then modify that file. Due to the bug described above, only the first refresh will actually query the fsmonitor; subsequent loop iterations will not.
This problem was reported by Ævar Arnfjörð Bjarmason.
--- t/helper/test-read-cache.c | 24 +++++++++++++++++++++++- t/t7519-status-fsmonitor.sh | 10 ++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-)
t/helper/test-read-cache.c +23 -1
@@ -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 ;
t/t7519-status-fsmonitor.sh +10 -0
@@ -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
Signature · 1 line
gitgitgadget
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:
t/t7519-status-fsmonitor.sh test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' · line 351
Quoted 11 lines
t/t7519-status-fsmonitor.sh +3
-0@@ -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 ...
?
t/t7519-status-fsmonitor.sh test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' · line 352
Quoted 1 line
t/t7519-status-fsmonitor.sh +1
-0+ test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
And then, does this not unset core.fsmonitor after the test anyway?
t/t7519-status-fsmonitor.sh test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' · line 359
Quoted 7 lines
t/t7519-status-fsmonitor.sh +6
-0+ 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 1/2] fsmonitor: demonstrate that it is not refreshed after discard_index()
Hi Hannes,
Quoted 9 lines · Johannes Sixt
Am 16.03.19 um 10:49 schrieb Johannes Schindelin via GitGitGadget:
Quoted 8 lines
t/t7519-status-fsmonitor.sh +0
-0@@ -346,4 +346,14 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' test_cmp before after '
Quoted 7 lines
t/t7519-status-fsmonitor.sh test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' · line 351
Quoted 3 lines
t/t7519-status-fsmonitor.sh +3
-0+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.
Quoted 3 lines
t/t7519-status-fsmonitor.sh test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' · line 352
Quoted 1 line
t/t7519-status-fsmonitor.sh +1
-0+ 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
Quoted 11 lines
t/t7519-status-fsmonitor.sh test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' · line 359
Quoted 8 lines
t/t7519-status-fsmonitor.sh +6
-0+ 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 2/2 fsmonitor: force a refresh after the index was discarded
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
With this change, the index_state struct becomes the new home for the flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.
It also gets re-set when the index is discarded, fixing the bug where fsmonitor-enabled Git would miss updates under certain circumstances.
--- cache.h | 3 ++- fsmonitor.c | 5 ++--- read-cache.c | 1 + t/t7519-status-fsmonitor.sh | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
cache.h +2 -1
@@ -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;
fsmonitor.c +2 -3
@@ -129,7 +129,6 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *nvoid 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" );/*
read-cache.c +1 -0
@@ -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);
t/t7519-status-fsmonitor.sh +1 -1
@@ -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" &&
Signature · 1 line
gitgitgadget
Quoted 9 lines · Johannes Schindelin via GitGitGadget
It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/[email protected]/T/#mb8718fe52e4721dacd3b143a09187ff9090ef4e3]
that there were problems with the fsmonitor feature in conjunction with the newly built-in git stash/git rebase.The culprit really is that the fsmonitor flag that says whether it was queried already was not re-set after
discard_index()was called by mistake.This fixes that, and apparently also other long-standing fsmonitor issues.
I've added this to my internal build & now the test suite passes in the fsmonitor mode without any test skipping.
Quoted 4 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 unnecessarylstat()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.
Hi Ævar,
Quoted 7 lines · Ævar Arnfjörð Bjarmason
Quoted 5 lines · Johannes Schindelin via GitGitGadget
It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/[email protected]/T/#mb8718fe52e4721dacd3b143a09187ff9090ef4e3]
that there were problems with the fsmonitor feature in conjunction with the newly built-in git stash/git rebase.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 5 lines
Quoted 4 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 unnecessarylstat()calls, but that seems to be unrelated to this here bug.)
Quoted 7 lines
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
Hi Ævar,
Quoted 23 lines · Ævar Arnfjörð Bjarmason
Quoted 11 lines · Johannes Schindelin via GitGitGadget
It was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/[email protected]/T/#mb8718fe52e4721dacd3b143a09187ff9090ef4e3]
that there were problems with the fsmonitor feature in conjunction with the newly built-in git stash/git rebase.The culprit really is that the fsmonitor flag that says whether it was queried already was not re-set after
discard_index()was called by mistake.This fixes that, and apparently also other long-standing fsmonitor issues.
I've added this to my internal build & now the test suite passes in the fsmonitor mode without any test skipping.
Quoted 4 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 unnecessarylstat()calls, but that seems to be unrelated to this here bug.)
Quoted 7 lines
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
v2 cover Fix fsmonitor after discard_index()
In reply to Johannes Schindelin via GitGitGadgetIt was reported by Ævar Arnfjörð Bjarmason
[https://public-inbox.org/git/[email protected]/T/#mb8718fe52e4721dacd3b143a09187ff9090ef4e3]
that there were problems with the fsmonitor feature in conjunction with the newly built-in git stash/git rebase.
The culprit really is that the fsmonitor flag that says whether it was queried already was not re-set after discard_index() was called by mistake.
This fixes that, and apparently also other long-standing fsmonitor issues.
(Note that there is still a flakiness around t7519
[https://github.com/git-for-windows/git/pull/2127#pullrequestreview-215010574]
where it tries to make sure that the fsmonitor hook can prevent unnecessary lstat() calls, but that seems to be unrelated to this here bug.)
Changes since v1:
* Removed unnecessary test_when_finished "test_config ..." line, pointed out by Hannes Sixt.
Johannes Schindelin (2):
fsmonitor: demonstrate that it is not refreshed after discard_index()
fsmonitor: force a refresh after the index was discarded
cache.h | 3 ++- fsmonitor.c | 5 ++--- read-cache.c | 1 + t/helper/test-read-cache.c | 24 +++++++++++++++++++++++- t/t7519-status-fsmonitor.sh | 8 ++++++++ 5 files changed, 36 insertions(+), 5 deletions(-)
base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-165%2Fdscho%2Ffix-fsmonitor-after-discard-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-165/dscho/fix-fsmonitor-after-discard-index-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/165
Range-diff vs v1:
1: f451752656 ! 1: 51a7edf22a fsmonitor: demonstrate that it is not refreshed after discard_index()
@@ -79,8 +79,6 @@
'
+test_expect_failure 'discard_index() also discards fsmonitor info' '
-+ test_when_finished \
-+ "git config core.monitor .git/hooks/fsmonitor-test" &&
+ test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
+ test_might_fail git update-index --refresh &&
+ test-tool read-cache --print-and-refresh=tracked 2 >actual && 2: 1d62623776 ! 2: 79fdd0d586 fsmonitor: force a refresh after the index was discarded
@@ -70,6 +70,6 @@
-test_expect_failure 'discard_index() also discards fsmonitor info' ' +test_expect_success 'discard_index() also discards fsmonitor info' '
- test_when_finished \
- "git config core.monitor .git/hooks/fsmonitor-test" &&
test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-all" &&
+ test_might_fail git update-index --refresh &&
+ test-tool read-cache --print-and-refresh=tracked 2 >actual &&
Signature · 1 line
gitgitgadget
PATCH v2 1/2 fsmonitor: demonstrate that it is not refreshed after discard_index()
From: Johannes Schindelin <[email protected]>
This one is tricky.
When core.fsmonitor is set, a refresh_index() will not perform a full scan of files that might be modified, but will query the fsmonitor and refresh only the ones that have been actually touched.
Due to implementation details, the fsmonitor is queried in refresh_cache_ent(), but of course it only has to be queried once, so we set a flag when we did that. But when the index was discarded, we did not re-set that flag.
So far, this is only covered by our test suite when running with GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all, and only due to the way the built-in stash interacts with the recursive merge machinery.
Let's introduce a straight-forward regression test for this.
We simply extend the "read & discard index" loop in test-tool read-cache to optionally refresh the index, report on a given file's status, and then modify that file. Due to the bug described above, only the first refresh will actually query the fsmonitor; subsequent loop iterations will not.
This problem was reported by Ævar Arnfjörð Bjarmason.
--- t/helper/test-read-cache.c | 24 +++++++++++++++++++++++- t/t7519-status-fsmonitor.sh | 8 ++++++++ 2 files changed, 31 insertions(+), 1 deletion(-)
t/helper/test-read-cache.c +23 -1
@@ -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 ;
t/t7519-status-fsmonitor.sh +8 -0
@@ -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
Signature · 1 line
gitgitgadget
PATCH v2 2/2 fsmonitor: force a refresh after the index was discarded
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
With this change, the index_state struct becomes the new home for the flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.
It also gets re-set when the index is discarded, fixing the bug where fsmonitor-enabled Git would miss updates under certain circumstances.
--- cache.h | 3 ++- fsmonitor.c | 5 ++--- read-cache.c | 1 + t/t7519-status-fsmonitor.sh | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
cache.h +2 -1
@@ -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;
fsmonitor.c +2 -3
@@ -129,7 +129,6 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *nvoid 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" );/*
read-cache.c +1 -0
@@ -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);
t/t7519-status-fsmonitor.sh +1 -1
@@ -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 &&
Signature · 1 line
gitgitgadget
Re: [PATCH v2 2/2] fsmonitor: force a refresh after the index was discarded
Quoted 8 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
With this change, the
index_statestruct 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?
v3 cover Fix fsmonitor after discard_index()
In reply to Johannes Schindelin via GitGitGadgetThis v3 is all Johannes's patches. The outstanding review on v2 could be clarified with a commit message change, which I've addressed, and v2 conflicted with a cache.h change that's since landed in "master", which I've rebased this on.
Junio: We're getting closer to the release so it would be great to have this. It's been broken for a long time, but having this finaly fixed in v2.22 would be great. The functional code changes are also isolated to the fsmonitor code path, which reduces the risk and makes this easier to review/reason about.
Johannes Schindelin (2):
fsmonitor: demonstrate that it is not refreshed after discard_index()
fsmonitor: force a refresh after the index was discarded
cache.h | 3 ++- fsmonitor.c | 5 ++--- read-cache.c | 1 + t/helper/test-read-cache.c | 24 +++++++++++++++++++++++- t/t7519-status-fsmonitor.sh | 8 ++++++++ 5 files changed, 36 insertions(+), 5 deletions(-)
Range-diff:
1: 51a7edf22a = 1: c31f834b07 fsmonitor: demonstrate that it is not refreshed after discard_index() 2: 79fdd0d586 ! 2: 7bf5f9f610 fsmonitor: force a refresh after the index was discarded
@@ -6,8 +6,10 @@
flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.
- It also gets re-set when the index is discarded, fixing the bug where
- fsmonitor-enabled Git would miss updates under certain circumstances.
+ It also gets re-set when the index is discarded, fixing the bug
+ demonstrated by the "test_expect_failure" test added in the preceding
+ commit. In that case fsmonitor-enabled Git would miss updates under
+ certain circumstances, see that preceding commit for details.
@@ -15,11 +17,11 @@
--- a/cache.h
+++ b/cache.h
@@
- struct cache_time timestamp;
- unsigned name_hash_initialized : 1,
initialized : 1,
-- drop_cache_tree : 1;
-+ drop_cache_tree : 1,
+ drop_cache_tree : 1,
+ updated_workdir : 1,
+- updated_skipworktree : 1;
++ updated_skipworktree : 1,
+ fsmonitor_has_run_once : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
Signature · 1 line
2.21.0.593.g511ec345e18
Quoted 10 lines · Æ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.hchange 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.
Quoted 13 lines · Junio C Hamano
Quoted 10 lines · Æ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.hchange 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.
Quoted 3 lines
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.
PATCH v3 1/2 fsmonitor: demonstrate that it is not refreshed after discard_index()
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This one is tricky.
When core.fsmonitor is set, a refresh_index() will not perform a full scan of files that might be modified, but will query the fsmonitor and refresh only the ones that have been actually touched.
Due to implementation details, the fsmonitor is queried in refresh_cache_ent(), but of course it only has to be queried once, so we set a flag when we did that. But when the index was discarded, we did not re-set that flag.
So far, this is only covered by our test suite when running with GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all, and only due to the way the built-in stash interacts with the recursive merge machinery.
Let's introduce a straight-forward regression test for this.
We simply extend the "read & discard index" loop in test-tool read-cache to optionally refresh the index, report on a given file's status, and then modify that file. Due to the bug described above, only the first refresh will actually query the fsmonitor; subsequent loop iterations will not.
This problem was reported by Ævar Arnfjörð Bjarmason.
--- t/helper/test-read-cache.c | 24 +++++++++++++++++++++++- t/t7519-status-fsmonitor.sh | 8 ++++++++ 2 files changed, 31 insertions(+), 1 deletion(-)
t/helper/test-read-cache.c +23 -1
@@ -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 ;
t/t7519-status-fsmonitor.sh +8 -0
@@ -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
Signature · 1 line
2.21.0.593.g511ec345e18
PATCH v3 2/2 fsmonitor: force a refresh after the index was discarded
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
With this change, the index_state struct becomes the new home for the flag that says whether the fsmonitor hook has been run, i.e. it is now per-index.
It also gets re-set when the index is discarded, fixing the bug demonstrated by the "test_expect_failure" test added in the preceding commit. In that case fsmonitor-enabled Git would miss updates under certain circumstances, see that preceding commit for details.
--- cache.h | 3 ++- fsmonitor.c | 5 ++--- read-cache.c | 1 + t/t7519-status-fsmonitor.sh | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
cache.h +2 -1
@@ -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;
fsmonitor.c +2 -3
@@ -129,7 +129,6 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *nvoid 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" );/*
read-cache.c +1 -0
@@ -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);
t/t7519-status-fsmonitor.sh +1 -1
@@ -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 &&
Signature · 1 line
2.21.0.593.g511ec345e18