Empty untracked cache performance issue
cover Empty untracked cache performance issue
This patchset addresses a performance issue with untracked cache. When a new untracked cache structure is added to the index but remains empty, subsequent "git status" calls populate it but do not write the index - so they perform as though the index were disabled.
This situation can be caused in several different ways:
* Running "git update-index --untracked-cache" on a repo that did not have the untracked cache
* Modifying the git configuration to enable untracked cache, but then immediately running "git status -uall", causing the untracked cache to be created, but not used/populated (and the index written).
* (likely others)
The patchset includes fixes to t7519, which otherwise starts failing because it wasn't testing what it intended to.
Tao Klerks (3):
Add a second's delay to t7519 for untracked cache In t7519, populate untracked cache before test Write index when populating empty untracked cache
dir.c | 14 +++++++++++--- t/t7519-status-fsmonitor.sh | 6 ++++++ 2 files changed, 17 insertions(+), 3 deletions(-)
base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-986%2FTaoK%2Ftaok-empty-untracked-cache-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-986/TaoK/taok-empty-untracked-cache-bug-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/986
Signature · 1 line
gitgitgadget
PATCH 1/3 Add a second's delay to t7519 for untracked cache
From: Tao Klerks <[email protected]>
In t7519 there is a test that writes files to disk, and immediately writes the untracked index. Because of mtime-comparison logic that uses a 1-second resolution, this means the cached entries are not trusted/used under some circumstances (see read-cache.c#is_racy_stat()).
Untracked cache tests in t7063 use a 1-second delay to avoid this issue. We should do the same here.
This change doesn't actually affect the outcome of the test, but does enhance its validity, and becomes relevant after later changes
--- t/t7519-status-fsmonitor.sh | 5 +++++ 1 file changed, 5 insertions(+)
t/t7519-status-fsmonitor.sh +5 -0
@@ -13,6 +13,10 @@ clean_repo () { git clean -fd } +avoid_racy() { + sleep 1 +} + dirty_repo () { : >untracked && : >dir1/untracked &&
@@ -332,6 +336,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' : >dir2/modified && write_integration_script && git config core.fsmonitor .git/hooks/fsmonitor-test && + avoid_racy && git update-index --untracked-cache && git update-index --fsmonitor && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace-before" \
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] Add a second's delay to t7519 for untracked cache
Quoted 10 lines · Tao Klerks via GitGitGadget
From: Tao Klerks <[email protected]>
In t7519 there is a test that writes files to disk, and immediately writes the untracked index. Because of mtime-comparison logic that uses a 1-second resolution, this means the cached entries are not trusted/used under some circumstances (see read-cache.c#is_racy_stat()).
Untracked cache tests in t7063 use a 1-second delay to avoid this issue. We should do the same here.
We shouldn't waste wallclock time by slowing down tests, which already take way too much time. If you can use "test-tool chmtime" to pretend as if the index file was written earlier than it actually is, that would be a better solution.
Quoted 1 line
+avoid_racy() {
Style (see below).
Quoted 4 lines
t/t7519-status-fsmonitor.sh +3
-0+ sleep 1 +} + dirty_repo () {
PATCH 2/3 In t7519, populate untracked cache before test
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
In its current state, the t7519 test dealing with untracked cache assumes that
"git update-index --untracked-cache" will *populate* the untracked cache. This is not correct - it will only add an empty untracked cache structure to the index.
If we're going to compare two git status runs with something interesting happening in-between, we need to ensure that the index is in a stable/steady state *before* that first run.
We achieve this by adding another prior "git status" run.
At this stage this change does nothing, because there is a bug, addressed in the next patch. whereby once the empty untracked cache structure is added by the update-index invocation, the untracked cache gets updated in every subsequent "git status" call, but the index with these updates does not get written down.
That bug actually invalidates this entire test case - but we're fixing that next.
--- t/t7519-status-fsmonitor.sh | 1 + 1 file changed, 1 insertion(+)
t/t7519-status-fsmonitor.sh +1 -0
@@ -339,6 +339,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' avoid_racy && git update-index --untracked-cache && git update-index --fsmonitor && + git status && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace-before" \ git status && test-tool dump-untracked-cache >../before
Signature · 1 line
gitgitgadget
PATCH 3/3 Write index when populating empty untracked cache
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
It is expected that an empty/unpopulated untracked cache structure can be written to the index - by update- index, or by a "git status" call that sees the untracked cache should be enabled, but is running with options that make it non-applicable in that run.
Currently, if that happens, then subsequent "git status" calls end up populating the untracked cache, but not writing the index (not saving their work) - so the performance outcome is almost identical to the cache being altogether disabled.
This continues until the index gets written with the cache populated, for some *other* reason.
In this change, we detect the "existing cache is empty and it looks like we are using it" condition, and queues an index write when this happens.
This change depends on previous fixes to t7519 for the "ignore .git changes when invalidating UNTR" test case to pass - before this fix, the test never actually did anything as it was not set up correctly.
--- dir.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
dir.c +11 -3
@@ -2703,7 +2703,8 @@ void remove_untracked_cache(struct index_state *istate)static struct untracked_cache_dir *validate_untracked_cache (struct dir_struct *dir,int base_len, -const struct pathspec *pathspec) +const struct pathspec *pathspec, +struct index_state *istate) {struct untracked_cache_dir *root;static int untracked_cache_disabled =-1 ;
@@ -2767,8 +2768,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dreturn NULL ; } -if (!dir->untracked->root) +if (!dir->untracked->root) {FLEX_ALLOC_STR (dir->untracked->root, name,"" ); +/* +* If we've had to initialize the root, then what we had was an +* empty uninitialized untracked cache structure. We will be +* populating it now, so we should trigger an index write. +*/ + istate->cache_changed |=UNTRACKED_CHANGED ; + }/* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root;
@@ -2838,7 +2846,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,return dir->nr; } - untracked =validate_untracked_cache (dir, len, pathspec); + untracked =validate_untracked_cache (dir, len, pathspec, istate);if (!untracked)/* * make sure untracked cache code path is disabled,
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] Write index when populating empty untracked cache
Quoted 1 line · Tao Klerks via GitGitGadget
Subject: Re: [PATCH 3/3] Write index when populating empty untracked cache
Common to all three patches.
Look at "git shortlog --no-merges -200 master" and observe the pattern, i.e. our commit title are <area> prefix, a colon and then a summary. Try to make your commits blend in.
Quoted 7 lines
From: Tao Klerks <[email protected]>
It is expected that an empty/unpopulated untracked cache structure can be written to the index - by update- index, or by a "git status" call that sees the untracked cache should be enabled, but is running with options that make it non-applicable in that run.
Would an example be helpful? What requests the untracked cache to be created, and what options (or perhaps the lack of what options) prevent the untracked cache to be "non-applicable"?
Quoted 8 lines
Currently, if that happens, then subsequent "git status" calls end up populating the untracked cache, but not writing the index (not saving their work) - so the performance outcome is almost identical to the cache being altogether disabled.
This continues until the index gets written with the cache populated, for some *other* reason.
Here (and only here), the word "cache" is used instead of "untracked cache"---but I suspect that "the cache" here is the same "untracked cache". If that is the case, being consistent and spelling it out would reduce the risk of confusing readers.
Quoted 3 lines
In this change, we detect the "existing cache is empty and it looks like we are using it" condition, and queues an index write when this happens.
"we detect ... and queues" sounds like a grammo.
In this project, we describe the change being proposed as if we are giving an order to the codebase to "become like so". So, perhaps
Detect the condition where an empty untracked cache exists in the index and we collect the list of untracked paths, and queue an index write under that condition, so that the collected untracked paths can be written out to the untracked cache extension in the index.
or something along the line.
Quoted 28 lines
This change depends on previous fixes to t7519 for the "ignore .git changes when invalidating UNTR" test case to pass - before this fix, the test never actually did anything as it was not set up correctly.
Signed-off-by Tao Klerks <[email protected]>--- dir.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)dir.c +2
-1@@ -2703,7 +2703,8 @@ void remove_untracked_cache(struct index_state *istate)static struct untracked_cache_dir *validate_untracked_cache (struct dir_struct *dir,int base_len, -const struct pathspec *pathspec) +const struct pathspec *pathspec, +struct index_state *istate) {struct untracked_cache_dir *root;static int untracked_cache_disabled =-1 ;@@ -2767,8 +2768,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dreturn NULL ; }
dir.c static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d · line 2779
Quoted 10 lines
dir.c +8
-1-if (!dir->untracked->root) +if (!dir->untracked->root) {FLEX_ALLOC_STR (dir->untracked->root, name,"" ); +/* +* If we've had to initialize the root, then what we had was an +* empty uninitialized untracked cache structure. We will be +* populating it now, so we should trigger an index write. +*/ + istate->cache_changed |=UNTRACKED_CHANGED ; + }
The logic sounds fairly straight-forward. The fact that we came this far in the helper function means that we know we want to use (i.e. untracked cache is not disabled, and various other "we shouldn't be contaminating the cache with the one-shot information" cases did not return from the helper) untracked cache, and root being NULL originally means the untracked cache wasn't populated.
Quoted 11 lines
dir.c +1
-1/* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root;@@ -2838,7 +2846,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,return dir->nr; } - untracked =validate_untracked_cache (dir, len, pathspec); + untracked =validate_untracked_cache (dir, len, pathspec, istate);if (!untracked)/* * make sure untracked cache code path is disabled,
Re: [PATCH 3/3] Write index when populating empty untracked cache
Hi Junio,
Thanks so much for reviewing this, and my apologies for not reacting earlier, after you took the time.
Quoted 2 lines
our commit title are <area> prefix, a colon and then a summary. Try to make your commits blend in.
Noted, will fix, thx. I believe I did this better in an unrelated patch series today, but I guess I will see what people say :)
Quoted 3 lines
What requests the untracked cache to
be created, and what options (or perhaps the lack of what options) prevent the untracked cache to be "non-applicable"?
Will try to clarify / be more specific.
Quoted 2 lines
I suspect that "the cache" here is the same "untracked cache"
Will fix.
Quoted 1 line
"we detect ... and queues" sounds like a grammo.
Yep, dreaded perspective shift, will fix.
Quoted 1 line
perhaps [proposed text] or something along the line.
Will incorporate, thx.
Quoted 1 line
The logic sounds fairly straight-forward.
I didn't understand here whether you were confirming that the change seems to make sense (yay!), or commenting that the extra comment block is redundant, stating something obvious, and should better be removed. Could you confirm please?
In the meantime, I'll "re-roll" (?) with the proposed changes.
Thanks again,
Tao
Quoted 111 lines · Junio C Hamano
Quoted 1 line · Tao Klerks via GitGitGadget
Subject: Re: [PATCH 3/3] Write index when populating empty untracked cache
Common to all three patches.
Look at "git shortlog --no-merges -200 master" and observe the pattern, i.e. our commit title are <area> prefix, a colon and then a summary. Try to make your commits blend in.
Quoted 7 lines
From: Tao Klerks <[email protected]>
It is expected that an empty/unpopulated untracked cache structure can be written to the index - by update- index, or by a "git status" call that sees the untracked cache should be enabled, but is running with options that make it non-applicable in that run.
Would an example be helpful? What requests the untracked cache to be created, and what options (or perhaps the lack of what options) prevent the untracked cache to be "non-applicable"?
Quoted 8 lines
Currently, if that happens, then subsequent "git status" calls end up populating the untracked cache, but not writing the index (not saving their work) - so the performance outcome is almost identical to the cache being altogether disabled.
This continues until the index gets written with the cache populated, for some *other* reason.
Here (and only here), the word "cache" is used instead of "untracked cache"---but I suspect that "the cache" here is the same "untracked cache". If that is the case, being consistent and spelling it out would reduce the risk of confusing readers.
Quoted 3 lines
In this change, we detect the "existing cache is empty and it looks like we are using it" condition, and queues an index write when this happens.
"we detect ... and queues" sounds like a grammo.
In this project, we describe the change being proposed as if we are giving an order to the codebase to "become like so". So, perhaps
Detect the condition where an empty untracked cache exists in the index and we collect the list of untracked paths, and queue an index write under that condition, so that the collected untracked paths can be written out to the untracked cache extension in the index.
or something along the line.
Quoted 28 lines
This change depends on previous fixes to t7519 for the "ignore .git changes when invalidating UNTR" test case to pass - before this fix, the test never actually did anything as it was not set up correctly.
Signed-off-by Tao Klerks <[email protected]>--- dir.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)dir.c +2
-1@@ -2703,7 +2703,8 @@ void remove_untracked_cache(struct index_state *istate)static struct untracked_cache_dir *validate_untracked_cache (struct dir_struct *dir,int base_len, -const struct pathspec *pathspec) +const struct pathspec *pathspec, +struct index_state *istate) {struct untracked_cache_dir *root;static int untracked_cache_disabled =-1 ;@@ -2767,8 +2768,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dreturn NULL ; }dir.c static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d · line 2779
Quoted 10 lines
dir.c +8
-1-if (!dir->untracked->root) +if (!dir->untracked->root) {FLEX_ALLOC_STR (dir->untracked->root, name,"" ); +/* +* If we've had to initialize the root, then what we had was an +* empty uninitialized untracked cache structure. We will be +* populating it now, so we should trigger an index write. +*/ + istate->cache_changed |=UNTRACKED_CHANGED ; + }The logic sounds fairly straight-forward. The fact that we came this far in the helper function means that we know we want to use (i.e. untracked cache is not disabled, and various other "we shouldn't be contaminating the cache with the one-shot information" cases did not return from the helper) untracked cache, and root being NULL originally means the untracked cache wasn't populated.
Quoted 11 lines
dir.c +1
-1/* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root;@@ -2838,7 +2846,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,return dir->nr; } - untracked =validate_untracked_cache (dir, len, pathspec); + untracked =validate_untracked_cache (dir, len, pathspec, istate);if (!untracked)/* * make sure untracked cache code path is disabled,
Re: [PATCH 3/3] Write index when populating empty untracked cache
Quoted 6 lines · Tao Klerks
Quoted 1 line
The logic sounds fairly straight-forward.
I didn't understand here whether you were confirming that the change seems to make sense (yay!), or commenting that the extra comment block is redundant, stating something obvious, and should better be removed. Could you confirm please?
I meant the former when I wrote it.
But now you made me re-read the patch, I am becoming slightly sympathetic to the "do we even need to comment?" interpretation, too ;-)
The question is if the comment to these two statements is redundant.
if (!dir->untracked->root) {
/*
* If we've had to initialize the root, then what we had was an
* empty uninitialized untracked cache structure. We will be
* populating it now, so we should trigger an index write.
*/
FLEX_ALLOC_STR(dir->untracked->root, name, "");
istate->cache_changed |= UNTRACKED_CHANGED;
}I can be pursuaded either way.
v2 cover Empty untracked cache performance issue
In reply to Tao Klerks via GitGitGadgetThis patchset addresses a performance issue with untracked cache. When a new untracked cache structure is added to the index but remains empty, subsequent "git status" calls populate it but do not write the index - so they perform as though the index were disabled.
This situation can be caused in several different ways:
* Running "git update-index --untracked-cache" on a repo that did not have the untracked cache
* Modifying the git configuration to enable untracked cache, but then immediately running "git status -uall", causing the untracked cache to be created, but not used/populated (and the index written).
* (likely others)
The patchset includes fixes to t7519, which otherwise starts failing because it wasn't testing what it intended to.
Tao Klerks (3):
t7519: avoid file to index mtime race for untracked cache
t7519: populate untracked cache before test
untracked-cache: write index when populating empty untracked cache
dir.c | 10 +++++++--- t/t7519-status-fsmonitor.sh | 7 +++++++ 2 files changed, 14 insertions(+), 3 deletions(-)
base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-986%2FTaoK%2Ftaok-empty-untracked-cache-bug-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-986/TaoK/taok-empty-untracked-cache-bug-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/986
Range-diff vs v1:
1: c4d2a3cab4b ! 1: 9421b71540d Add a second's delay to t7519 for untracked cache
@@ Metadata
Author: Tao Klerks <[email protected]>
## Commit message ##
- Add a second's delay to t7519 for untracked cache
+ t7519: avoid file to index mtime race for untracked cache
In t7519 there is a test that writes files to disk, and immediately
- writes the untracked index. Because of mtime-comparison logic
- that uses a 1-second resolution, this means the cached entries
- are not trusted/used under some circumstances (see
- read-cache.c#is_racy_stat()).
+ writes the untracked index. Because of mtime-comparison logic that
+ uses a 1-second resolution, this means the cached entries are not
+ trusted/used under some circumstances
+ (see read-cache.c#is_racy_stat()).
- Untracked cache tests in t7063 use a 1-second delay to avoid
- this issue. We should do the same here.
+ Untracked cache tests in t7063 use a 1-second delay to avoid this
+ issue, but we don't want to introduce arbitrary slowdowns, so instead
+ use test-tool chmtime to backdate the files slightly.
- This change doesn't actually affect the outcome of the test,
- but does enhance its validity, and becomes relevant after
- later changes
+ This change doesn't actually affect the outcome of the test, but does
+ enhance its validity, and becomes relevant after later changes.
## t/t7519-status-fsmonitor.sh ##
-@@ t/t7519-status-fsmonitor.sh: clean_repo () {
- git clean -fd
- }
-
-+avoid_racy() {
-+ sleep 1
-+}
-+
- dirty_repo () {
- : >untracked &&
- : >dir1/untracked &&
@@ t/t7519-status-fsmonitor.sh: test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
+ cd dot-git &&
+ mkdir -p .git/hooks &&
+ : >tracked &&
++ test-tool chmtime =-60 tracked &&
+ : >modified &&
++ test-tool chmtime =-60 modified &&
+ mkdir dir1 &&
+ : >dir1/tracked &&
++ test-tool chmtime =-60 dir1/tracked &&
+ : >dir1/modified &&
++ test-tool chmtime =-60 dir1/modified &&
+ mkdir dir2 &&
+ : >dir2/tracked &&
++ test-tool chmtime =-60 dir2/tracked &&
: >dir2/modified &&
++ test-tool chmtime =-60 dir2/modified &&
write_integration_script &&
git config core.fsmonitor .git/hooks/fsmonitor-test &&
-+ avoid_racy &&
git update-index --untracked-cache &&
- git update-index --fsmonitor &&
- GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace-before" \
2: d63faad03a4 ! 2: d29a68e65a0 In t7519, populate untracked cache before test
@@ Metadata
Author: Tao Klerks <[email protected]>
## Commit message ##
- In t7519, populate untracked cache before test
+ t7519: populate untracked cache before test
- In its current state, the t7519 test dealing with untracked
- cache assumes that
- "git update-index --untracked-cache" will *populate*
- the untracked cache. This is not correct - it will only add
- an empty untracked cache structure to the index.
+ In its current state, the t7519 test dealing with untracked cache
+ assumes that "git update-index --untracked-cache" will *populate* the
+ untracked cache. This is not correct - it will only add an empty
+ untracked cache structure to the index.
- If we're going to compare two git status runs with
- something interesting happening in-between, we
- need to ensure that the index is in a stable/steady
- state *before* that first run.
+ If we're going to compare two git status runs with something
+ interesting happening in-between, we need to ensure that the index is
+ in a stable/steady state *before* that first run.
- We achieve this by adding another prior "git status"
- run.
+ Achieve this by adding another prior "git status" run.
- At this stage this change does nothing, because there
- is a bug, addressed in the next patch. whereby once
- the empty untracked cache structure is added by the
- update-index invocation, the untracked cache gets
- updated in every subsequent "git status" call, but the
- index with these updates does not get written down.
+ At this stage this change does nothing, because there is a bug,
+ addressed in the next patch, whereby once the empty untracked cache
+ structure is added by the update-index invocation, the untracked cache
+ gets updated in every subsequent "git status" call, but the index with
+ these updates does not get written down.
- That bug actually invalidates this entire test case -
- but we're fixing that next.
+ That bug actually invalidates this entire test case - but we're fixing
+ that next.
## t/t7519-status-fsmonitor.sh ##
@@ t/t7519-status-fsmonitor.sh: test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'
- avoid_racy &&
+ git config core.fsmonitor .git/hooks/fsmonitor-test &&
git update-index --untracked-cache &&
git update-index --fsmonitor &&
+ git status &&
3: 627f1952fd8 ! 3: 190b27e518a Write index when populating empty untracked cache
@@ Metadata
Author: Tao Klerks <[email protected]>
## Commit message ##
- Write index when populating empty untracked cache
+ untracked-cache: write index when populating empty untracked cache
- It is expected that an empty/unpopulated untracked
- cache structure can be written to the index - by update-
- index, or by a "git status" call that sees the untracked cache
- should be enabled, but is running with options that make
- it non-applicable in that run.
+ It is expected that an empty/unpopulated untracked cache structure can
+ be written to the index - by update-index, or by a "git status" call
+ that sees the untracked cache should be enabled and is not, but is
+ running with options that make the untracked cache non-applicable in
+ that run (eg a pathspec).
- Currently, if that happens, then subsequent "git status"
- calls end up populating the untracked cache, but not
- writing the index (not saving their work) - so the
- performance outcome is almost identical to the cache
- being altogether disabled.
+ Currently, if that happens, then subsequent "git status" calls end up
+ populating the untracked cache, but not writing the index (not saving
+ their work) - so the performance outcome is almost identical to the
+ cache being altogether disabled.
- This continues until the index gets written with the cache
- populated, for some *other* reason.
+ This continues until the index gets written with the untracked cache
+ populated, for some *other* reason, such as a working tree change.
- In this change, we detect the "existing cache is empty
- and it looks like we are using it" condition, and queues
- an index write when this happens.
+ Detect the condition where an empty untracked cache exists in the
+ index and we will collect the list of untracked paths, and queue an
+ index write under that condition, so that the collected untracked
+ paths can be written out to the untracked cache extension in the
+ index.
- This change depends on previous fixes to t7519 for the
- "ignore .git changes when invalidating UNTR" test case to
- pass - before this fix, the test never actually did anything
- as it was not set up correctly.
+ This change depends on previous fixes to t7519 for the "ignore .git
+ changes when invalidating UNTR" test case to pass - before this fix,
+ the test never actually did anything as it was not set up correctly.
@@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_st
- if (!dir->untracked->root)
+ if (!dir->untracked->root) {
++ /* Untracked cache existed but is not initialized; fix that */
FLEX_ALLOC_STR(dir->untracked->root, name, "");
-+ /*
-+ * If we've had to initialize the root, then what we had was an
-+ * empty uninitialized untracked cache structure. We will be
-+ * populating it now, so we should trigger an index write.
-+ */
+ istate->cache_changed |= UNTRACKED_CHANGED;
+ }
Signature · 1 line
gitgitgadget
PATCH v2 1/3 t7519: avoid file to index mtime race for untracked cache
From: Tao Klerks <[email protected]>
In t7519 there is a test that writes files to disk, and immediately writes the untracked index. Because of mtime-comparison logic that uses a 1-second resolution, this means the cached entries are not trusted/used under some circumstances
(see read-cache.c#is_racy_stat()).
Untracked cache tests in t7063 use a 1-second delay to avoid this issue, but we don't want to introduce arbitrary slowdowns, so instead use test-tool chmtime to backdate the files slightly.
This change doesn't actually affect the outcome of the test, but does enhance its validity, and becomes relevant after later changes.
--- t/t7519-status-fsmonitor.sh | 6 ++++++ 1 file changed, 6 insertions(+)
t/t7519-status-fsmonitor.sh +6 -0
@@ -324,13 +324,19 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' cd dot-git && mkdir -p .git/hooks && : >tracked && + test-tool chmtime =-60 tracked && : >modified && + test-tool chmtime =-60 modified && mkdir dir1 && : >dir1/tracked && + test-tool chmtime =-60 dir1/tracked && : >dir1/modified && + test-tool chmtime =-60 dir1/modified && mkdir dir2 && : >dir2/tracked && + test-tool chmtime =-60 dir2/tracked && : >dir2/modified && + test-tool chmtime =-60 dir2/modified && write_integration_script && git config core.fsmonitor .git/hooks/fsmonitor-test && git update-index --untracked-cache &&
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/3] t7519: avoid file to index mtime race for untracked cache
Quoted 4 lines · Tao Klerks via GitGitGadget
From: Tao Klerks <[email protected]>
In t7519 there is a test that writes files to disk, and immediately writes the untracked index. Because of mtime-comparison logic that
"untracked cache", I think.
Quoted 7 lines
uses a 1-second resolution, this means the cached entries are not trusted/used under some circumstances
(see read-cache.c#is_racy_stat()).Untracked cache tests in t7063 use a 1-second delay to avoid this issue, but we don't want to introduce arbitrary slowdowns, so instead use test-tool chmtime to backdate the files slightly.
Good approach. Perhaps fixing 7063 can be marked as #leftoverbit?
Quoted 32 lines
This change doesn't actually affect the outcome of the test, but does enhance its validity, and becomes relevant after later changes.
Signed-off-by Tao Klerks <[email protected]>--- t/t7519-status-fsmonitor.sh | 6 ++++++ 1 file changed, 6 insertions(+)t/t7519-status-fsmonitor.sh +6
-0@@ -324,13 +324,19 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' cd dot-git && mkdir -p .git/hooks && : >tracked && + test-tool chmtime =-60 tracked && : >modified && + test-tool chmtime =-60 modified && mkdir dir1 && : >dir1/tracked && + test-tool chmtime =-60 dir1/tracked && : >dir1/modified && + test-tool chmtime =-60 dir1/modified && mkdir dir2 && : >dir2/tracked && + test-tool chmtime =-60 dir2/tracked && : >dir2/modified && + test-tool chmtime =-60 dir2/modified && write_integration_script && git config core.fsmonitor .git/hooks/fsmonitor-test && git update-index --untracked-cache &&
Re: [PATCH v2 1/3] t7519: avoid file to index mtime race for untracked cache
Quoted 8 lines · Junio C Hamano
Quoted 2 lines · Tao Klerks via GitGitGadget
In t7519 there is a test that writes files to disk, and immediately writes the untracked index. Because of mtime-comparison logic that
"untracked cache", I think.
Yep, fixed, thx.
Quoted 10 lines
Quoted 7 lines
uses a 1-second resolution, this means the cached entries are not trusted/used under some circumstances
(see read-cache.c#is_racy_stat()).Untracked cache tests in t7063 use a 1-second delay to avoid this issue, but we don't want to introduce arbitrary slowdowns, so instead use test-tool chmtime to backdate the files slightly.
Good approach. Perhaps fixing 7063 can be marked as #leftoverbit?
Sure. Updated the commit msg.
I also started another thread with what that looks like - it's not very pretty in the end because of Windows compatibility issues, and still incomplete because (as far as I can tell) the sleep is used for a couple of different purposes, and only one of them lends itself to chmtime manipulation.
PATCH v2 2/3 t7519: populate untracked cache before test
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
In its current state, the t7519 test dealing with untracked cache assumes that "git update-index --untracked-cache" will *populate* the untracked cache. This is not correct - it will only add an empty untracked cache structure to the index.
If we're going to compare two git status runs with something interesting happening in-between, we need to ensure that the index is in a stable/steady state *before* that first run.
Achieve this by adding another prior "git status" run.
At this stage this change does nothing, because there is a bug, addressed in the next patch, whereby once the empty untracked cache structure is added by the update-index invocation, the untracked cache gets updated in every subsequent "git status" call, but the index with these updates does not get written down.
That bug actually invalidates this entire test case - but we're fixing that next.
--- t/t7519-status-fsmonitor.sh | 1 + 1 file changed, 1 insertion(+)
t/t7519-status-fsmonitor.sh +1 -0
@@ -341,6 +341,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' git config core.fsmonitor .git/hooks/fsmonitor-test && git update-index --untracked-cache && git update-index --fsmonitor && + git status && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace-before" \ git status && test-tool dump-untracked-cache >../before
Signature · 1 line
gitgitgadget
PATCH v2 3/3 untracked-cache: write index when populating empty untracked cache
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
It is expected that an empty/unpopulated untracked cache structure can be written to the index - by update-index, or by a "git status" call that sees the untracked cache should be enabled and is not, but is running with options that make the untracked cache non-applicable in that run (eg a pathspec).
Currently, if that happens, then subsequent "git status" calls end up populating the untracked cache, but not writing the index (not saving their work) - so the performance outcome is almost identical to the cache being altogether disabled.
This continues until the index gets written with the untracked cache populated, for some *other* reason, such as a working tree change.
Detect the condition where an empty untracked cache exists in the index and we will collect the list of untracked paths, and queue an index write under that condition, so that the collected untracked paths can be written out to the untracked cache extension in the index.
This change depends on previous fixes to t7519 for the "ignore .git changes when invalidating UNTR" test case to pass - before this fix, the test never actually did anything as it was not set up correctly.
--- dir.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
dir.c +7 -3
@@ -2781,7 +2781,8 @@ void remove_untracked_cache(struct index_state *istate)static struct untracked_cache_dir *validate_untracked_cache (struct dir_struct *dir,int base_len, -const struct pathspec *pathspec) +const struct pathspec *pathspec, +struct index_state *istate) {struct untracked_cache_dir *root;static int untracked_cache_disabled =-1 ;
@@ -2845,8 +2846,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dreturn NULL ; } -if (!dir->untracked->root) +if (!dir->untracked->root) { +/* Untracked cache existed but is not initialized; fix that */ FLEX_ALLOC_STR (dir->untracked->root, name,"" ); + istate->cache_changed |=UNTRACKED_CHANGED ; + }/* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root;
@@ -2916,7 +2920,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,return dir->nr; } - untracked =validate_untracked_cache (dir, len, pathspec); + untracked =validate_untracked_cache (dir, len, pathspec, istate);if (!untracked)/* * make sure untracked cache code path is disabled,
Signature · 1 line
gitgitgadget
Re: [PATCH v2 3/3] untracked-cache: write index when populating empty untracked cache
Quoted 28 lines · Tao Klerks via GitGitGadget
dir.c +6
-2@@ -2781,7 +2781,8 @@ void remove_untracked_cache(struct index_state *istate)static struct untracked_cache_dir *validate_untracked_cache (struct dir_struct *dir,int base_len, -const struct pathspec *pathspec) +const struct pathspec *pathspec, +struct index_state *istate) {struct untracked_cache_dir *root;static int untracked_cache_disabled =-1 ;@@ -2845,8 +2846,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dreturn NULL ; } -if (!dir->untracked->root) +if (!dir->untracked->root) { +/* Untracked cache existed but is not initialized; fix that */ FLEX_ALLOC_STR (dir->untracked->root, name,"" ); + istate->cache_changed |=UNTRACKED_CHANGED ; + }/* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root;
dir.c int read_directory(struct dir_struct *dir, struct index_state *istate, · line 2926
Quoted 8 lines
dir.c +1
-1@@ -2916,7 +2920,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,return dir->nr; } - untracked =validate_untracked_cache (dir, len, pathspec); + untracked =validate_untracked_cache (dir, len, pathspec, istate);if (!untracked)/* * make sure untracked cache code path is disabled,
Makes sense. Will queue. Thanks.
v3 cover Empty untracked cache performance issue
In reply to Tao Klerks via GitGitGadgetThis patchset addresses a performance issue with untracked cache. When a new untracked cache structure is added to the index but remains empty, subsequent "git status" calls populate it but do not write the index - so they perform as though the index were disabled.
This situation can be caused in several different ways:
* Running "git update-index --untracked-cache" on a repo that did not have the untracked cache
* Modifying the git configuration to enable untracked cache, but then immediately running "git status -uall", causing the untracked cache to be created, but not used/populated (and the index written).
* (likely others)
The patchset includes fixes to t7519, which otherwise starts failing because it wasn't testing what it intended to.
Tao Klerks (3):
t7519: avoid file to index mtime race for untracked cache
t7519: populate untracked cache before test
untracked-cache: write index when populating empty untracked cache
dir.c | 10 +++++++--- t/t7519-status-fsmonitor.sh | 7 +++++++ 2 files changed, 14 insertions(+), 3 deletions(-)
base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-986%2FTaoK%2Ftaok-empty-untracked-cache-bug-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-986/TaoK/taok-empty-untracked-cache-bug-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/986
Range-diff vs v2:
1: 9421b71540d ! 1: 8b1f89c259e t7519: avoid file to index mtime race for untracked cache
@@ Commit message
t7519: avoid file to index mtime race for untracked cache
In t7519 there is a test that writes files to disk, and immediately
- writes the untracked index. Because of mtime-comparison logic that
- uses a 1-second resolution, this means the cached entries are not
- trusted/used under some circumstances
+ writes the index with the untracked cache. Because of
+ mtime-comparison logic that uses a 1-second resolution, this means
+ the cached entries are not trusted/used under some circumstances
(see read-cache.c#is_racy_stat()).
Untracked cache tests in t7063 use a 1-second delay to avoid this issue, but we don't want to introduce arbitrary slowdowns, so instead
- use test-tool chmtime to backdate the files slightly.
+ use test-tool chmtime to backdate the files slightly. The t7063
+ delays are a #leftoverbit, to be worked on in a separate series.
This change doesn't actually affect the outcome of the test, but does enhance its validity, and becomes relevant after later changes.
2: d29a68e65a0 = 2: c901f9d96ca t7519: populate untracked cache before test 3: 190b27e518a = 3: 9795a08414a untracked-cache: write index when populating empty untracked cache
Signature · 1 line
gitgitgadget
PATCH v3 1/3 t7519: avoid file to index mtime race for untracked cache
From: Tao Klerks <[email protected]>
In t7519 there is a test that writes files to disk, and immediately writes the index with the untracked cache. Because of mtime-comparison logic that uses a 1-second resolution, this means the cached entries are not trusted/used under some circumstances (see read-cache.c#is_racy_stat()).
Untracked cache tests in t7063 use a 1-second delay to avoid this issue, but we don't want to introduce arbitrary slowdowns, so instead use test-tool chmtime to backdate the files slightly. The t7063 delays are a #leftoverbit, to be worked on in a separate series.
This change doesn't actually affect the outcome of the test, but does enhance its validity, and becomes relevant after later changes.
--- t/t7519-status-fsmonitor.sh | 6 ++++++ 1 file changed, 6 insertions(+)
t/t7519-status-fsmonitor.sh +6 -0
@@ -324,13 +324,19 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' cd dot-git && mkdir -p .git/hooks && : >tracked && + test-tool chmtime =-60 tracked && : >modified && + test-tool chmtime =-60 modified && mkdir dir1 && : >dir1/tracked && + test-tool chmtime =-60 dir1/tracked && : >dir1/modified && + test-tool chmtime =-60 dir1/modified && mkdir dir2 && : >dir2/tracked && + test-tool chmtime =-60 dir2/tracked && : >dir2/modified && + test-tool chmtime =-60 dir2/modified && write_integration_script && git config core.fsmonitor .git/hooks/fsmonitor-test && git update-index --untracked-cache &&
Signature · 1 line
gitgitgadget
PATCH v3 2/3 t7519: populate untracked cache before test
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
In its current state, the t7519 test dealing with untracked cache assumes that "git update-index --untracked-cache" will *populate* the untracked cache. This is not correct - it will only add an empty untracked cache structure to the index.
If we're going to compare two git status runs with something interesting happening in-between, we need to ensure that the index is in a stable/steady state *before* that first run.
Achieve this by adding another prior "git status" run.
At this stage this change does nothing, because there is a bug, addressed in the next patch, whereby once the empty untracked cache structure is added by the update-index invocation, the untracked cache gets updated in every subsequent "git status" call, but the index with these updates does not get written down.
That bug actually invalidates this entire test case - but we're fixing that next.
--- t/t7519-status-fsmonitor.sh | 1 + 1 file changed, 1 insertion(+)
t/t7519-status-fsmonitor.sh +1 -0
@@ -341,6 +341,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR' git config core.fsmonitor .git/hooks/fsmonitor-test && git update-index --untracked-cache && git update-index --fsmonitor && + git status && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace-before" \ git status && test-tool dump-untracked-cache >../before
Signature · 1 line
gitgitgadget
PATCH v3 3/3 untracked-cache: write index when populating empty untracked cache
In reply to Tao Klerks via GitGitGadgetFrom: Tao Klerks <[email protected]>
It is expected that an empty/unpopulated untracked cache structure can be written to the index - by update-index, or by a "git status" call that sees the untracked cache should be enabled and is not, but is running with options that make the untracked cache non-applicable in that run (eg a pathspec).
Currently, if that happens, then subsequent "git status" calls end up populating the untracked cache, but not writing the index (not saving their work) - so the performance outcome is almost identical to the cache being altogether disabled.
This continues until the index gets written with the untracked cache populated, for some *other* reason, such as a working tree change.
Detect the condition where an empty untracked cache exists in the index and we will collect the list of untracked paths, and queue an index write under that condition, so that the collected untracked paths can be written out to the untracked cache extension in the index.
This change depends on previous fixes to t7519 for the "ignore .git changes when invalidating UNTR" test case to pass - before this fix, the test never actually did anything as it was not set up correctly.
--- dir.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
dir.c +7 -3
@@ -2781,7 +2781,8 @@ void remove_untracked_cache(struct index_state *istate)static struct untracked_cache_dir *validate_untracked_cache (struct dir_struct *dir,int base_len, -const struct pathspec *pathspec) +const struct pathspec *pathspec, +struct index_state *istate) {struct untracked_cache_dir *root;static int untracked_cache_disabled =-1 ;
@@ -2845,8 +2846,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dreturn NULL ; } -if (!dir->untracked->root) +if (!dir->untracked->root) { +/* Untracked cache existed but is not initialized; fix that */ FLEX_ALLOC_STR (dir->untracked->root, name,"" ); + istate->cache_changed |=UNTRACKED_CHANGED ; + }/* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root;
@@ -2916,7 +2920,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,return dir->nr; } - untracked =validate_untracked_cache (dir, len, pathspec); + untracked =validate_untracked_cache (dir, len, pathspec, istate);if (!untracked)/* * make sure untracked cache code path is disabled,
Signature · 1 line
gitgitgadget