{"thread":{"id":"56002","subject":"[PATCH 0/3] Empty untracked cache performance issue","startedAt":"2021-06-24T18:30:11Z","lastAt":"2022-02-27T22:12:54Z","messageCount":19,"participants":["Tao Klerks via GitGitGadget","Junio C Hamano","Tao Klerks"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"428391","messageId":"pull.986.git.1624559401.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":null,"subject":"[PATCH 0/3] Empty untracked cache performance issue","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-24T18:29:58Z","receivedAt":"2021-06-24T18:30:11Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"This patchset addresses a performance issue with untracked cache. When a new\nuntracked cache structure is added to the index but remains empty,\nsubsequent \"git status\" calls populate it but do not write the index - so\nthey perform as though the index were disabled.\n\nThis situation can be caused in several different ways:\n\n * Running \"git update-index --untracked-cache\" on a repo that did not have\n the untracked cache\n * Modifying the git configuration to enable untracked cache, but then\n immediately running \"git status -uall\", causing the untracked cache to be\n created, but not used/populated (and the index written).\n * (likely others)\n\nThe patchset includes fixes to t7519, which otherwise starts failing because\nit wasn't testing what it intended to.\n\nTao Klerks (3):\n Add a second's delay to t7519 for untracked cache\n In t7519, populate untracked cache before test\n Write index when populating empty untracked cache\n\n dir.c | 14 +++++++++++---\n t/t7519-status-fsmonitor.sh | 6 ++++++\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\n\nbase-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-986%2FTaoK%2Ftaok-empty-untracked-cache-bug-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-986/TaoK/taok-empty-untracked-cache-bug-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/986\n-- \ngitgitgadget\n"},{"id":"428392","messageId":"c4d2a3cab4bd5243a776002e4f04fead7b1f34c5.1624559402.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.git.1624559401.gitgitgadget@gmail.com","subject":"[PATCH 1/3] Add a second's delay to t7519 for untracked cache","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-24T18:29:59Z","receivedAt":"2021-06-24T18:30:13Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIn t7519 there is a test that writes files to disk, and immediately\nwrites the untracked index. Because of mtime-comparison logic\nthat uses a 1-second resolution, this means the cached entries\nare not trusted/used under some circumstances (see\nread-cache.c#is_racy_stat()).\n\nUntracked cache tests in t7063 use a 1-second delay to avoid\nthis issue. We should do the same here.\n\nThis change doesn't actually affect the outcome of the test,\nbut does enhance its validity, and becomes relevant after\nlater changes\n\nSigned-off-by: Tao Klerks \n---\n t/t7519-status-fsmonitor.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 637391c6ce4..1209fa93499 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -13,6 +13,10 @@ clean_repo () {\n \tgit clean -fd\n }\n \n+avoid_racy() {\n+\tsleep 1\n+}\n+\n dirty_repo () {\n \t: >untracked &&\n \t: >dir1/untracked &&\n@@ -332,6 +336,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n \t\t: >dir2/modified &&\n \t\twrite_integration_script &&\n \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n+\t\tavoid_racy &&\n \t\tgit update-index --untracked-cache &&\n \t\tgit update-index --fsmonitor &&\n \t\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace-before\" \\\n-- \ngitgitgadget\n\n"},{"id":"428393","messageId":"d63faad03a4bc41ea778bc4dd61c320e09424cd9.1624559402.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.git.1624559401.gitgitgadget@gmail.com","subject":"[PATCH 2/3] In t7519, populate untracked cache before test","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-24T18:30:00Z","receivedAt":"2021-06-24T18:30:19Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIn its current state, the t7519 test dealing with untracked\ncache assumes that\n\"git update-index --untracked-cache\" will *populate*\nthe untracked cache. This is not correct - it will only add\nan empty untracked cache structure to the index.\n\nIf we're going to compare two git status runs with\nsomething interesting happening in-between, we\nneed to ensure that the index is in a stable/steady\nstate *before* that first run.\n\nWe achieve this by adding another prior \"git status\"\nrun.\n\nAt this stage this change does nothing, because there\nis a bug, addressed in the next patch. whereby once\nthe empty untracked cache structure is added by the\nupdate-index invocation, the untracked cache gets\nupdated in every subsequent \"git status\" call, but the\nindex with these updates does not get written down.\n\nThat bug actually invalidates this entire test case -\nbut we're fixing that next.\n\nSigned-off-by: Tao Klerks \n---\n t/t7519-status-fsmonitor.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 1209fa93499..ef75c548d90 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -339,6 +339,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n \t\tavoid_racy &&\n \t\tgit update-index --untracked-cache &&\n \t\tgit update-index --fsmonitor &&\n+\t\tgit status &&\n \t\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace-before\" \\\n \t\tgit status &&\n \t\ttest-tool dump-untracked-cache >../before\n-- \ngitgitgadget\n\n"},{"id":"428394","messageId":"627f1952fd8d4864b6b87f5539a9d9b802c5796b.1624559402.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.git.1624559401.gitgitgadget@gmail.com","subject":"[PATCH 3/3] Write index when populating empty untracked cache","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-24T18:30:01Z","receivedAt":"2021-06-24T18:30:20Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIt is expected that an empty/unpopulated untracked\ncache structure can be written to the index - by update-\nindex, or by a \"git status\" call that sees the untracked cache\nshould be enabled, but is running with options that make\nit non-applicable in that run.\n\nCurrently, if that happens, then subsequent \"git status\"\ncalls end up populating the untracked cache, but not\nwriting the index (not saving their work) - so the\nperformance outcome is almost identical to the cache\nbeing altogether disabled.\n\nThis continues until the index gets written with the cache\npopulated, for some *other* reason.\n\nIn this change, we detect the \"existing cache is empty\nand it looks like we are using it\" condition, and queues\nan index write when this happens.\n\nThis change depends on previous fixes to t7519 for the\n\"ignore .git changes when invalidating UNTR\" test case to\npass - before this fix, the test never actually did anything\nas it was not set up correctly.\n\nSigned-off-by: Tao Klerks \n---\n dir.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ebe5ec046e0..a326e40e1c1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2703,7 +2703,8 @@ void remove_untracked_cache(struct index_state *istate)\n \n static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n \t\t\t\t\t\t int base_len,\n-\t\t\t\t\t\t const struct pathspec *pathspec)\n+\t\t\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t\t\t struct index_state *istate)\n {\n \tstruct untracked_cache_dir *root;\n \tstatic int untracked_cache_disabled = -1;\n@@ -2767,8 +2768,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n-\tif (!dir->untracked->root)\n+\tif (!dir->untracked->root) {\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n+\t\t/*\n+\t\t * If we've had to initialize the root, then what we had was an\n+\t\t * empty uninitialized untracked cache structure. We will be\n+\t\t * populating it now, so we should trigger an index write.\n+\t\t */\n+\t\tistate->cache_changed |= UNTRACKED_CHANGED;\n+\t}\n \n \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n \troot = dir->untracked->root;\n@@ -2838,7 +2846,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\treturn dir->nr;\n \t}\n \n-\tuntracked = validate_untracked_cache(dir, len, pathspec);\n+\tuntracked = validate_untracked_cache(dir, len, pathspec, istate);\n \tif (!untracked)\n \t\t/*\n \t\t * make sure untracked cache code path is disabled,\n-- \ngitgitgadget\n"},{"id":"428711","messageId":"xmqq8s2t1er6.fsf@gitster.g","threadId":"56002","inReplyTo":"c4d2a3cab4bd5243a776002e4f04fead7b1f34c5.1624559402.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] Add a second's delay to t7519 for untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T04:22:37Z","receivedAt":"2021-06-29T04:22:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> From: Tao Klerks \n>\n> In t7519 there is a test that writes files to disk, and immediately\n> writes the untracked index. Because of mtime-comparison logic\n> that uses a 1-second resolution, this means the cached entries\n> are not trusted/used under some circumstances (see\n> read-cache.c#is_racy_stat()).\n>\n> Untracked cache tests in t7063 use a 1-second delay to avoid\n> this issue. We should do the same here.\n\nWe shouldn't waste wallclock time by slowing down tests, which\nalready take way too much time. If you can use \"test-tool chmtime\"\nto pretend as if the index file was written earlier than it actually\nis, that would be a better solution.\n\n> +avoid_racy() {\n\nStyle (see below).\n\n> +\tsleep 1\n> +}\n> +\n> dirty_repo () {\n"},{"id":"428712","messageId":"xmqq35t11dtu.fsf@gitster.g","threadId":"56002","inReplyTo":"627f1952fd8d4864b6b87f5539a9d9b802c5796b.1624559402.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] Write index when populating empty untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T04:42:37Z","receivedAt":"2021-06-29T04:42:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> Subject: Re: [PATCH 3/3] Write index when populating empty untracked cache\n\nCommon to all three patches. \n\nLook at \"git shortlog --no-merges -200 master\" and observe the\npattern, i.e. our commit title are prefix, a colon and then a\nsummary. Try to make your commits blend in.\n\n> From: Tao Klerks \n>\n> It is expected that an empty/unpopulated untracked\n> cache structure can be written to the index - by update-\n> index, or by a \"git status\" call that sees the untracked cache\n> should be enabled, but is running with options that make\n> it non-applicable in that run.\n\nWould an example be helpful? What requests the untracked cache to\nbe created, and what options (or perhaps the lack of what options)\nprevent the untracked cache to be \"non-applicable\"?\n\n> Currently, if that happens, then subsequent \"git status\"\n> calls end up populating the untracked cache, but not\n> writing the index (not saving their work) - so the\n> performance outcome is almost identical to the cache\n> being altogether disabled.\n>\n> This continues until the index gets written with the cache\n> populated, for some *other* reason.\n\nHere (and only here), the word \"cache\" is used instead of \"untracked\ncache\"---but I suspect that \"the cache\" here is the same \"untracked\ncache\". If that is the case, being consistent and spelling it out\nwould reduce the risk of confusing readers.\n\n> In this change, we detect the \"existing cache is empty\n> and it looks like we are using it\" condition, and queues\n> an index write when this happens.\n\n\"we detect ... and queues\" sounds like a grammo.\n\nIn this project, we describe the change being proposed as if we are\ngiving an order to the codebase to \"become like so\". So, perhaps\n\n Detect the condition where an empty untracked cache exists in\n the index and we collect the list of untracked paths, and queue\n an index write under that condition, so that the collected\n untracked paths can be written out to the untracked cache\n extension in the index.\n\nor something along the line.\n\n> This change depends on previous fixes to t7519 for the\n> \"ignore .git changes when invalidating UNTR\" test case to\n> pass - before this fix, the test never actually did anything\n> as it was not set up correctly.\n>\n> Signed-off-by: Tao Klerks \n> ---\n> dir.c | 14 +++++++++++---\n> 1 file changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index ebe5ec046e0..a326e40e1c1 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2703,7 +2703,8 @@ void remove_untracked_cache(struct index_state *istate)\n> \n> static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n> \t\t\t\t\t\t int base_len,\n> -\t\t\t\t\t\t const struct pathspec *pathspec)\n> +\t\t\t\t\t\t const struct pathspec *pathspec,\n> +\t\t\t\t\t\t struct index_state *istate)\n> {\n> \tstruct untracked_cache_dir *root;\n> \tstatic int untracked_cache_disabled = -1;\n> @@ -2767,8 +2768,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> \t\treturn NULL;\n> \t}\n> \n> -\tif (!dir->untracked->root)\n> +\tif (!dir->untracked->root) {\n> \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n> +\t\t/*\n> +\t\t * If we've had to initialize the root, then what we had was an\n> +\t\t * empty uninitialized untracked cache structure. We will be\n> +\t\t * populating it now, so we should trigger an index write.\n> +\t\t */\n> +\t\tistate->cache_changed |= UNTRACKED_CHANGED;\n> +\t}\n\nThe logic sounds fairly straight-forward. The fact that we came\nthis far in the helper function means that we know we want to use\n(i.e. untracked cache is not disabled, and various other \"we\nshouldn't be contaminating the cache with the one-shot information\"\ncases did not return from the helper) untracked cache, and root\nbeing NULL originally means the untracked cache wasn't populated.\n\n> \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n> \troot = dir->untracked->root;\n> @@ -2838,7 +2846,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n> \t\treturn dir->nr;\n> \t}\n> \n> -\tuntracked = validate_untracked_cache(dir, len, pathspec);\n> +\tuntracked = validate_untracked_cache(dir, len, pathspec, istate);\n> \tif (!untracked)\n> \t\t/*\n> \t\t * make sure untracked cache code path is disabled,\n"},{"id":"449485","messageId":"CAPMMpogerttWdjGBNxJaqHT4bd3_igDx4_Fxev2eNHqexZ=aLQ@mail.gmail.com","threadId":"56002","inReplyTo":"xmqq35t11dtu.fsf@gitster.g","subject":"Re: [PATCH 3/3] Write index when populating empty untracked cache","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-02-24T17:52:27Z","receivedAt":"2022-02-24T17:52:42Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Hi Junio,\n\nThanks so much for reviewing this, and my apologies for not reacting\nearlier, after you took the time.\n\n> our commit title are prefix, a colon and then a\n> summary. Try to make your commits blend in.\n\nNoted, will fix, thx. I believe I did this better in an unrelated\npatch series today, but I guess I will see what people say :)\n\n> What requests the untracked cache to\n> be created, and what options (or perhaps the lack of what options)\n> prevent the untracked cache to be \"non-applicable\"?\n\nWill try to clarify / be more specific.\n\n> I suspect that \"the cache\" here is the same \"untracked\n> cache\"\n\nWill fix.\n\n> \"we detect ... and queues\" sounds like a grammo.\n\nYep, dreaded perspective shift, will fix.\n\n> perhaps [proposed text] or something along the line.\n\nWill incorporate, thx.\n\n> The logic sounds fairly straight-forward.\n\nI didn't understand here whether you were confirming that the change\nseems to make sense (yay!), or commenting that the extra comment block\nis redundant, stating something obvious, and should better be removed.\nCould you confirm please?\n\nIn the meantime, I'll \"re-roll\" (?) with the proposed changes.\n\nThanks again,\nTao\n\nOn Tue, Jun 29, 2021 at 6:42 AM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > Subject: Re: [PATCH 3/3] Write index when populating empty untracked cache\n>\n> Common to all three patches.\n>\n> Look at \"git shortlog --no-merges -200 master\" and observe the\n> pattern, i.e. our commit title are prefix, a colon and then a\n> summary. Try to make your commits blend in.\n>\n> > From: Tao Klerks \n> >\n> > It is expected that an empty/unpopulated untracked\n> > cache structure can be written to the index - by update-\n> > index, or by a \"git status\" call that sees the untracked cache\n> > should be enabled, but is running with options that make\n> > it non-applicable in that run.\n>\n> Would an example be helpful? What requests the untracked cache to\n> be created, and what options (or perhaps the lack of what options)\n> prevent the untracked cache to be \"non-applicable\"?\n>\n> > Currently, if that happens, then subsequent \"git status\"\n> > calls end up populating the untracked cache, but not\n> > writing the index (not saving their work) - so the\n> > performance outcome is almost identical to the cache\n> > being altogether disabled.\n> >\n> > This continues until the index gets written with the cache\n> > populated, for some *other* reason.\n>\n> Here (and only here), the word \"cache\" is used instead of \"untracked\n> cache\"---but I suspect that \"the cache\" here is the same \"untracked\n> cache\". If that is the case, being consistent and spelling it out\n> would reduce the risk of confusing readers.\n>\n> > In this change, we detect the \"existing cache is empty\n> > and it looks like we are using it\" condition, and queues\n> > an index write when this happens.\n>\n> \"we detect ... and queues\" sounds like a grammo.\n>\n> In this project, we describe the change being proposed as if we are\n> giving an order to the codebase to \"become like so\". So, perhaps\n>\n> Detect the condition where an empty untracked cache exists in\n> the index and we collect the list of untracked paths, and queue\n> an index write under that condition, so that the collected\n> untracked paths can be written out to the untracked cache\n> extension in the index.\n>\n> or something along the line.\n>\n> > This change depends on previous fixes to t7519 for the\n> > \"ignore .git changes when invalidating UNTR\" test case to\n> > pass - before this fix, the test never actually did anything\n> > as it was not set up correctly.\n> >\n> > Signed-off-by: Tao Klerks \n> > ---\n> > dir.c | 14 +++++++++++---\n> > 1 file changed, 11 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/dir.c b/dir.c\n> > index ebe5ec046e0..a326e40e1c1 100644\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -2703,7 +2703,8 @@ void remove_untracked_cache(struct index_state *istate)\n> >\n> > static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n> > int base_len,\n> > - const struct pathspec *pathspec)\n> > + const struct pathspec *pathspec,\n> > + struct index_state *istate)\n> > {\n> > struct untracked_cache_dir *root;\n> > static int untracked_cache_disabled = -1;\n> > @@ -2767,8 +2768,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> > return NULL;\n> > }\n> >\n> > - if (!dir->untracked->root)\n> > + if (!dir->untracked->root) {\n> > FLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n> > + /*\n> > + * If we've had to initialize the root, then what we had was an\n> > + * empty uninitialized untracked cache structure. We will be\n> > + * populating it now, so we should trigger an index write.\n> > + */\n> > + istate->cache_changed |= UNTRACKED_CHANGED;\n> > + }\n>\n> The logic sounds fairly straight-forward. The fact that we came\n> this far in the helper function means that we know we want to use\n> (i.e. untracked cache is not disabled, and various other \"we\n> shouldn't be contaminating the cache with the one-shot information\"\n> cases did not return from the helper) untracked cache, and root\n> being NULL originally means the untracked cache wasn't populated.\n>\n> > /* Validate $GIT_DIR/info/exclude and core.excludesfile */\n> > root = dir->untracked->root;\n> > @@ -2838,7 +2846,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n> > return dir->nr;\n> > }\n> >\n> > - untracked = validate_untracked_cache(dir, len, pathspec);\n> > + untracked = validate_untracked_cache(dir, len, pathspec, istate);\n> > if (!untracked)\n> > /*\n> > * make sure untracked cache code path is disabled,\n"},{"id":"449503","messageId":"xmqq5yp4knpa.fsf@gitster.g","threadId":"56002","inReplyTo":"CAPMMpogerttWdjGBNxJaqHT4bd3_igDx4_Fxev2eNHqexZ=aLQ@mail.gmail.com","subject":"Re: [PATCH 3/3] Write index when populating empty untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-24T20:35:13Z","receivedAt":"2022-02-24T20:35:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks writes:\n\n>> The logic sounds fairly straight-forward.\n>\n> I didn't understand here whether you were confirming that the change\n> seems to make sense (yay!), or commenting that the extra comment block\n> is redundant, stating something obvious, and should better be removed.\n> Could you confirm please?\n\nI meant the former when I wrote it.\n\nBut now you made me re-read the patch, I am becoming slightly\nsympathetic to the \"do we even need to comment?\" interpretation, too\n;-)\n\nThe question is if the comment to these two statements is redundant.\n\n\tif (!dir->untracked->root) {\n\t\t/*\n\t\t * If we've had to initialize the root, then what we had was an\n\t\t * empty uninitialized untracked cache structure. We will be\n\t\t * populating it now, so we should trigger an index write.\n\t\t */\n\t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n\t\tistate->cache_changed |= UNTRACKED_CHANGED;\n\t}\n\nI can be pursuaded either way.\n"},{"id":"449604","messageId":"9421b71540d1f1764db6931d0781576d8a710866.1645809015.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.v2.git.1645809015.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t7519: avoid file to index mtime race for untracked cache","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T17:10:12Z","receivedAt":"2022-02-25T17:10:20Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIn t7519 there is a test that writes files to disk, and immediately\nwrites the untracked index. Because of mtime-comparison logic that\nuses a 1-second resolution, this means the cached entries are not\ntrusted/used under some circumstances\n(see read-cache.c#is_racy_stat()).\n\nUntracked cache tests in t7063 use a 1-second delay to avoid this\nissue, but we don't want to introduce arbitrary slowdowns, so instead\nuse test-tool chmtime to backdate the files slightly.\n\nThis change doesn't actually affect the outcome of the test, but does\nenhance its validity, and becomes relevant after later changes.\n\nSigned-off-by: Tao Klerks \n---\n t/t7519-status-fsmonitor.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex a6308acf006..3f984136ea9 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -324,13 +324,19 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n \t\tcd dot-git &&\n \t\tmkdir -p .git/hooks &&\n \t\t: >tracked &&\n+\t\ttest-tool chmtime =-60 tracked &&\n \t\t: >modified &&\n+\t\ttest-tool chmtime =-60 modified &&\n \t\tmkdir dir1 &&\n \t\t: >dir1/tracked &&\n+\t\ttest-tool chmtime =-60 dir1/tracked &&\n \t\t: >dir1/modified &&\n+\t\ttest-tool chmtime =-60 dir1/modified &&\n \t\tmkdir dir2 &&\n \t\t: >dir2/tracked &&\n+\t\ttest-tool chmtime =-60 dir2/tracked &&\n \t\t: >dir2/modified &&\n+\t\ttest-tool chmtime =-60 dir2/modified &&\n \t\twrite_integration_script &&\n \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n \t\tgit update-index --untracked-cache &&\n-- \ngitgitgadget\n\n"},{"id":"449605","messageId":"pull.986.v2.git.1645809015.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.git.1624559401.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Empty untracked cache performance issue","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T17:10:11Z","receivedAt":"2022-02-25T17:10:22Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"This patchset addresses a performance issue with untracked cache. When a new\nuntracked cache structure is added to the index but remains empty,\nsubsequent \"git status\" calls populate it but do not write the index - so\nthey perform as though the index were disabled.\n\nThis situation can be caused in several different ways:\n\n * Running \"git update-index --untracked-cache\" on a repo that did not have\n the untracked cache\n * Modifying the git configuration to enable untracked cache, but then\n immediately running \"git status -uall\", causing the untracked cache to be\n created, but not used/populated (and the index written).\n * (likely others)\n\nThe patchset includes fixes to t7519, which otherwise starts failing because\nit wasn't testing what it intended to.\n\nTao Klerks (3):\n t7519: avoid file to index mtime race for untracked cache\n t7519: populate untracked cache before test\n untracked-cache: write index when populating empty untracked cache\n\n dir.c | 10 +++++++---\n t/t7519-status-fsmonitor.sh | 7 +++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-986%2FTaoK%2Ftaok-empty-untracked-cache-bug-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-986/TaoK/taok-empty-untracked-cache-bug-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/986\n\nRange-diff vs v1:\n\n 1: c4d2a3cab4b ! 1: 9421b71540d Add a second's delay to t7519 for untracked cache\n @@ Metadata\n Author: Tao Klerks \n \n ## Commit message ##\n - Add a second's delay to t7519 for untracked cache\n + t7519: avoid file to index mtime race for untracked cache\n \n In t7519 there is a test that writes files to disk, and immediately\n - writes the untracked index. Because of mtime-comparison logic\n - that uses a 1-second resolution, this means the cached entries\n - are not trusted/used under some circumstances (see\n - read-cache.c#is_racy_stat()).\n + writes the untracked index. Because of mtime-comparison logic that\n + uses a 1-second resolution, this means the cached entries are not\n + trusted/used under some circumstances\n + (see read-cache.c#is_racy_stat()).\n \n - Untracked cache tests in t7063 use a 1-second delay to avoid\n - this issue. We should do the same here.\n + Untracked cache tests in t7063 use a 1-second delay to avoid this\n + issue, but we don't want to introduce arbitrary slowdowns, so instead\n + use test-tool chmtime to backdate the files slightly.\n \n - This change doesn't actually affect the outcome of the test,\n - but does enhance its validity, and becomes relevant after\n - later changes\n + This change doesn't actually affect the outcome of the test, but does\n + enhance its validity, and becomes relevant after later changes.\n \n Signed-off-by: Tao Klerks \n \n ## t/t7519-status-fsmonitor.sh ##\n -@@ t/t7519-status-fsmonitor.sh: clean_repo () {\n - \tgit clean -fd\n - }\n - \n -+avoid_racy() {\n -+\tsleep 1\n -+}\n -+\n - dirty_repo () {\n - \t: >untracked &&\n - \t: >dir1/untracked &&\n @@ t/t7519-status-fsmonitor.sh: test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n + \t\tcd dot-git &&\n + \t\tmkdir -p .git/hooks &&\n + \t\t: >tracked &&\n ++\t\ttest-tool chmtime =-60 tracked &&\n + \t\t: >modified &&\n ++\t\ttest-tool chmtime =-60 modified &&\n + \t\tmkdir dir1 &&\n + \t\t: >dir1/tracked &&\n ++\t\ttest-tool chmtime =-60 dir1/tracked &&\n + \t\t: >dir1/modified &&\n ++\t\ttest-tool chmtime =-60 dir1/modified &&\n + \t\tmkdir dir2 &&\n + \t\t: >dir2/tracked &&\n ++\t\ttest-tool chmtime =-60 dir2/tracked &&\n \t\t: >dir2/modified &&\n ++\t\ttest-tool chmtime =-60 dir2/modified &&\n \t\twrite_integration_script &&\n \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n -+\t\tavoid_racy &&\n \t\tgit update-index --untracked-cache &&\n - \t\tgit update-index --fsmonitor &&\n - \t\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace-before\" \\\n 2: d63faad03a4 ! 2: d29a68e65a0 In t7519, populate untracked cache before test\n @@ Metadata\n Author: Tao Klerks \n \n ## Commit message ##\n - In t7519, populate untracked cache before test\n + t7519: populate untracked cache before test\n \n - In its current state, the t7519 test dealing with untracked\n - cache assumes that\n - \"git update-index --untracked-cache\" will *populate*\n - the untracked cache. This is not correct - it will only add\n - an empty untracked cache structure to the index.\n + In its current state, the t7519 test dealing with untracked cache\n + assumes that \"git update-index --untracked-cache\" will *populate* the\n + untracked cache. This is not correct - it will only add an empty\n + untracked cache structure to the index.\n \n - If we're going to compare two git status runs with\n - something interesting happening in-between, we\n - need to ensure that the index is in a stable/steady\n - state *before* that first run.\n + If we're going to compare two git status runs with something\n + interesting happening in-between, we need to ensure that the index is\n + in a stable/steady state *before* that first run.\n \n - We achieve this by adding another prior \"git status\"\n - run.\n + Achieve this by adding another prior \"git status\" run.\n \n - At this stage this change does nothing, because there\n - is a bug, addressed in the next patch. whereby once\n - the empty untracked cache structure is added by the\n - update-index invocation, the untracked cache gets\n - updated in every subsequent \"git status\" call, but the\n - index with these updates does not get written down.\n + At this stage this change does nothing, because there is a bug,\n + addressed in the next patch, whereby once the empty untracked cache\n + structure is added by the update-index invocation, the untracked cache\n + gets updated in every subsequent \"git status\" call, but the index with\n + these updates does not get written down.\n \n - That bug actually invalidates this entire test case -\n - but we're fixing that next.\n + That bug actually invalidates this entire test case - but we're fixing\n + that next.\n \n Signed-off-by: Tao Klerks \n \n ## t/t7519-status-fsmonitor.sh ##\n @@ t/t7519-status-fsmonitor.sh: test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n - \t\tavoid_racy &&\n + \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n \t\tgit update-index --untracked-cache &&\n \t\tgit update-index --fsmonitor &&\n +\t\tgit status &&\n 3: 627f1952fd8 ! 3: 190b27e518a Write index when populating empty untracked cache\n @@ Metadata\n Author: Tao Klerks \n \n ## Commit message ##\n - Write index when populating empty untracked cache\n + untracked-cache: write index when populating empty untracked cache\n \n - It is expected that an empty/unpopulated untracked\n - cache structure can be written to the index - by update-\n - index, or by a \"git status\" call that sees the untracked cache\n - should be enabled, but is running with options that make\n - it non-applicable in that run.\n + It is expected that an empty/unpopulated untracked cache structure can\n + be written to the index - by update-index, or by a \"git status\" call\n + that sees the untracked cache should be enabled and is not, but is\n + running with options that make the untracked cache non-applicable in\n + that run (eg a pathspec).\n \n - Currently, if that happens, then subsequent \"git status\"\n - calls end up populating the untracked cache, but not\n - writing the index (not saving their work) - so the\n - performance outcome is almost identical to the cache\n - being altogether disabled.\n + Currently, if that happens, then subsequent \"git status\" calls end up\n + populating the untracked cache, but not writing the index (not saving\n + their work) - so the performance outcome is almost identical to the\n + cache being altogether disabled.\n \n - This continues until the index gets written with the cache\n - populated, for some *other* reason.\n + This continues until the index gets written with the untracked cache\n + populated, for some *other* reason, such as a working tree change.\n \n - In this change, we detect the \"existing cache is empty\n - and it looks like we are using it\" condition, and queues\n - an index write when this happens.\n + Detect the condition where an empty untracked cache exists in the\n + index and we will collect the list of untracked paths, and queue an\n + index write under that condition, so that the collected untracked\n + paths can be written out to the untracked cache extension in the\n + index.\n \n - This change depends on previous fixes to t7519 for the\n - \"ignore .git changes when invalidating UNTR\" test case to\n - pass - before this fix, the test never actually did anything\n - as it was not set up correctly.\n + This change depends on previous fixes to t7519 for the \"ignore .git\n + changes when invalidating UNTR\" test case to pass - before this fix,\n + the test never actually did anything as it was not set up correctly.\n \n Signed-off-by: Tao Klerks \n \n @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_st\n \n -\tif (!dir->untracked->root)\n +\tif (!dir->untracked->root) {\n ++\t\t/* Untracked cache existed but is not initialized; fix that */\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n -+\t\t/*\n -+\t\t * If we've had to initialize the root, then what we had was an\n -+\t\t * empty uninitialized untracked cache structure. We will be\n -+\t\t * populating it now, so we should trigger an index write.\n -+\t\t */\n +\t\tistate->cache_changed |= UNTRACKED_CHANGED;\n +\t}\n \n\n-- \ngitgitgadget\n"},{"id":"449606","messageId":"d29a68e65a0f5d37460175d9a4276fb2e1810dab.1645809015.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.v2.git.1645809015.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] t7519: populate untracked cache before test","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T17:10:13Z","receivedAt":"2022-02-25T17:10:23Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIn its current state, the t7519 test dealing with untracked cache\nassumes that \"git update-index --untracked-cache\" will *populate* the\nuntracked cache. This is not correct - it will only add an empty\nuntracked cache structure to the index.\n\nIf we're going to compare two git status runs with something\ninteresting happening in-between, we need to ensure that the index is\nin a stable/steady state *before* that first run.\n\nAchieve this by adding another prior \"git status\" run.\n\nAt this stage this change does nothing, because there is a bug,\naddressed in the next patch, whereby once the empty untracked cache\nstructure is added by the update-index invocation, the untracked cache\ngets updated in every subsequent \"git status\" call, but the index with\nthese updates does not get written down.\n\nThat bug actually invalidates this entire test case - but we're fixing\nthat next.\n\nSigned-off-by: Tao Klerks \n---\n t/t7519-status-fsmonitor.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 3f984136ea9..fffc57120d6 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -341,6 +341,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n \t\tgit update-index --untracked-cache &&\n \t\tgit update-index --fsmonitor &&\n+\t\tgit status &&\n \t\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace-before\" \\\n \t\tgit status &&\n \t\ttest-tool dump-untracked-cache >../before\n-- \ngitgitgadget\n\n"},{"id":"449607","messageId":"190b27e518a22e2253b318e8087446d761e918b2.1645809015.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.v2.git.1645809015.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] untracked-cache: write index when populating empty untracked cache","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T17:10:14Z","receivedAt":"2022-02-25T17:10:26Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIt is expected that an empty/unpopulated untracked cache structure can\nbe written to the index - by update-index, or by a \"git status\" call\nthat sees the untracked cache should be enabled and is not, but is\nrunning with options that make the untracked cache non-applicable in\nthat run (eg a pathspec).\n\nCurrently, if that happens, then subsequent \"git status\" calls end up\npopulating the untracked cache, but not writing the index (not saving\ntheir work) - so the performance outcome is almost identical to the\ncache being altogether disabled.\n\nThis continues until the index gets written with the untracked cache\npopulated, for some *other* reason, such as a working tree change.\n\nDetect the condition where an empty untracked cache exists in the\nindex and we will collect the list of untracked paths, and queue an\nindex write under that condition, so that the collected untracked\npaths can be written out to the untracked cache extension in the\nindex.\n\nThis change depends on previous fixes to t7519 for the \"ignore .git\nchanges when invalidating UNTR\" test case to pass - before this fix,\nthe test never actually did anything as it was not set up correctly.\n\nSigned-off-by: Tao Klerks \n---\n dir.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d91295f2bcd..4eee45dec91 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2781,7 +2781,8 @@ void remove_untracked_cache(struct index_state *istate)\n \n static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n \t\t\t\t\t\t int base_len,\n-\t\t\t\t\t\t const struct pathspec *pathspec)\n+\t\t\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t\t\t struct index_state *istate)\n {\n \tstruct untracked_cache_dir *root;\n \tstatic int untracked_cache_disabled = -1;\n@@ -2845,8 +2846,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n-\tif (!dir->untracked->root)\n+\tif (!dir->untracked->root) {\n+\t\t/* Untracked cache existed but is not initialized; fix that */\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n+\t\tistate->cache_changed |= UNTRACKED_CHANGED;\n+\t}\n \n \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n \troot = dir->untracked->root;\n@@ -2916,7 +2920,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\treturn dir->nr;\n \t}\n \n-\tuntracked = validate_untracked_cache(dir, len, pathspec);\n+\tuntracked = validate_untracked_cache(dir, len, pathspec, istate);\n \tif (!untracked)\n \t\t/*\n \t\t * make sure untracked cache code path is disabled,\n-- \ngitgitgadget\n"},{"id":"449625","messageId":"xmqqr17qbw8l.fsf@gitster.g","threadId":"56002","inReplyTo":"9421b71540d1f1764db6931d0781576d8a710866.1645809015.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] t7519: avoid file to index mtime race for untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T19:07:54Z","receivedAt":"2022-02-25T19:08:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> From: Tao Klerks \n>\n> In t7519 there is a test that writes files to disk, and immediately\n> writes the untracked index. Because of mtime-comparison logic that\n\n\"untracked cache\", I think.\n\n> uses a 1-second resolution, this means the cached entries are not\n> trusted/used under some circumstances\n> (see read-cache.c#is_racy_stat()).\n>\n> Untracked cache tests in t7063 use a 1-second delay to avoid this\n> issue, but we don't want to introduce arbitrary slowdowns, so instead\n> use test-tool chmtime to backdate the files slightly.\n\nGood approach. Perhaps fixing 7063 can be marked as #leftoverbit?\n\n> This change doesn't actually affect the outcome of the test, but does\n> enhance its validity, and becomes relevant after later changes.\n>\n> Signed-off-by: Tao Klerks \n> ---\n> t/t7519-status-fsmonitor.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index a6308acf006..3f984136ea9 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -324,13 +324,19 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n> \t\tcd dot-git &&\n> \t\tmkdir -p .git/hooks &&\n> \t\t: >tracked &&\n> +\t\ttest-tool chmtime =-60 tracked &&\n> \t\t: >modified &&\n> +\t\ttest-tool chmtime =-60 modified &&\n> \t\tmkdir dir1 &&\n> \t\t: >dir1/tracked &&\n> +\t\ttest-tool chmtime =-60 dir1/tracked &&\n> \t\t: >dir1/modified &&\n> +\t\ttest-tool chmtime =-60 dir1/modified &&\n> \t\tmkdir dir2 &&\n> \t\t: >dir2/tracked &&\n> +\t\ttest-tool chmtime =-60 dir2/tracked &&\n> \t\t: >dir2/modified &&\n> +\t\ttest-tool chmtime =-60 dir2/modified &&\n> \t\twrite_integration_script &&\n> \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n> \t\tgit update-index --untracked-cache &&\n"},{"id":"449626","messageId":"xmqqmtiebw09.fsf@gitster.g","threadId":"56002","inReplyTo":"190b27e518a22e2253b318e8087446d761e918b2.1645809015.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] untracked-cache: write index when populating empty untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T19:12:54Z","receivedAt":"2022-02-25T19:13:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> diff --git a/dir.c b/dir.c\n> index d91295f2bcd..4eee45dec91 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2781,7 +2781,8 @@ void remove_untracked_cache(struct index_state *istate)\n> \n> static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n> \t\t\t\t\t\t int base_len,\n> -\t\t\t\t\t\t const struct pathspec *pathspec)\n> +\t\t\t\t\t\t const struct pathspec *pathspec,\n> +\t\t\t\t\t\t struct index_state *istate)\n> {\n> \tstruct untracked_cache_dir *root;\n> \tstatic int untracked_cache_disabled = -1;\n> @@ -2845,8 +2846,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> \t\treturn NULL;\n> \t}\n> \n> -\tif (!dir->untracked->root)\n> +\tif (!dir->untracked->root) {\n> +\t\t/* Untracked cache existed but is not initialized; fix that */\n> \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n> +\t\tistate->cache_changed |= UNTRACKED_CHANGED;\n> +\t}\n> \n> \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n> \troot = dir->untracked->root;\n> @@ -2916,7 +2920,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n> \t\treturn dir->nr;\n> \t}\n> \n> -\tuntracked = validate_untracked_cache(dir, len, pathspec);\n> +\tuntracked = validate_untracked_cache(dir, len, pathspec, istate);\n> \tif (!untracked)\n> \t\t/*\n> \t\t * make sure untracked cache code path is disabled,\n\nMakes sense. Will queue. Thanks.\n"},{"id":"449713","messageId":"8b1f89c259e46f5b09f24e1e94614136365e4f69.1645999021.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.v3.git.1645999021.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] t7519: avoid file to index mtime race for untracked cache","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-27T21:56:59Z","receivedAt":"2022-02-27T21:57:09Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIn t7519 there is a test that writes files to disk, and immediately\nwrites the index with the untracked cache. Because of\nmtime-comparison logic that uses a 1-second resolution, this means\nthe cached entries are not trusted/used under some circumstances\n(see read-cache.c#is_racy_stat()).\n\nUntracked cache tests in t7063 use a 1-second delay to avoid this\nissue, but we don't want to introduce arbitrary slowdowns, so instead\nuse test-tool chmtime to backdate the files slightly. The t7063\ndelays are a #leftoverbit, to be worked on in a separate series.\n\nThis change doesn't actually affect the outcome of the test, but does\nenhance its validity, and becomes relevant after later changes.\n\nSigned-off-by: Tao Klerks \n---\n t/t7519-status-fsmonitor.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex a6308acf006..3f984136ea9 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -324,13 +324,19 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n \t\tcd dot-git &&\n \t\tmkdir -p .git/hooks &&\n \t\t: >tracked &&\n+\t\ttest-tool chmtime =-60 tracked &&\n \t\t: >modified &&\n+\t\ttest-tool chmtime =-60 modified &&\n \t\tmkdir dir1 &&\n \t\t: >dir1/tracked &&\n+\t\ttest-tool chmtime =-60 dir1/tracked &&\n \t\t: >dir1/modified &&\n+\t\ttest-tool chmtime =-60 dir1/modified &&\n \t\tmkdir dir2 &&\n \t\t: >dir2/tracked &&\n+\t\ttest-tool chmtime =-60 dir2/tracked &&\n \t\t: >dir2/modified &&\n+\t\ttest-tool chmtime =-60 dir2/modified &&\n \t\twrite_integration_script &&\n \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n \t\tgit update-index --untracked-cache &&\n-- \ngitgitgadget\n\n"},{"id":"449714","messageId":"pull.986.v3.git.1645999021.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.v2.git.1645809015.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Empty untracked cache performance issue","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-27T21:56:58Z","receivedAt":"2022-02-27T21:57:10Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"This patchset addresses a performance issue with untracked cache. When a new\nuntracked cache structure is added to the index but remains empty,\nsubsequent \"git status\" calls populate it but do not write the index - so\nthey perform as though the index were disabled.\n\nThis situation can be caused in several different ways:\n\n * Running \"git update-index --untracked-cache\" on a repo that did not have\n the untracked cache\n * Modifying the git configuration to enable untracked cache, but then\n immediately running \"git status -uall\", causing the untracked cache to be\n created, but not used/populated (and the index written).\n * (likely others)\n\nThe patchset includes fixes to t7519, which otherwise starts failing because\nit wasn't testing what it intended to.\n\nTao Klerks (3):\n t7519: avoid file to index mtime race for untracked cache\n t7519: populate untracked cache before test\n untracked-cache: write index when populating empty untracked cache\n\n dir.c | 10 +++++++---\n t/t7519-status-fsmonitor.sh | 7 +++++++\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-986%2FTaoK%2Ftaok-empty-untracked-cache-bug-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-986/TaoK/taok-empty-untracked-cache-bug-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/986\n\nRange-diff vs v2:\n\n 1: 9421b71540d ! 1: 8b1f89c259e t7519: avoid file to index mtime race for untracked cache\n @@ Commit message\n t7519: avoid file to index mtime race for untracked cache\n \n In t7519 there is a test that writes files to disk, and immediately\n - writes the untracked index. Because of mtime-comparison logic that\n - uses a 1-second resolution, this means the cached entries are not\n - trusted/used under some circumstances\n + writes the index with the untracked cache. Because of\n + mtime-comparison logic that uses a 1-second resolution, this means\n + the cached entries are not trusted/used under some circumstances\n (see read-cache.c#is_racy_stat()).\n \n Untracked cache tests in t7063 use a 1-second delay to avoid this\n issue, but we don't want to introduce arbitrary slowdowns, so instead\n - use test-tool chmtime to backdate the files slightly.\n + use test-tool chmtime to backdate the files slightly. The t7063\n + delays are a #leftoverbit, to be worked on in a separate series.\n \n This change doesn't actually affect the outcome of the test, but does\n enhance its validity, and becomes relevant after later changes.\n 2: d29a68e65a0 = 2: c901f9d96ca t7519: populate untracked cache before test\n 3: 190b27e518a = 3: 9795a08414a untracked-cache: write index when populating empty untracked cache\n\n-- \ngitgitgadget\n"},{"id":"449715","messageId":"c901f9d96ca35da24aa36e1978f6ec27981d0e44.1645999021.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.v3.git.1645999021.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] t7519: populate untracked cache before test","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-27T21:57:00Z","receivedAt":"2022-02-27T21:57:15Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIn its current state, the t7519 test dealing with untracked cache\nassumes that \"git update-index --untracked-cache\" will *populate* the\nuntracked cache. This is not correct - it will only add an empty\nuntracked cache structure to the index.\n\nIf we're going to compare two git status runs with something\ninteresting happening in-between, we need to ensure that the index is\nin a stable/steady state *before* that first run.\n\nAchieve this by adding another prior \"git status\" run.\n\nAt this stage this change does nothing, because there is a bug,\naddressed in the next patch, whereby once the empty untracked cache\nstructure is added by the update-index invocation, the untracked cache\ngets updated in every subsequent \"git status\" call, but the index with\nthese updates does not get written down.\n\nThat bug actually invalidates this entire test case - but we're fixing\nthat next.\n\nSigned-off-by: Tao Klerks \n---\n t/t7519-status-fsmonitor.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 3f984136ea9..fffc57120d6 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -341,6 +341,7 @@ test_expect_success UNTRACKED_CACHE 'ignore .git changes when invalidating UNTR'\n \t\tgit config core.fsmonitor .git/hooks/fsmonitor-test &&\n \t\tgit update-index --untracked-cache &&\n \t\tgit update-index --fsmonitor &&\n+\t\tgit status &&\n \t\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace-before\" \\\n \t\tgit status &&\n \t\ttest-tool dump-untracked-cache >../before\n-- \ngitgitgadget\n\n"},{"id":"449716","messageId":"9795a08414a9269bc6ae929ea8e8b22ad6ec0762.1645999021.git.gitgitgadget@gmail.com","threadId":"56002","inReplyTo":"pull.986.v3.git.1645999021.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] untracked-cache: write index when populating empty untracked cache","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-27T21:57:01Z","receivedAt":"2022-02-27T21:57:16Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nIt is expected that an empty/unpopulated untracked cache structure can\nbe written to the index - by update-index, or by a \"git status\" call\nthat sees the untracked cache should be enabled and is not, but is\nrunning with options that make the untracked cache non-applicable in\nthat run (eg a pathspec).\n\nCurrently, if that happens, then subsequent \"git status\" calls end up\npopulating the untracked cache, but not writing the index (not saving\ntheir work) - so the performance outcome is almost identical to the\ncache being altogether disabled.\n\nThis continues until the index gets written with the untracked cache\npopulated, for some *other* reason, such as a working tree change.\n\nDetect the condition where an empty untracked cache exists in the\nindex and we will collect the list of untracked paths, and queue an\nindex write under that condition, so that the collected untracked\npaths can be written out to the untracked cache extension in the\nindex.\n\nThis change depends on previous fixes to t7519 for the \"ignore .git\nchanges when invalidating UNTR\" test case to pass - before this fix,\nthe test never actually did anything as it was not set up correctly.\n\nSigned-off-by: Tao Klerks \n---\n dir.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d91295f2bcd..4eee45dec91 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2781,7 +2781,8 @@ void remove_untracked_cache(struct index_state *istate)\n \n static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n \t\t\t\t\t\t int base_len,\n-\t\t\t\t\t\t const struct pathspec *pathspec)\n+\t\t\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t\t\t struct index_state *istate)\n {\n \tstruct untracked_cache_dir *root;\n \tstatic int untracked_cache_disabled = -1;\n@@ -2845,8 +2846,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n-\tif (!dir->untracked->root)\n+\tif (!dir->untracked->root) {\n+\t\t/* Untracked cache existed but is not initialized; fix that */\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n+\t\tistate->cache_changed |= UNTRACKED_CHANGED;\n+\t}\n \n \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n \troot = dir->untracked->root;\n@@ -2916,7 +2920,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\treturn dir->nr;\n \t}\n \n-\tuntracked = validate_untracked_cache(dir, len, pathspec);\n+\tuntracked = validate_untracked_cache(dir, len, pathspec, istate);\n \tif (!untracked)\n \t\t/*\n \t\t * make sure untracked cache code path is disabled,\n-- \ngitgitgadget\n"},{"id":"449718","messageId":"CAPMMpoi4oQ4nORc-Jq00pFQq7eFYe0mZhwdU7fvTabvCdL1Okw@mail.gmail.com","threadId":"56002","inReplyTo":"xmqqr17qbw8l.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] t7519: avoid file to index mtime race for untracked cache","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-02-27T22:12:33Z","receivedAt":"2022-02-27T22:12:54Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Fri, Feb 25, 2022 at 8:07 PM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > In t7519 there is a test that writes files to disk, and immediately\n> > writes the untracked index. Because of mtime-comparison logic that\n>\n> \"untracked cache\", I think.\n>\n\nYep, fixed, thx.\n\n> > uses a 1-second resolution, this means the cached entries are not\n> > trusted/used under some circumstances\n> > (see read-cache.c#is_racy_stat()).\n> >\n> > Untracked cache tests in t7063 use a 1-second delay to avoid this\n> > issue, but we don't want to introduce arbitrary slowdowns, so instead\n> > use test-tool chmtime to backdate the files slightly.\n>\n> Good approach. Perhaps fixing 7063 can be marked as #leftoverbit?\n>\n\nSure. Updated the commit msg.\n\nI also started another thread with what that looks like - it's not very pretty\nin the end because of Windows compatibility issues, and still incomplete\nbecause (as far as I can tell) the sleep is used for a couple of different\npurposes, and only one of them lends itself to chmtime manipulation.\n"}]}