From a67d178be4e604426152212c1c589b2e7f05e29f Mon Sep 17 00:00:00 2001 From: Tao Klerks Date: Sun, 27 Feb 2022 21:56:59 +0000 Subject: [PATCH 1/3] 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 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. Signed-off-by: Tao Klerks Signed-off-by: Junio C Hamano --- t/t7519-status-fsmonitor.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index a6308acf00..3f984136ea 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -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 && From 37482b4080b2e965cedfd85bab5fef2feb415992 Mon Sep 17 00:00:00 2001 From: Tao Klerks Date: Sun, 27 Feb 2022 21:57:00 +0000 Subject: [PATCH 2/3] 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. 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. Signed-off-by: Tao Klerks Signed-off-by: Junio C Hamano --- t/t7519-status-fsmonitor.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index 3f984136ea..fffc57120d 100755 --- a/t/t7519-status-fsmonitor.sh +++ b/t/t7519-status-fsmonitor.sh @@ -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 From 317956d91239e86b26ce95735451698b042dbe5d Mon Sep 17 00:00:00 2001 From: Tao Klerks Date: Sun, 27 Feb 2022 21:57:01 +0000 Subject: [PATCH 3/3] 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 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. Signed-off-by: Tao Klerks Signed-off-by: Junio C Hamano --- dir.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index d91295f2bc..4eee45dec9 100644 --- a/dir.c +++ b/dir.c @@ -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 *d return 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,