{"thread":{"id":"55990","subject":"[PATCH] Support untracked cache with '--untracked-files=all' if configured","startedAt":"2021-06-23T06:44:34Z","lastAt":"2022-03-31T16:02:30Z","messageCount":22,"participants":["Tao Klerks via GitGitGadget","Junio C Hamano","Tao Klerks","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"428256","messageId":"pull.985.git.1624430668741.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":null,"subject":"[PATCH] Support untracked cache with '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-23T06:44:28Z","receivedAt":"2021-06-23T06:44:34Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n'-untracked-files=normal', but this is a concern for UI tooling\nthat wants to see \"all\" on a frequent basis, and the conditions\nthat prevented applicability to the \"all\" mode no longer seem to\napply.\n\nThe disqualification of untracked cache is a particular problem on\nWindows with the defaulted fscache, where the introduction of\nfsmonitor can make the first and costly directory-iteration happen\nin untracked file detection, single-threaded, rather than in\npreload-index on multiple threads. Improving the performance of a\n\"normal\" 'git status' run with fsmonitor can make\n'git status --untracked-files=all' perform much worse.\n\nHere we align the supported directory flags for the stored\nuntracked cache data with the git config. If a user specifies an\n'--untracked-files=' commandline parameter that does not align\nwith their 'status.showuntrackedfiles' config value, then the\nuntracked cache will be ignored - as it is for other unsupported\nsituations like when a pathspec is specified.\n\nIf the previously stored flags no longer match the current\nconfiguration, but the currently-applicable flags do match the\ncurrent configuration, then the previously stored untracked cache\ndata is discarded.\n\nFor most users there will be no change in behavior. Users who\nneed '--untracked-files=all' to perform well will have the option\nof setting \"status.showuntrackedfiles\" to \"all\".\n\nUsers who need '--untracked-files=all' to perform well for their\ntooling AND prefer to avoid the verbosity of \"all\" when running\ngit status explicitly... are out of luck for now.\n\nUsers who set \"status.showuntrackedfiles\" to \"all\" and yet\nmost frequently explicitly call\n'git status --untracked-files=normal' (and use the untracked\ncache) are the only users who will be explicitly disadvantaged\nby this change. These should be vanishingly rare, if there are\nany at all.\n\nSigned-off-by: Tao Klerks \n---\n Support untracked cache with '--untracked-files=all' if configured\n \n This patch aims to make it possible for users of -uall, either by\n preference or by need (eg UI tooling), to benefit from the untracked\n cache when they set their 'status.showuntrackedfiles' config setting to\n 'all'. This is very important for large repos in Windows.\n \n More detail on the change and context in the commit message, I assume\n repeating a verbose message here is discouraged.\n \n These changes result from a couple of conversations,\n 81153d02-8e7a-be59-e709-e90cd5906f3a@jeffhostetler.com and\n CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com>.\n \n The test suite passes, but as a n00b I do have questions:\n \n * Is there any additional validatiln that could/should be done to\n confirm that \"-uall\" untracked data can be cached safely? It looks\n that way to me, both from following the code, discussing briefly with\n Elijah Newren in the thread above, and manual testing, but I'm not\n sure what edge-cases to be looking out for\n * Is it OK to check the repo configuration in the body of processing?\n It seems to be a rare pattern\n * Should I be adding test cases? To the existing t7603 untracked cache\n tests, or elsewhere?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-985%2FTaoK%2Ftaok-untracked-cache-with-uall-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-985/TaoK/taok-untracked-cache-with-uall-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/985\n\n dir.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 68 insertions(+), 18 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ebe5ec046e..49fc13c111 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2668,13 +2668,33 @@ static void set_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n-static void new_untracked_cache(struct index_state *istate)\n+static unsigned configured_default_dir_flags(struct index_state *istate)\n+{\n+\t/* This logic is coordinated with the setting of these flags in\n+\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n+\t * of the config setting in commit.c#git_status_config()\n+\t */\n+\tchar *status_untracked_files_config_value;\n+\tint config_outcome = repo_config_get_string(istate->repo,\n+\t\t\t\t\t\t\t\t\"status.showuntrackedfiles\",\n+\t\t\t\t\t\t\t\t&status_untracked_files_config_value);\n+\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n+\t\treturn 0;\n+\t} else {\n+\t\t/*\n+\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n+\t\t * If the value is \"none\" then it really doesn't matter.\n+\t\t */\n+\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\t}\n+}\n+\n+static void new_untracked_cache(struct index_state *istate, unsigned flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n-\t/* should be the same flags used by git-status */\n-\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tuc->dir_flags = flags;\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n@@ -2683,11 +2703,13 @@ static void new_untracked_cache(struct index_state *istate)\n void add_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n-\t\tnew_untracked_cache(istate);\n+\t\tnew_untracked_cache(istate,\n+\t\t\t\tconfigured_default_dir_flags(istate));\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate);\n+\t\t\tnew_untracked_cache(istate,\n+\t\t\t\t\tconfigured_default_dir_flags(istate));\n \t\t}\n \t}\n }\n@@ -2703,10 +2725,12 @@ 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\t struct index_state *istate)\n {\n \tstruct untracked_cache_dir *root;\n \tstatic int untracked_cache_disabled = -1;\n+\tunsigned configured_dir_flags;\n \n \tif (!dir->untracked)\n \t\treturn NULL;\n@@ -2734,17 +2758,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n \n-\t/* Different set of flags may produce different results */\n-\tif (dir->flags != dir->untracked->dir_flags ||\n-\t /*\n-\t * See treat_directory(), case index_nonexistent. Without\n-\t * this flag, we may need to also cache .git file content\n-\t * for the resolve_gitlink_ref() call, which we don't.\n-\t */\n-\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n-\t /* We don't support collecting ignore files */\n-\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n-\t\t\t DIR_COLLECT_IGNORED)))\n+\t/* We don't support collecting ignore files */\n+\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n+\t\t\tDIR_COLLECT_IGNORED))\n \t\treturn NULL;\n \n \t/*\n@@ -2767,6 +2783,40 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n+\t/* We don't support using or preparing the untracked cache if\n+\t * the current effective flags don't match the configured\n+\t * flags.\n+\t */\n+\tconfigured_dir_flags = configured_default_dir_flags(istate);\n+\tif (dir->flags != configured_dir_flags)\n+\t\treturn NULL;\n+\n+\t/* If the untracked structure we received does not have the same flags\n+\t * as configured, but the configured flags do match the effective flags,\n+\t * then we need to reset / create a new \"untracked\" structure to match\n+\t * the new config.\n+\t * Keeping the saved and used untracked cache in-line with the\n+\t * configuration provides an opportunity for frequent users of\n+\t * \"git status -uall\" to leverage the untracked cache by aligning their\n+\t * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n+\t * \"normal\" as appropriate), where previously this option was\n+\t * incompatible with untracked cache and *consistently* caused\n+\t * surprisingly bad performance (with fscache and fsmonitor enabled) on\n+\t * Windows.\n+\t *\n+\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n+\t * to not be as bound up with the desired output in a given run,\n+\t * and instead iterated through and stored enough information to\n+\t * correctly serve both \"modes\", then users could get peak performance\n+\t * with or without '-uall' regardless of their\n+\t * \"status.showuntrackedfiles\" config.\n+\t */\n+\tif (dir->flags != dir->untracked->dir_flags) {\n+\t\tfree_untracked_cache(istate->untracked);\n+\t\tnew_untracked_cache(istate, configured_dir_flags);\n+\t\tdir->untracked = istate->untracked;\n+\t}\n+\n \tif (!dir->untracked->root)\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n \n@@ -2838,7 +2888,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\nbase-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558\n-- \ngitgitgadget\n"},{"id":"449614","messageId":"pull.985.v2.git.1645811564461.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.git.1624430668741.gitgitgadget@gmail.com","subject":"[PATCH v2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T17:52:44Z","receivedAt":"2022-02-25T17:52:52Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n'-untracked-files=normal', but this is a concern for UI tooling that\nwants to see \"all\" on a frequent basis, and the conditions that\nprevented applicability to the \"all\" mode no longer seem to apply.\n\nThe disqualification of untracked cache is a particular problem on\nWindows with the defaulted fscache, where the introduction of\nfsmonitor can make the first and costly directory-iteration happen\nin untracked file detection, single-threaded, rather than in\npreload-index on multiple threads. Improving the performance of a\n\"normal\" 'git status' run with fsmonitor can make\n'git status --untracked-files=all' perform much worse.\n\nIn this change, align the supported directory flags for the stored\nuntracked cache data with the git config. If a user specifies an\n'--untracked-files=' commandline parameter that does not align with\ntheir 'status.showuntrackedfiles' config value, then the untracked\ncache will be ignored - as it is for other unsupported situations like\nwhen a pathspec is specified.\n\nIf the previously stored flags no longer match the current\nconfiguration, but the currently-applicable flags do match the current\nconfiguration, then the previously stored untracked cache data is\ndiscarded.\n\nFor most users there will be no change in behavior. Users who need\n'--untracked-files=all' to perform well will have the option of\nsetting \"status.showuntrackedfiles\" to \"all\".\n\nUsers who need '--untracked-files=all' to perform well for their\ntooling AND prefer to avoid the verbosity of \"all\" when running\ngit status explicitly without options... are out of luck for now (no\nchange).\n\nUsers who set \"status.showuntrackedfiles\" to \"all\" and yet most\nfrequently explicitly call 'git status --untracked-files=normal' (and\nuse the untracked cache) are the only ones who would be disadvantaged\nby this change. It seems unlikely there are any such users.\n\nSigned-off-by: Tao Klerks \n---\n Support untracked cache with '--untracked-files=all' if configured\n \n Resending this patch from a few months ago (with better standards\n compliance) - it hasn't seen any comments yet, I would dearly love some\n eyes on this as the change can make a big difference to hundreds of\n windows users in my environment (and I'd really rather not start\n distributing customized git builds!)\n \n This patch aims to make it possible for users of the -uall flag to git\n status, either by preference or by need (eg UI tooling), to benefit from\n the untracked cache when they set their 'status.showuntrackedfiles'\n config setting to 'all'. This is very important for large repos in\n Windows.\n \n More detail on the change and context in the commit message, I assume\n repeating a verbose message here is discouraged.\n \n These changes result from a couple of conversations,\n 81153d02-8e7a-be59-e709-e90cd5906f3a@jeffhostetler.com and\n CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com>.\n \n The test suite passes, but as a n00b I do have questions:\n \n * Is there any additional validation that could/should be done to\n confirm that \"-uall\" untracked data can be cached safely?\n \n ** It looks safe from following the code ** It seems from discussing\n briefly with Elijah Newren in the thread above that thare are no obvious\n red flags ** Manual testing, explicitly and implicitly through months of\n use, yields no issues\n \n * Is it OK to check the repo configuration in the body of processing?\n It seems to be a rare pattern\n * Can anyone think of a way to test this change?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-985%2FTaoK%2Ftaok-untracked-cache-with-uall-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-985/TaoK/taok-untracked-cache-with-uall-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/985\n\nRange-diff vs v1:\n\n 1: 2797efad9a4 ! 1: e2f1ad26c78 Support untracked cache with '--untracked-files=all' if configured\n @@ Metadata\n Author: Tao Klerks \n \n ## Commit message ##\n - Support untracked cache with '--untracked-files=all' if configured\n + untracked-cache: support '--untracked-files=all' if configured\n \n Untracked cache was originally designed to only work with\n - '-untracked-files=normal', but this is a concern for UI tooling\n - that wants to see \"all\" on a frequent basis, and the conditions\n - that prevented applicability to the \"all\" mode no longer seem to\n - apply.\n + '-untracked-files=normal', but this is a concern for UI tooling that\n + wants to see \"all\" on a frequent basis, and the conditions that\n + prevented applicability to the \"all\" mode no longer seem to apply.\n \n The disqualification of untracked cache is a particular problem on\n Windows with the defaulted fscache, where the introduction of\n @@ Commit message\n \"normal\" 'git status' run with fsmonitor can make\n 'git status --untracked-files=all' perform much worse.\n \n - Here we align the supported directory flags for the stored\n + In this change, align the supported directory flags for the stored\n untracked cache data with the git config. If a user specifies an\n - '--untracked-files=' commandline parameter that does not align\n - with their 'status.showuntrackedfiles' config value, then the\n - untracked cache will be ignored - as it is for other unsupported\n - situations like when a pathspec is specified.\n + '--untracked-files=' commandline parameter that does not align with\n + their 'status.showuntrackedfiles' config value, then the untracked\n + cache will be ignored - as it is for other unsupported situations like\n + when a pathspec is specified.\n \n If the previously stored flags no longer match the current\n - configuration, but the currently-applicable flags do match the\n - current configuration, then the previously stored untracked cache\n - data is discarded.\n + configuration, but the currently-applicable flags do match the current\n + configuration, then the previously stored untracked cache data is\n + discarded.\n \n - For most users there will be no change in behavior. Users who\n - need '--untracked-files=all' to perform well will have the option\n - of setting \"status.showuntrackedfiles\" to \"all\".\n + For most users there will be no change in behavior. Users who need\n + '--untracked-files=all' to perform well will have the option of\n + setting \"status.showuntrackedfiles\" to \"all\".\n \n Users who need '--untracked-files=all' to perform well for their\n tooling AND prefer to avoid the verbosity of \"all\" when running\n - git status explicitly... are out of luck for now.\n + git status explicitly without options... are out of luck for now (no\n + change).\n \n - Users who set \"status.showuntrackedfiles\" to \"all\" and yet\n - most frequently explicitly call\n - 'git status --untracked-files=normal' (and use the untracked\n - cache) are the only users who will be explicitly disadvantaged\n - by this change. These should be vanishingly rare, if there are\n - any at all.\n + Users who set \"status.showuntrackedfiles\" to \"all\" and yet most\n + frequently explicitly call 'git status --untracked-files=normal' (and\n + use the untracked cache) are the only ones who would be disadvantaged\n + by this change. It seems unlikely there are any such users.\n \n Signed-off-by: Tao Klerks \n \n\n\n dir.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 68 insertions(+), 18 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d91295f2bcd..e35331d3f71 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2746,13 +2746,33 @@ static void set_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n-static void new_untracked_cache(struct index_state *istate)\n+static unsigned configured_default_dir_flags(struct index_state *istate)\n+{\n+\t/* This logic is coordinated with the setting of these flags in\n+\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n+\t * of the config setting in commit.c#git_status_config()\n+\t */\n+\tchar *status_untracked_files_config_value;\n+\tint config_outcome = repo_config_get_string(istate->repo,\n+\t\t\t\t\t\t\t\t\"status.showuntrackedfiles\",\n+\t\t\t\t\t\t\t\t&status_untracked_files_config_value);\n+\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n+\t\treturn 0;\n+\t} else {\n+\t\t/*\n+\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n+\t\t * If the value is \"none\" then it really doesn't matter.\n+\t\t */\n+\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\t}\n+}\n+\n+static void new_untracked_cache(struct index_state *istate, unsigned flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n-\t/* should be the same flags used by git-status */\n-\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tuc->dir_flags = flags;\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n@@ -2761,11 +2781,13 @@ static void new_untracked_cache(struct index_state *istate)\n void add_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n-\t\tnew_untracked_cache(istate);\n+\t\tnew_untracked_cache(istate,\n+\t\t\t\tconfigured_default_dir_flags(istate));\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate);\n+\t\t\tnew_untracked_cache(istate,\n+\t\t\t\t\tconfigured_default_dir_flags(istate));\n \t\t}\n \t}\n }\n@@ -2781,10 +2803,12 @@ 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\t struct index_state *istate)\n {\n \tstruct untracked_cache_dir *root;\n \tstatic int untracked_cache_disabled = -1;\n+\tunsigned configured_dir_flags;\n \n \tif (!dir->untracked)\n \t\treturn NULL;\n@@ -2812,17 +2836,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n \n-\t/* Different set of flags may produce different results */\n-\tif (dir->flags != dir->untracked->dir_flags ||\n-\t /*\n-\t * See treat_directory(), case index_nonexistent. Without\n-\t * this flag, we may need to also cache .git file content\n-\t * for the resolve_gitlink_ref() call, which we don't.\n-\t */\n-\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n-\t /* We don't support collecting ignore files */\n-\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n-\t\t\t DIR_COLLECT_IGNORED)))\n+\t/* We don't support collecting ignore files */\n+\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n+\t\t\tDIR_COLLECT_IGNORED))\n \t\treturn NULL;\n \n \t/*\n@@ -2845,6 +2861,40 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n+\t/* We don't support using or preparing the untracked cache if\n+\t * the current effective flags don't match the configured\n+\t * flags.\n+\t */\n+\tconfigured_dir_flags = configured_default_dir_flags(istate);\n+\tif (dir->flags != configured_dir_flags)\n+\t\treturn NULL;\n+\n+\t/* If the untracked structure we received does not have the same flags\n+\t * as configured, but the configured flags do match the effective flags,\n+\t * then we need to reset / create a new \"untracked\" structure to match\n+\t * the new config.\n+\t * Keeping the saved and used untracked cache in-line with the\n+\t * configuration provides an opportunity for frequent users of\n+\t * \"git status -uall\" to leverage the untracked cache by aligning their\n+\t * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n+\t * \"normal\" as appropriate), where previously this option was\n+\t * incompatible with untracked cache and *consistently* caused\n+\t * surprisingly bad performance (with fscache and fsmonitor enabled) on\n+\t * Windows.\n+\t *\n+\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n+\t * to not be as bound up with the desired output in a given run,\n+\t * and instead iterated through and stored enough information to\n+\t * correctly serve both \"modes\", then users could get peak performance\n+\t * with or without '-uall' regardless of their\n+\t * \"status.showuntrackedfiles\" config.\n+\t */\n+\tif (dir->flags != dir->untracked->dir_flags) {\n+\t\tfree_untracked_cache(istate->untracked);\n+\t\tnew_untracked_cache(istate, configured_dir_flags);\n+\t\tdir->untracked = istate->untracked;\n+\t}\n+\n \tif (!dir->untracked->root)\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n \n@@ -2916,7 +2966,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\nbase-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n-- \ngitgitgadget\n"},{"id":"449632","messageId":"xmqqtucmag00.fsf@gitster.g","threadId":"55990","inReplyTo":"pull.985.v2.git.1645811564461.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T19:43:59Z","receivedAt":"2022-02-25T19:44:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> If the previously stored flags no longer match the current\n> configuration, but the currently-applicable flags do match the current\n> configuration, then the previously stored untracked cache data is\n> discarded.\n>\n> For most users there will be no change in behavior. Users who need\n> '--untracked-files=all' to perform well will have the option of\n> setting \"status.showuntrackedfiles\" to \"all\".\n>\n> Users who need '--untracked-files=all' to perform well for their\n> tooling AND prefer to avoid the verbosity of \"all\" when running\n> git status explicitly without options... are out of luck for now (no\n> change).\n\nSo, in short, the root of the problem is that untracked cache can be\nused to serve only one mode (between normal and all) of operation,\nand the real solution to that problem must come in a different form,\ni.e. allowing a single unified untracked cache to be usable in both\nmodes, perhaps by maintaining all the untracked ones in the cache,\nbut filter out upon output when the 'normal' mode is asked for?\n\n> Users who set \"status.showuntrackedfiles\" to \"all\" and yet most\n> frequently explicitly call 'git status --untracked-files=normal' (and\n> use the untracked cache) are the only ones who would be disadvantaged\n> by this change. It seems unlikely there are any such users.\n\nGiven how widely used we are these days, I am afraid that the days\nwe can safely say \"users with such a strange use pattern would not\nexist at all\" is long gone.\n\n> +static unsigned configured_default_dir_flags(struct index_state *istate)\n> +{\n> +\t/* This logic is coordinated with the setting of these flags in\n> +\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n> +\t * of the config setting in commit.c#git_status_config()\n> +\t */\n\nGood thing to note here.\n\nStyle.\n\n /*\n * Our multi-line comments reads more like this, with\n * slash-asterisk that opens, and asterisk-slash that closes,\n * sitting on their own lines.\n */\n\n> +\tchar *status_untracked_files_config_value;\n> +\tint config_outcome = repo_config_get_string(istate->repo,\n> +\t\t\t\t\t\t\t\t\"status.showuntrackedfiles\",\n\nThe indentation looks a bit off.\n\nIn this project, tab width is 8. The beginning of the second\nparameter to the function call on the second line should align with\nthe beginning of the first parameter that immediately follows the\nopen parenthesis '('.\n\n> +\t\t\t\t\t\t\t\t&status_untracked_files_config_value);\n> +\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n> +\t\treturn 0;\n> +\t} else {\n> +\t\t/*\n> +\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n> +\t\t * If the value is \"none\" then it really doesn't matter.\n> +\t\t */\n> +\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> +\t}\n> +}\n\nI didn't see the need to pass istate to this function, though.\nShouldn't it take a repository instead?\n\n> -static void new_untracked_cache(struct index_state *istate)\n> +static void new_untracked_cache(struct index_state *istate, unsigned flags)\n> {\n> \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n> \tstrbuf_init(&uc->ident, 100);\n> \tuc->exclude_per_dir = \".gitignore\";\n> -\t/* should be the same flags used by git-status */\n> -\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> +\tuc->dir_flags = flags;\n\nSo we used to hardcode these two flags to match what is done in\nwt-status.c when show_untracked_files != SHOW_ALL_UNTRACKEDFILES;\nwe allow a different set of flags to be given by the caller.\n\n> @@ -2761,11 +2781,13 @@ static void new_untracked_cache(struct index_state *istate)\n> void add_untracked_cache(struct index_state *istate)\n> {\n> \tif (!istate->untracked) {\n> -\t\tnew_untracked_cache(istate);\n> +\t\tnew_untracked_cache(istate,\n> +\t\t\t\tconfigured_default_dir_flags(istate));\n> \t} else {\n> \t\tif (!ident_in_untracked(istate->untracked)) {\n> \t\t\tfree_untracked_cache(istate->untracked);\n> -\t\t\tnew_untracked_cache(istate);\n> +\t\t\tnew_untracked_cache(istate,\n> +\t\t\t\t\tconfigured_default_dir_flags(istate));\n> \t\t}\n> \t}\n> }\n\nOK. That's quite straight-forward to see how the tweak is made.\n\n> @@ -2781,10 +2803,12 @@ 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\t struct index_state *istate)\n> {\n> \tstruct untracked_cache_dir *root;\n> \tstatic int untracked_cache_disabled = -1;\n> +\tunsigned configured_dir_flags;\n> \n> \tif (!dir->untracked)\n> \t\treturn NULL;\n> @@ -2812,17 +2836,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> \tif (base_len || (pathspec && pathspec->nr))\n> \t\treturn NULL;\n> \n> -\t/* Different set of flags may produce different results */\n> -\tif (dir->flags != dir->untracked->dir_flags ||\n\nThis is removed because we are making sure that dir->flags and\ndir->untracked->dir_flags match?\n\n> -\t /*\n> -\t * See treat_directory(), case index_nonexistent. Without\n> -\t * this flag, we may need to also cache .git file content\n> -\t * for the resolve_gitlink_ref() call, which we don't.\n> -\t */\n> -\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n\nThis is removed because...?\n\n> -\t /* We don't support collecting ignore files */\n> -\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> -\t\t\t DIR_COLLECT_IGNORED)))\n\n> +\t/* We don't support collecting ignore files */\n> +\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> +\t\t\tDIR_COLLECT_IGNORED))\n> \t\treturn NULL;\n> \n> \t/*\n> @@ -2845,6 +2861,40 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> \t\treturn NULL;\n> \t}\n> \n> +\t/* We don't support using or preparing the untracked cache if\n> +\t * the current effective flags don't match the configured\n> +\t * flags.\n> +\t */\n\nStyle (another one in large comment below).\n\n> +\tconfigured_dir_flags = configured_default_dir_flags(istate);\n> +\tif (dir->flags != configured_dir_flags)\n> +\t\treturn NULL;\n\nHmph. If this weren't necessary, this function does not need to\ncall configured_default_dir_flags(), and it can lose the\nconfigured_dir_flags variable, too. Which means that\nnew_untracked_cache() function does not need to take the flags word\nas a caller-supplied parameter. Instead, it can make a call to\nconfigured_dir_flags() and assign the result to uc->dir_flags\nitself, which would have been much nicer.\n\n> +\t/* If the untracked structure we received does not have the same flags\n> +\t * as configured, but the configured flags do match the effective flags,\n> +\t * then we need to reset / create a new \"untracked\" structure to match\n> +\t * the new config.\n> +\t * Keeping the saved and used untracked cache in-line with the\n> +\t * configuration provides an opportunity for frequent users of\n> +\t * \"git status -uall\" to leverage the untracked cache by aligning their\n> +\t * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n> +\t * \"normal\" as appropriate), where previously this option was\n> +\t * incompatible with untracked cache and *consistently* caused\n> +\t * surprisingly bad performance (with fscache and fsmonitor enabled) on\n> +\t * Windows.\n> +\t *\n> +\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n> +\t * to not be as bound up with the desired output in a given run,\n> +\t * and instead iterated through and stored enough information to\n> +\t * correctly serve both \"modes\", then users could get peak performance\n> +\t * with or without '-uall' regardless of their\n> +\t * \"status.showuntrackedfiles\" config.\n> +\t */\n> +\tif (dir->flags != dir->untracked->dir_flags) {\n> +\t\tfree_untracked_cache(istate->untracked);\n> +\t\tnew_untracked_cache(istate, configured_dir_flags);\n> +\t\tdir->untracked = istate->untracked;\n> +\t}\n\n\nThis compensates what we lost in the validate_untracked_cache()\nabove by making them match. Looking reasonable.\n\n> \tif (!dir->untracked->root)\n> \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n> \n> @@ -2916,7 +2966,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>\n> base-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n"},{"id":"449691","messageId":"CAPMMpoixi3x1PHrSHJPV1GRBzMpuOQ4meMr-fipXuDvz-96MEA@mail.gmail.com","threadId":"55990","inReplyTo":"xmqqtucmag00.fsf@gitster.g","subject":"Re: [PATCH v2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-02-27T11:21:30Z","receivedAt":"2022-02-27T11:21:46Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Fri, Feb 25, 2022 at 8:44 PM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > If the previously stored flags no longer match the current\n> > configuration, but the currently-applicable flags do match the current\n> > configuration, then the previously stored untracked cache data is\n> > discarded.\n> >\n> > For most users there will be no change in behavior. Users who need\n> > '--untracked-files=all' to perform well will have the option of\n> > setting \"status.showuntrackedfiles\" to \"all\".\n> >\n> > Users who need '--untracked-files=all' to perform well for their\n> > tooling AND prefer to avoid the verbosity of \"all\" when running\n> > git status explicitly without options... are out of luck for now (no\n> > change).\n>\n> So, in short, the root of the problem is that untracked cache can be\n> used to serve only one mode (between normal and all) of operation,\n> and the real solution to that problem must come in a different form,\n> i.e. allowing a single unified untracked cache to be usable in both\n> modes, perhaps by maintaining all the untracked ones in the cache,\n> but filter out upon output when the 'normal' mode is asked for?\n\nI wouldn't call this the root of the problem I was trying to solve with this\npatch, but rather the root of the remaining problem, yes.\n\nThe challenge that I can't get my head around for this longer-term\napproach or objective, is *when* the untracked-folder nested files\nshould be enumerated.\n\nCurrently, untracked folders are only recursed into if -uall is\nspecified or configured - and that's a significant characteristic:\nIt's perfectly plausible that some users sometimes have huge\ndeep untracked folder hierarchies that take a long time to explore,\nbut git never needs to because they never specify -uall.\n\nIf we decided to serve both modes with one single untracked\ncache structure, then we would either need to always fully\nrecurse, or implement some sort of \"just-in-time\" filling in of the\nrecursive bits when someone specifies -uall for the first time.\n\nEither way, I'm pretty sure it's beyond me to do that right. Hence\nthis very-pragmatic approach that makes it *possible* to get\ngood/normal performance with -uall.\n\n>\n> > Users who set \"status.showuntrackedfiles\" to \"all\" and yet most\n> > frequently explicitly call 'git status --untracked-files=normal' (and\n> > use the untracked cache) are the only ones who would be disadvantaged\n> > by this change. It seems unlikely there are any such users.\n>\n> Given how widely used we are these days, I am afraid that the days\n> we can safely say \"users with such a strange use pattern would not\n> exist at all\" is long gone.\n>\n> > +static unsigned configured_default_dir_flags(struct index_state *istate)\n> > +{\n> > + /* This logic is coordinated with the setting of these flags in\n> > + * wt-status.c#wt_status_collect_untracked(), and the evaluation\n> > + * of the config setting in commit.c#git_status_config()\n> > + */\n>\n> Good thing to note here.\n>\n> Style.\n>\n> /*\n> * Our multi-line comments reads more like this, with\n> * slash-asterisk that opens, and asterisk-slash that closes,\n> * sitting on their own lines.\n> */\n\nThanks, sorry, I thought I'd corrected them all but clearly missed some.\n\n>\n> > + char *status_untracked_files_config_value;\n> > + int config_outcome = repo_config_get_string(istate->repo,\n> > + \"status.showuntrackedfiles\",\n>\n> The indentation looks a bit off.\n>\n> In this project, tab width is 8. The beginning of the second\n> parameter to the function call on the second line should align with\n> the beginning of the first parameter that immediately follows the\n> open parenthesis '('.\n>\n\nFixed, thx.\n\n> > + &status_untracked_files_config_value);\n> > + if (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n> > + return 0;\n> > + } else {\n> > + /*\n> > + * The default, if \"all\" is not set, is \"normal\" - leading us here.\n> > + * If the value is \"none\" then it really doesn't matter.\n> > + */\n> > + return DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> > + }\n> > +}\n>\n> I didn't see the need to pass istate to this function, though.\n> Shouldn't it take a repository instead?\n\nMakes sense, fixed, thx.\n\n>\n> > -static void new_untracked_cache(struct index_state *istate)\n> > +static void new_untracked_cache(struct index_state *istate, unsigned flags)\n> > {\n> > struct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n> > strbuf_init(&uc->ident, 100);\n> > uc->exclude_per_dir = \".gitignore\";\n> > - /* should be the same flags used by git-status */\n> > - uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> > + uc->dir_flags = flags;\n>\n> So we used to hardcode these two flags to match what is done in\n> wt-status.c when show_untracked_files != SHOW_ALL_UNTRACKEDFILES;\n> we allow a different set of flags to be given by the caller.\n>\n> > @@ -2761,11 +2781,13 @@ static void new_untracked_cache(struct index_state *istate)\n> > void add_untracked_cache(struct index_state *istate)\n> > {\n> > if (!istate->untracked) {\n> > - new_untracked_cache(istate);\n> > + new_untracked_cache(istate,\n> > + configured_default_dir_flags(istate));\n> > } else {\n> > if (!ident_in_untracked(istate->untracked)) {\n> > free_untracked_cache(istate->untracked);\n> > - new_untracked_cache(istate);\n> > + new_untracked_cache(istate,\n> > + configured_default_dir_flags(istate));\n> > }\n> > }\n> > }\n>\n> OK. That's quite straight-forward to see how the tweak is made.\n>\n> > @@ -2781,10 +2803,12 @@ 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> > + unsigned configured_dir_flags;\n> >\n> > if (!dir->untracked)\n> > return NULL;\n> > @@ -2812,17 +2836,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> > if (base_len || (pathspec && pathspec->nr))\n> > return NULL;\n> >\n> > - /* Different set of flags may produce different results */\n> > - if (dir->flags != dir->untracked->dir_flags ||\n>\n> This is removed because we are making sure that dir->flags and\n> dir->untracked->dir_flags match?\n>\n> > - /*\n> > - * See treat_directory(), case index_nonexistent. Without\n> > - * this flag, we may need to also cache .git file content\n> > - * for the resolve_gitlink_ref() call, which we don't.\n> > - */\n> > - !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n>\n> This is removed because...?\n\nBecause we *do* now support using untracked cache with -uall...\nAs long as the config matches the runtime flags (new check later).\n\n>\n> > - /* We don't support collecting ignore files */\n> > - (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> > - DIR_COLLECT_IGNORED)))\n>\n> > + /* We don't support collecting ignore files */\n> > + if (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> > + DIR_COLLECT_IGNORED))\n> > return NULL;\n> >\n> > /*\n> > @@ -2845,6 +2861,40 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> > return NULL;\n> > }\n> >\n> > + /* We don't support using or preparing the untracked cache if\n> > + * the current effective flags don't match the configured\n> > + * flags.\n> > + */\n>\n> Style (another one in large comment below).\n>\n\nThx.\n\n> > + configured_dir_flags = configured_default_dir_flags(istate);\n> > + if (dir->flags != configured_dir_flags)\n> > + return NULL;\n>\n> Hmph. If this weren't necessary, this function does not need to\n> call configured_default_dir_flags(), and it can lose the\n> configured_dir_flags variable, too. Which means that\n> new_untracked_cache() function does not need to take the flags word\n> as a caller-supplied parameter. Instead, it can make a call to\n> configured_dir_flags() and assign the result to uc->dir_flags\n> itself, which would have been much nicer.\n\nI've tightened this up a little with an inline call to\nconfigured_default_dir_flags(), getting rid of the variable, let's see\nif that makes more sense / is cleaner.\n\nSending new version with these changes.\n\n>\n> > + /* If the untracked structure we received does not have the same flags\n> > + * as configured, but the configured flags do match the effective flags,\n> > + * then we need to reset / create a new \"untracked\" structure to match\n> > + * the new config.\n> > + * Keeping the saved and used untracked cache in-line with the\n> > + * configuration provides an opportunity for frequent users of\n> > + * \"git status -uall\" to leverage the untracked cache by aligning their\n> > + * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n> > + * \"normal\" as appropriate), where previously this option was\n> > + * incompatible with untracked cache and *consistently* caused\n> > + * surprisingly bad performance (with fscache and fsmonitor enabled) on\n> > + * Windows.\n> > + *\n> > + * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n> > + * to not be as bound up with the desired output in a given run,\n> > + * and instead iterated through and stored enough information to\n> > + * correctly serve both \"modes\", then users could get peak performance\n> > + * with or without '-uall' regardless of their\n> > + * \"status.showuntrackedfiles\" config.\n> > + */\n> > + if (dir->flags != dir->untracked->dir_flags) {\n> > + free_untracked_cache(istate->untracked);\n> > + new_untracked_cache(istate, configured_dir_flags);\n> > + dir->untracked = istate->untracked;\n> > + }\n>\n>\n> This compensates what we lost in the validate_untracked_cache()\n> above by making them match. Looking reasonable.\n>\n> > if (!dir->untracked->root)\n> > FLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n> >\n> > @@ -2916,7 +2966,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> >\n> > base-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n"},{"id":"449692","messageId":"pull.985.v3.git.1645960973798.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v2.git.1645811564461.gitgitgadget@gmail.com","subject":"[PATCH v3] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-27T11:22:53Z","receivedAt":"2022-02-27T11:23:01Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n'-untracked-files=normal', but this causes performance issues for UI\ntooling that wants to see \"all\" on a frequent basis. On the other hand,\nthe conditions that prevented applicability to the \"all\" mode no\nlonger seem to apply.\n\nThe disqualification of untracked cache has a particularly significant\nimpact on Windows with the defaulted fscache, where the introduction\nof fsmonitor can make the first and costly directory-iteration happen\nin untracked file detection, single-threaded, rather than in\npreload-index on multiple threads. Improving the performance of a\n\"normal\" 'git status' run with fsmonitor can make\n'git status --untracked-files=all' perform much worse.\n\nTo partially address this, align the supported directory flags for the\nstored untracked cache data with the git config. If a user specifies\nan '--untracked-files=' commandline parameter that does not align with\ntheir 'status.showuntrackedfiles' config value, then the untracked\ncache will be ignored - as it is for other unsupported situations like\nwhen a pathspec is specified.\n\nIf the previously stored flags no longer match the current\nconfiguration, but the currently-applicable flags do match the current\nconfiguration, then discard the previously stored untracked cache\ndata.\n\nFor most users there will be no change in behavior. Users who need\n'--untracked-files=all' to perform well will now have the option of\nsetting \"status.showuntrackedfiles\" to \"all\" for better / more\nconsistent performance.\n\nUsers who need '--untracked-files=all' to perform well for their\ntooling AND prefer to avoid the verbosity of \"all\" when running\ngit status explicitly without options... are out of luck for now (no\nchange).\n\nUsers who have the \"status.showuntrackedfiles\" config set to \"all\"\nand yet frequently explicitly call\n'git status --untracked-files=normal' (and use the untracked cache)\nare the only ones who will be disadvantaged by this change. Their\n\"--untracked-files=normal\" calls will, after this change, no longer\nuse the untracked cache.\n\nSigned-off-by: Tao Klerks \n---\n Support untracked cache with '--untracked-files=all' if configured\n \n Resending this patch from a few months ago (with better standards\n compliance) - it hasn't seen any comments yet, I would dearly love some\n eyes on this as the change can make a big difference to hundreds of\n windows users in my environment (and I'd really rather not start\n distributing customized git builds!)\n \n This patch aims to make it possible for users of the -uall flag to git\n status, either by preference or by need (eg UI tooling), to benefit from\n the untracked cache when they set their 'status.showuntrackedfiles'\n config setting to 'all'. This is very important for large repos in\n Windows.\n \n More detail on the change and context in the commit message, I assume\n repeating a verbose message here is discouraged.\n \n These changes result from a couple of conversations,\n 81153d02-8e7a-be59-e709-e90cd5906f3a@jeffhostetler.com and\n CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com>.\n \n The test suite passes, but as a n00b I do have questions:\n \n * Is there any additional validation that could/should be done to\n confirm that \"-uall\" untracked data can be cached safely?\n * It looks safe from following the code\n * It seems from discussing briefly with Elijah Newren in the thread\n above that thare are no obvious red flags\n * Manual testing, explicitly and implicitly through months of use,\n yields no issues\n * Is it OK to check the repo configuration in the body of processing?\n It seems to be a rare pattern\n * Can anyone think of a way to test this change?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-985%2FTaoK%2Ftaok-untracked-cache-with-uall-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-985/TaoK/taok-untracked-cache-with-uall-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/985\n\nRange-diff vs v2:\n\n 1: e2f1ad26c78 ! 1: 49cf90bfab5 untracked-cache: support '--untracked-files=all' if configured\n @@ Commit message\n untracked-cache: support '--untracked-files=all' if configured\n \n Untracked cache was originally designed to only work with\n - '-untracked-files=normal', but this is a concern for UI tooling that\n - wants to see \"all\" on a frequent basis, and the conditions that\n - prevented applicability to the \"all\" mode no longer seem to apply.\n + '-untracked-files=normal', but this causes performance issues for UI\n + tooling that wants to see \"all\" on a frequent basis. On the other hand,\n + the conditions that prevented applicability to the \"all\" mode no\n + longer seem to apply.\n \n - The disqualification of untracked cache is a particular problem on\n - Windows with the defaulted fscache, where the introduction of\n - fsmonitor can make the first and costly directory-iteration happen\n + The disqualification of untracked cache has a particularly significant\n + impact on Windows with the defaulted fscache, where the introduction\n + of fsmonitor can make the first and costly directory-iteration happen\n in untracked file detection, single-threaded, rather than in\n preload-index on multiple threads. Improving the performance of a\n \"normal\" 'git status' run with fsmonitor can make\n 'git status --untracked-files=all' perform much worse.\n \n - In this change, align the supported directory flags for the stored\n - untracked cache data with the git config. If a user specifies an\n - '--untracked-files=' commandline parameter that does not align with\n + To partially address this, align the supported directory flags for the\n + stored untracked cache data with the git config. If a user specifies\n + an '--untracked-files=' commandline parameter that does not align with\n their 'status.showuntrackedfiles' config value, then the untracked\n cache will be ignored - as it is for other unsupported situations like\n when a pathspec is specified.\n \n If the previously stored flags no longer match the current\n configuration, but the currently-applicable flags do match the current\n - configuration, then the previously stored untracked cache data is\n - discarded.\n + configuration, then discard the previously stored untracked cache\n + data.\n \n For most users there will be no change in behavior. Users who need\n - '--untracked-files=all' to perform well will have the option of\n - setting \"status.showuntrackedfiles\" to \"all\".\n + '--untracked-files=all' to perform well will now have the option of\n + setting \"status.showuntrackedfiles\" to \"all\" for better / more\n + consistent performance.\n \n Users who need '--untracked-files=all' to perform well for their\n tooling AND prefer to avoid the verbosity of \"all\" when running\n git status explicitly without options... are out of luck for now (no\n change).\n \n - Users who set \"status.showuntrackedfiles\" to \"all\" and yet most\n - frequently explicitly call 'git status --untracked-files=normal' (and\n - use the untracked cache) are the only ones who would be disadvantaged\n - by this change. It seems unlikely there are any such users.\n + Users who have the \"status.showuntrackedfiles\" config set to \"all\"\n + and yet frequently explicitly call\n + 'git status --untracked-files=normal' (and use the untracked cache)\n + are the only ones who will be disadvantaged by this change. Their\n + \"--untracked-files=normal\" calls will, after this change, no longer\n + use the untracked cache.\n \n Signed-off-by: Tao Klerks \n \n\n\n dir.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 68 insertions(+), 18 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d91295f2bcd..e35331d3f71 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2746,13 +2746,33 @@ static void set_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n-static void new_untracked_cache(struct index_state *istate)\n+static unsigned configured_default_dir_flags(struct index_state *istate)\n+{\n+\t/* This logic is coordinated with the setting of these flags in\n+\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n+\t * of the config setting in commit.c#git_status_config()\n+\t */\n+\tchar *status_untracked_files_config_value;\n+\tint config_outcome = repo_config_get_string(istate->repo,\n+\t\t\t\t\t\t\t\t\"status.showuntrackedfiles\",\n+\t\t\t\t\t\t\t\t&status_untracked_files_config_value);\n+\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n+\t\treturn 0;\n+\t} else {\n+\t\t/*\n+\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n+\t\t * If the value is \"none\" then it really doesn't matter.\n+\t\t */\n+\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\t}\n+}\n+\n+static void new_untracked_cache(struct index_state *istate, unsigned flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n-\t/* should be the same flags used by git-status */\n-\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tuc->dir_flags = flags;\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n@@ -2761,11 +2781,13 @@ static void new_untracked_cache(struct index_state *istate)\n void add_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n-\t\tnew_untracked_cache(istate);\n+\t\tnew_untracked_cache(istate,\n+\t\t\t\tconfigured_default_dir_flags(istate));\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate);\n+\t\t\tnew_untracked_cache(istate,\n+\t\t\t\t\tconfigured_default_dir_flags(istate));\n \t\t}\n \t}\n }\n@@ -2781,10 +2803,12 @@ 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\t struct index_state *istate)\n {\n \tstruct untracked_cache_dir *root;\n \tstatic int untracked_cache_disabled = -1;\n+\tunsigned configured_dir_flags;\n \n \tif (!dir->untracked)\n \t\treturn NULL;\n@@ -2812,17 +2836,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n \n-\t/* Different set of flags may produce different results */\n-\tif (dir->flags != dir->untracked->dir_flags ||\n-\t /*\n-\t * See treat_directory(), case index_nonexistent. Without\n-\t * this flag, we may need to also cache .git file content\n-\t * for the resolve_gitlink_ref() call, which we don't.\n-\t */\n-\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n-\t /* We don't support collecting ignore files */\n-\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n-\t\t\t DIR_COLLECT_IGNORED)))\n+\t/* We don't support collecting ignore files */\n+\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n+\t\t\tDIR_COLLECT_IGNORED))\n \t\treturn NULL;\n \n \t/*\n@@ -2845,6 +2861,40 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n+\t/* We don't support using or preparing the untracked cache if\n+\t * the current effective flags don't match the configured\n+\t * flags.\n+\t */\n+\tconfigured_dir_flags = configured_default_dir_flags(istate);\n+\tif (dir->flags != configured_dir_flags)\n+\t\treturn NULL;\n+\n+\t/* If the untracked structure we received does not have the same flags\n+\t * as configured, but the configured flags do match the effective flags,\n+\t * then we need to reset / create a new \"untracked\" structure to match\n+\t * the new config.\n+\t * Keeping the saved and used untracked cache in-line with the\n+\t * configuration provides an opportunity for frequent users of\n+\t * \"git status -uall\" to leverage the untracked cache by aligning their\n+\t * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n+\t * \"normal\" as appropriate), where previously this option was\n+\t * incompatible with untracked cache and *consistently* caused\n+\t * surprisingly bad performance (with fscache and fsmonitor enabled) on\n+\t * Windows.\n+\t *\n+\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n+\t * to not be as bound up with the desired output in a given run,\n+\t * and instead iterated through and stored enough information to\n+\t * correctly serve both \"modes\", then users could get peak performance\n+\t * with or without '-uall' regardless of their\n+\t * \"status.showuntrackedfiles\" config.\n+\t */\n+\tif (dir->flags != dir->untracked->dir_flags) {\n+\t\tfree_untracked_cache(istate->untracked);\n+\t\tnew_untracked_cache(istate, configured_dir_flags);\n+\t\tdir->untracked = istate->untracked;\n+\t}\n+\n \tif (!dir->untracked->root)\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n \n@@ -2916,7 +2966,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\nbase-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n-- \ngitgitgadget\n"},{"id":"449696","messageId":"CAPMMpojQa05yq6ePuK0T8GKhsULPzv-kdjC6DUDpaED9gU17_g@mail.gmail.com","threadId":"55990","inReplyTo":"pull.985.v3.git.1645960973798.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-02-27T14:39:20Z","receivedAt":"2022-02-27T14:39:37Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Bad submission, my apologies - commit message is right but code\nchanges are missing. New one coming soon.\n\nOn Sun, Feb 27, 2022 at 12:22 PM Tao Klerks via GitGitGadget\n wrote:\n>\n> From: Tao Klerks \n>\n> Untracked cache was originally designed to only work with\n> '-untracked-files=normal', but this causes performance issues for UI\n> tooling that wants to see \"all\" on a frequent basis. On the other hand,\n> the conditions that prevented applicability to the \"all\" mode no\n> longer seem to apply.\n>\n> The disqualification of untracked cache has a particularly significant\n> impact on Windows with the defaulted fscache, where the introduction\n> of fsmonitor can make the first and costly directory-iteration happen\n> in untracked file detection, single-threaded, rather than in\n> preload-index on multiple threads. Improving the performance of a\n> \"normal\" 'git status' run with fsmonitor can make\n> 'git status --untracked-files=all' perform much worse.\n>\n> To partially address this, align the supported directory flags for the\n> stored untracked cache data with the git config. If a user specifies\n> an '--untracked-files=' commandline parameter that does not align with\n> their 'status.showuntrackedfiles' config value, then the untracked\n> cache will be ignored - as it is for other unsupported situations like\n> when a pathspec is specified.\n>\n> If the previously stored flags no longer match the current\n> configuration, but the currently-applicable flags do match the current\n> configuration, then discard the previously stored untracked cache\n> data.\n>\n> For most users there will be no change in behavior. Users who need\n> '--untracked-files=all' to perform well will now have the option of\n> setting \"status.showuntrackedfiles\" to \"all\" for better / more\n> consistent performance.\n>\n> Users who need '--untracked-files=all' to perform well for their\n> tooling AND prefer to avoid the verbosity of \"all\" when running\n> git status explicitly without options... are out of luck for now (no\n> change).\n>\n> Users who have the \"status.showuntrackedfiles\" config set to \"all\"\n> and yet frequently explicitly call\n> 'git status --untracked-files=normal' (and use the untracked cache)\n> are the only ones who will be disadvantaged by this change. Their\n> \"--untracked-files=normal\" calls will, after this change, no longer\n> use the untracked cache.\n>\n> Signed-off-by: Tao Klerks \n> ---\n> Support untracked cache with '--untracked-files=all' if configured\n>\n> Resending this patch from a few months ago (with better standards\n> compliance) - it hasn't seen any comments yet, I would dearly love some\n> eyes on this as the change can make a big difference to hundreds of\n> windows users in my environment (and I'd really rather not start\n> distributing customized git builds!)\n>\n> This patch aims to make it possible for users of the -uall flag to git\n> status, either by preference or by need (eg UI tooling), to benefit from\n> the untracked cache when they set their 'status.showuntrackedfiles'\n> config setting to 'all'. This is very important for large repos in\n> Windows.\n>\n> More detail on the change and context in the commit message, I assume\n> repeating a verbose message here is discouraged.\n>\n> These changes result from a couple of conversations,\n> 81153d02-8e7a-be59-e709-e90cd5906f3a@jeffhostetler.com and\n> CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com>.\n>\n> The test suite passes, but as a n00b I do have questions:\n>\n> * Is there any additional validation that could/should be done to\n> confirm that \"-uall\" untracked data can be cached safely?\n> * It looks safe from following the code\n> * It seems from discussing briefly with Elijah Newren in the thread\n> above that thare are no obvious red flags\n> * Manual testing, explicitly and implicitly through months of use,\n> yields no issues\n> * Is it OK to check the repo configuration in the body of processing?\n> It seems to be a rare pattern\n> * Can anyone think of a way to test this change?\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-985%2FTaoK%2Ftaok-untracked-cache-with-uall-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-985/TaoK/taok-untracked-cache-with-uall-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/985\n>\n> Range-diff vs v2:\n>\n> 1: e2f1ad26c78 ! 1: 49cf90bfab5 untracked-cache: support '--untracked-files=all' if configured\n> @@ Commit message\n> untracked-cache: support '--untracked-files=all' if configured\n>\n> Untracked cache was originally designed to only work with\n> - '-untracked-files=normal', but this is a concern for UI tooling that\n> - wants to see \"all\" on a frequent basis, and the conditions that\n> - prevented applicability to the \"all\" mode no longer seem to apply.\n> + '-untracked-files=normal', but this causes performance issues for UI\n> + tooling that wants to see \"all\" on a frequent basis. On the other hand,\n> + the conditions that prevented applicability to the \"all\" mode no\n> + longer seem to apply.\n>\n> - The disqualification of untracked cache is a particular problem on\n> - Windows with the defaulted fscache, where the introduction of\n> - fsmonitor can make the first and costly directory-iteration happen\n> + The disqualification of untracked cache has a particularly significant\n> + impact on Windows with the defaulted fscache, where the introduction\n> + of fsmonitor can make the first and costly directory-iteration happen\n> in untracked file detection, single-threaded, rather than in\n> preload-index on multiple threads. Improving the performance of a\n> \"normal\" 'git status' run with fsmonitor can make\n> 'git status --untracked-files=all' perform much worse.\n>\n> - In this change, align the supported directory flags for the stored\n> - untracked cache data with the git config. If a user specifies an\n> - '--untracked-files=' commandline parameter that does not align with\n> + To partially address this, align the supported directory flags for the\n> + stored untracked cache data with the git config. If a user specifies\n> + an '--untracked-files=' commandline parameter that does not align with\n> their 'status.showuntrackedfiles' config value, then the untracked\n> cache will be ignored - as it is for other unsupported situations like\n> when a pathspec is specified.\n>\n> If the previously stored flags no longer match the current\n> configuration, but the currently-applicable flags do match the current\n> - configuration, then the previously stored untracked cache data is\n> - discarded.\n> + configuration, then discard the previously stored untracked cache\n> + data.\n>\n> For most users there will be no change in behavior. Users who need\n> - '--untracked-files=all' to perform well will have the option of\n> - setting \"status.showuntrackedfiles\" to \"all\".\n> + '--untracked-files=all' to perform well will now have the option of\n> + setting \"status.showuntrackedfiles\" to \"all\" for better / more\n> + consistent performance.\n>\n> Users who need '--untracked-files=all' to perform well for their\n> tooling AND prefer to avoid the verbosity of \"all\" when running\n> git status explicitly without options... are out of luck for now (no\n> change).\n>\n> - Users who set \"status.showuntrackedfiles\" to \"all\" and yet most\n> - frequently explicitly call 'git status --untracked-files=normal' (and\n> - use the untracked cache) are the only ones who would be disadvantaged\n> - by this change. It seems unlikely there are any such users.\n> + Users who have the \"status.showuntrackedfiles\" config set to \"all\"\n> + and yet frequently explicitly call\n> + 'git status --untracked-files=normal' (and use the untracked cache)\n> + are the only ones who will be disadvantaged by this change. Their\n> + \"--untracked-files=normal\" calls will, after this change, no longer\n> + use the untracked cache.\n>\n> Signed-off-by: Tao Klerks \n>\n>\n>\n> dir.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++-------------\n> 1 file changed, 68 insertions(+), 18 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index d91295f2bcd..e35331d3f71 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2746,13 +2746,33 @@ static void set_untracked_ident(struct untracked_cache *uc)\n> strbuf_addch(&uc->ident, 0);\n> }\n>\n> -static void new_untracked_cache(struct index_state *istate)\n> +static unsigned configured_default_dir_flags(struct index_state *istate)\n> +{\n> + /* This logic is coordinated with the setting of these flags in\n> + * wt-status.c#wt_status_collect_untracked(), and the evaluation\n> + * of the config setting in commit.c#git_status_config()\n> + */\n> + char *status_untracked_files_config_value;\n> + int config_outcome = repo_config_get_string(istate->repo,\n> + \"status.showuntrackedfiles\",\n> + &status_untracked_files_config_value);\n> + if (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n> + return 0;\n> + } else {\n> + /*\n> + * The default, if \"all\" is not set, is \"normal\" - leading us here.\n> + * If the value is \"none\" then it really doesn't matter.\n> + */\n> + return DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> + }\n> +}\n> +\n> +static void new_untracked_cache(struct index_state *istate, unsigned flags)\n> {\n> struct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n> strbuf_init(&uc->ident, 100);\n> uc->exclude_per_dir = \".gitignore\";\n> - /* should be the same flags used by git-status */\n> - uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> + uc->dir_flags = flags;\n> set_untracked_ident(uc);\n> istate->untracked = uc;\n> istate->cache_changed |= UNTRACKED_CHANGED;\n> @@ -2761,11 +2781,13 @@ static void new_untracked_cache(struct index_state *istate)\n> void add_untracked_cache(struct index_state *istate)\n> {\n> if (!istate->untracked) {\n> - new_untracked_cache(istate);\n> + new_untracked_cache(istate,\n> + configured_default_dir_flags(istate));\n> } else {\n> if (!ident_in_untracked(istate->untracked)) {\n> free_untracked_cache(istate->untracked);\n> - new_untracked_cache(istate);\n> + new_untracked_cache(istate,\n> + configured_default_dir_flags(istate));\n> }\n> }\n> }\n> @@ -2781,10 +2803,12 @@ 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> + unsigned configured_dir_flags;\n>\n> if (!dir->untracked)\n> return NULL;\n> @@ -2812,17 +2836,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> if (base_len || (pathspec && pathspec->nr))\n> return NULL;\n>\n> - /* Different set of flags may produce different results */\n> - if (dir->flags != dir->untracked->dir_flags ||\n> - /*\n> - * See treat_directory(), case index_nonexistent. Without\n> - * this flag, we may need to also cache .git file content\n> - * for the resolve_gitlink_ref() call, which we don't.\n> - */\n> - !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n> - /* We don't support collecting ignore files */\n> - (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> - DIR_COLLECT_IGNORED)))\n> + /* We don't support collecting ignore files */\n> + if (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> + DIR_COLLECT_IGNORED))\n> return NULL;\n>\n> /*\n> @@ -2845,6 +2861,40 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> return NULL;\n> }\n>\n> + /* We don't support using or preparing the untracked cache if\n> + * the current effective flags don't match the configured\n> + * flags.\n> + */\n> + configured_dir_flags = configured_default_dir_flags(istate);\n> + if (dir->flags != configured_dir_flags)\n> + return NULL;\n> +\n> + /* If the untracked structure we received does not have the same flags\n> + * as configured, but the configured flags do match the effective flags,\n> + * then we need to reset / create a new \"untracked\" structure to match\n> + * the new config.\n> + * Keeping the saved and used untracked cache in-line with the\n> + * configuration provides an opportunity for frequent users of\n> + * \"git status -uall\" to leverage the untracked cache by aligning their\n> + * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n> + * \"normal\" as appropriate), where previously this option was\n> + * incompatible with untracked cache and *consistently* caused\n> + * surprisingly bad performance (with fscache and fsmonitor enabled) on\n> + * Windows.\n> + *\n> + * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n> + * to not be as bound up with the desired output in a given run,\n> + * and instead iterated through and stored enough information to\n> + * correctly serve both \"modes\", then users could get peak performance\n> + * with or without '-uall' regardless of their\n> + * \"status.showuntrackedfiles\" config.\n> + */\n> + if (dir->flags != dir->untracked->dir_flags) {\n> + free_untracked_cache(istate->untracked);\n> + new_untracked_cache(istate, configured_dir_flags);\n> + dir->untracked = istate->untracked;\n> + }\n> +\n> if (!dir->untracked->root)\n> FLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n>\n> @@ -2916,7 +2966,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>\n> base-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n> --\n> gitgitgadget\n"},{"id":"449697","messageId":"pull.985.v4.git.1645974782256.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v3.git.1645960973798.gitgitgadget@gmail.com","subject":"[PATCH v4] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-27T15:13:02Z","receivedAt":"2022-02-27T15:13:10Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n'-untracked-files=normal', but this causes performance issues for UI\ntooling that wants to see \"all\" on a frequent basis. On the other hand,\nthe conditions that prevented applicability to the \"all\" mode no\nlonger seem to apply.\n\nThe disqualification of untracked cache has a particularly significant\nimpact on Windows with the defaulted fscache, where the introduction\nof fsmonitor can make the first and costly directory-iteration happen\nin untracked file detection, single-threaded, rather than in\npreload-index on multiple threads. Improving the performance of a\n\"normal\" 'git status' run with fsmonitor can make\n'git status --untracked-files=all' perform much worse.\n\nTo partially address this, align the supported directory flags for the\nstored untracked cache data with the git config. If a user specifies\nan '--untracked-files=' commandline parameter that does not align with\ntheir 'status.showuntrackedfiles' config value, then the untracked\ncache will be ignored - as it is for other unsupported situations like\nwhen a pathspec is specified.\n\nIf the previously stored flags no longer match the current\nconfiguration, but the currently-applicable flags do match the current\nconfiguration, then discard the previously stored untracked cache\ndata.\n\nFor most users there will be no change in behavior. Users who need\n'--untracked-files=all' to perform well will now have the option of\nsetting \"status.showuntrackedfiles\" to \"all\" for better / more\nconsistent performance.\n\nUsers who need '--untracked-files=all' to perform well for their\ntooling AND prefer to avoid the verbosity of \"all\" when running\ngit status explicitly without options... are out of luck for now (no\nchange).\n\nUsers who have the \"status.showuntrackedfiles\" config set to \"all\"\nand yet frequently explicitly call\n'git status --untracked-files=normal' (and use the untracked cache)\nare the only ones who will be disadvantaged by this change. Their\n\"--untracked-files=normal\" calls will, after this change, no longer\nuse the untracked cache.\n\nSigned-off-by: Tao Klerks \n---\n Support untracked cache with '--untracked-files=all' if configured\n \n Resending this patch from a few months ago (with better standards\n compliance) - it hasn't seen any comments yet, I would dearly love some\n eyes on this as the change can make a big difference to hundreds of\n windows users in my environment (and I'd really rather not start\n distributing customized git builds!)\n \n This patch aims to make it possible for users of the -uall flag to git\n status, either by preference or by need (eg UI tooling), to benefit from\n the untracked cache when they set their 'status.showuntrackedfiles'\n config setting to 'all'. This is very important for large repos in\n Windows.\n \n More detail on the change and context in the commit message, I assume\n repeating a verbose message here is discouraged.\n \n These changes result from a couple of conversations,\n 81153d02-8e7a-be59-e709-e90cd5906f3a@jeffhostetler.com and\n CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com>.\n \n The test suite passes, but as a n00b I do have questions:\n \n * Is there any additional validation that could/should be done to\n confirm that \"-uall\" untracked data can be cached safely?\n * It looks safe from following the code\n * It seems from discussing briefly with Elijah Newren in the thread\n above that thare are no obvious red flags\n * Manual testing, explicitly and implicitly through months of use,\n yields no issues\n * Is it OK to check the repo configuration in the body of processing?\n It seems to be a rare pattern\n * Can anyone think of a way to test this change?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-985%2FTaoK%2Ftaok-untracked-cache-with-uall-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-985/TaoK/taok-untracked-cache-with-uall-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/985\n\nRange-diff vs v3:\n\n 1: 49cf90bfab5 ! 1: 5da418e5c60 untracked-cache: support '--untracked-files=all' if configured\n @@ dir.c: static void set_untracked_ident(struct untracked_cache *uc)\n }\n \n -static void new_untracked_cache(struct index_state *istate)\n -+static unsigned configured_default_dir_flags(struct index_state *istate)\n ++static unsigned configured_default_dir_flags(struct repository *repo)\n +{\n -+\t/* This logic is coordinated with the setting of these flags in\n ++\t/*\n ++\t * This logic is coordinated with the setting of these flags in\n +\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n +\t * of the config setting in commit.c#git_status_config()\n +\t */\n +\tchar *status_untracked_files_config_value;\n -+\tint config_outcome = repo_config_get_string(istate->repo,\n -+\t\t\t\t\t\t\t\t\"status.showuntrackedfiles\",\n -+\t\t\t\t\t\t\t\t&status_untracked_files_config_value);\n ++\tint config_outcome = repo_config_get_string(repo,\n ++\t\t\t\t\t\t \"status.showuntrackedfiles\",\n ++\t\t\t\t\t\t &status_untracked_files_config_value);\n +\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n +\t\treturn 0;\n +\t} else {\n @@ dir.c: static void new_untracked_cache(struct index_state *istate)\n \tif (!istate->untracked) {\n -\t\tnew_untracked_cache(istate);\n +\t\tnew_untracked_cache(istate,\n -+\t\t\t\tconfigured_default_dir_flags(istate));\n ++\t\t\t\t configured_default_dir_flags(istate->repo));\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n -\t\t\tnew_untracked_cache(istate);\n +\t\t\tnew_untracked_cache(istate,\n -+\t\t\t\t\tconfigured_default_dir_flags(istate));\n ++\t\t\t\t\t configured_default_dir_flags(istate->repo));\n \t\t}\n \t}\n }\n @@ dir.c: void remove_untracked_cache(struct index_state *istate)\n + }\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 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\t struct index_state *istate)\n ++\t\t\t\t\t\t\t int base_len,\n ++\t\t\t\t\t\t\t const struct pathspec *pathspec,\n ++\t\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 -+\tunsigned configured_dir_flags;\n - \n - \tif (!dir->untracked)\n - \t\treturn NULL;\n @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_st\n \t\treturn NULL;\n \t}\n \n -+\t/* We don't support using or preparing the untracked cache if\n ++\t/*\n ++\t * We don't support using or preparing the untracked cache if\n +\t * the current effective flags don't match the configured\n +\t * flags.\n +\t */\n -+\tconfigured_dir_flags = configured_default_dir_flags(istate);\n -+\tif (dir->flags != configured_dir_flags)\n ++\tif (dir->flags != configured_default_dir_flags(istate->repo))\n +\t\treturn NULL;\n +\n -+\t/* If the untracked structure we received does not have the same flags\n ++\t/*\n ++\t * If the untracked structure we received does not have the same flags\n +\t * as configured, but the configured flags do match the effective flags,\n +\t * then we need to reset / create a new \"untracked\" structure to match\n +\t * the new config.\n @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_st\n +\t */\n +\tif (dir->flags != dir->untracked->dir_flags) {\n +\t\tfree_untracked_cache(istate->untracked);\n -+\t\tnew_untracked_cache(istate, configured_dir_flags);\n ++\t\tnew_untracked_cache(istate, dir->flags);\n +\t\tdir->untracked = istate->untracked;\n +\t}\n +\n\n\n dir.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 70 insertions(+), 19 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d91295f2bcd..57a7d42482f 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2746,13 +2746,34 @@ static void set_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n-static void new_untracked_cache(struct index_state *istate)\n+static unsigned configured_default_dir_flags(struct repository *repo)\n+{\n+\t/*\n+\t * This logic is coordinated with the setting of these flags in\n+\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n+\t * of the config setting in commit.c#git_status_config()\n+\t */\n+\tchar *status_untracked_files_config_value;\n+\tint config_outcome = repo_config_get_string(repo,\n+\t\t\t\t\t\t \"status.showuntrackedfiles\",\n+\t\t\t\t\t\t &status_untracked_files_config_value);\n+\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n+\t\treturn 0;\n+\t} else {\n+\t\t/*\n+\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n+\t\t * If the value is \"none\" then it really doesn't matter.\n+\t\t */\n+\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\t}\n+}\n+\n+static void new_untracked_cache(struct index_state *istate, unsigned flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n-\t/* should be the same flags used by git-status */\n-\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tuc->dir_flags = flags;\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n@@ -2761,11 +2782,13 @@ static void new_untracked_cache(struct index_state *istate)\n void add_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n-\t\tnew_untracked_cache(istate);\n+\t\tnew_untracked_cache(istate,\n+\t\t\t\t configured_default_dir_flags(istate->repo));\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate);\n+\t\t\tnew_untracked_cache(istate,\n+\t\t\t\t\t configured_default_dir_flags(istate->repo));\n \t\t}\n \t}\n }\n@@ -2780,8 +2803,9 @@ void remove_untracked_cache(struct index_state *istate)\n }\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\t int base_len,\n+\t\t\t\t\t\t\t const struct pathspec *pathspec,\n+\t\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@@ -2812,17 +2836,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n \n-\t/* Different set of flags may produce different results */\n-\tif (dir->flags != dir->untracked->dir_flags ||\n-\t /*\n-\t * See treat_directory(), case index_nonexistent. Without\n-\t * this flag, we may need to also cache .git file content\n-\t * for the resolve_gitlink_ref() call, which we don't.\n-\t */\n-\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n-\t /* We don't support collecting ignore files */\n-\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n-\t\t\t DIR_COLLECT_IGNORED)))\n+\t/* We don't support collecting ignore files */\n+\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n+\t\t\tDIR_COLLECT_IGNORED))\n \t\treturn NULL;\n \n \t/*\n@@ -2845,6 +2861,41 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n+\t/*\n+\t * We don't support using or preparing the untracked cache if\n+\t * the current effective flags don't match the configured\n+\t * flags.\n+\t */\n+\tif (dir->flags != configured_default_dir_flags(istate->repo))\n+\t\treturn NULL;\n+\n+\t/*\n+\t * If the untracked structure we received does not have the same flags\n+\t * as configured, but the configured flags do match the effective flags,\n+\t * then we need to reset / create a new \"untracked\" structure to match\n+\t * the new config.\n+\t * Keeping the saved and used untracked cache in-line with the\n+\t * configuration provides an opportunity for frequent users of\n+\t * \"git status -uall\" to leverage the untracked cache by aligning their\n+\t * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n+\t * \"normal\" as appropriate), where previously this option was\n+\t * incompatible with untracked cache and *consistently* caused\n+\t * surprisingly bad performance (with fscache and fsmonitor enabled) on\n+\t * Windows.\n+\t *\n+\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n+\t * to not be as bound up with the desired output in a given run,\n+\t * and instead iterated through and stored enough information to\n+\t * correctly serve both \"modes\", then users could get peak performance\n+\t * with or without '-uall' regardless of their\n+\t * \"status.showuntrackedfiles\" config.\n+\t */\n+\tif (dir->flags != dir->untracked->dir_flags) {\n+\t\tfree_untracked_cache(istate->untracked);\n+\t\tnew_untracked_cache(istate, dir->flags);\n+\t\tdir->untracked = istate->untracked;\n+\t}\n+\n \tif (!dir->untracked->root)\n \t\tFLEX_ALLOC_STR(dir->untracked->root, name, \"\");\n \n@@ -2916,7 +2967,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\nbase-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84\n-- \ngitgitgadget\n"},{"id":"449706","messageId":"xmqqbkys3x21.fsf@gitster.g","threadId":"55990","inReplyTo":"CAPMMpoixi3x1PHrSHJPV1GRBzMpuOQ4meMr-fipXuDvz-96MEA@mail.gmail.com","subject":"Re: [PATCH v2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-27T19:54:14Z","receivedAt":"2022-02-27T19:54:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks writes:\n\n>> > + configured_dir_flags = configured_default_dir_flags(istate);\n>> > + if (dir->flags != configured_dir_flags)\n>> > + return NULL;\n>>\n>> Hmph. If this weren't necessary, this function does not need to\n>> call configured_default_dir_flags(), and it can lose the\n>> configured_dir_flags variable, too. Which means that\n>> new_untracked_cache() function does not need to take the flags word\n>> as a caller-supplied parameter. Instead, it can make a call to\n>> configured_dir_flags() and assign the result to uc->dir_flags\n>> itself, which would have been much nicer.\n>\n> I've tightened this up a little with an inline call to\n> configured_default_dir_flags(), getting rid of the variable, let's see\n> if that makes more sense / is cleaner.\n\nThe extra variable does not bother me at all. Leaving the room for\nthe caller to screw up and pass an incorrect configred_dir_flags is\nwhat disturbs me.\n\nIf this caller needs to call configred_default_dir_flags(istate),\nthen we cannot avoid it. And the extra variable needed to call the\nfunction only once, store its result, and use that result twice, is\nperfectly a good thing to have. We don't want to see inline or\nanything tricky.\n\nThanks.\n"},{"id":"449761","messageId":"220228.86y21v3wyj.gmgdl@evledraar.gmail.com","threadId":"55990","inReplyTo":"pull.985.v4.git.1645974782256.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] untracked-cache: support '--untracked-files=all' if configured","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-28T14:02:24Z","receivedAt":"2022-02-28T14:08:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 27 2022, Tao Klerks via GitGitGadget wrote:\n\n> From: Tao Klerks \n> [...]\n> The test suite passes, but as a n00b I do have questions:\n> \n> * Is there any additional validation that could/should be done to\n> confirm that \"-uall\" untracked data can be cached safely?\n\nI haven't given this any substantial review, sorry.\n\n> * It looks safe from following the code\n\nYes, at a glance it looks safe to me.\n\n> * It seems from discussing briefly with Elijah Newren in the thread\n> above that thare are no obvious red flags\n> * Manual testing, explicitly and implicitly through months of use,\n> yields no issues\n> * Is it OK to check the repo configuration in the body of processing?\n> It seems to be a rare pattern\n\nYes, it's not very common, but it's fine.\n\n> * Can anyone think of a way to test this change?\n\nWell, if we set \"flags\" to 0 in your new helper the existing tests will\nfail, so that's something at least.\n\n> -static void new_untracked_cache(struct index_state *istate)\n> +static unsigned configured_default_dir_flags(struct repository *repo)\n> +{\n> +\t/*\n> +\t * This logic is coordinated with the setting of these flags in\n> +\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n> +\t * of the config setting in commit.c#git_status_config()\n> +\t */\n> +\tchar *status_untracked_files_config_value;\n> +\tint config_outcome = repo_config_get_string(repo,\n> +\t\t\t\t\t\t \"status.showuntrackedfiles\",\n> +\t\t\t\t\t\t &status_untracked_files_config_value);\n> +\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n> +\t\treturn 0;\n> +\t} else {\n> +\t\t/*\n> +\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n> +\t\t * If the value is \"none\" then it really doesn't matter.\n> +\t\t */\n> +\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> +\t}\n> +}\n> +\n> [...]\n\nIn reviewing this I found the addition of very long lines & indentation\nmade this a bit harder to read. I came up with the below which should be\nsquashable on top, and perhaps makes this easier to read.\n\nI.e. the one caller that needs custom flags passes them, others pass -1\nnow.\n\nFor throwaway \"char *\" variables we usually use \"char *p\", \"char *val\"\nor whatever. A \"status_untracked_files_config_value\" is a bit much for\nsomething function local whose body is <10 lines of code.\n\nAnd if you drop the \"int config_outcome\" + rename the variable the\nrepo_config_get_string() fits on one line:\n\ndiff --git a/dir.c b/dir.c\nindex 57a7d42482f..22a27d7780f 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2746,34 +2746,33 @@ static void set_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n-static unsigned configured_default_dir_flags(struct repository *repo)\n+static unsigned new_untracked_cache_flags(struct index_state *istate)\n {\n+\tstruct repository *repo = istate->repo;\n+\tchar *val;\n+\n \t/*\n \t * This logic is coordinated with the setting of these flags in\n \t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n \t * of the config setting in commit.c#git_status_config()\n \t */\n-\tchar *status_untracked_files_config_value;\n-\tint config_outcome = repo_config_get_string(repo,\n-\t\t\t\t\t\t \"status.showuntrackedfiles\",\n-\t\t\t\t\t\t &status_untracked_files_config_value);\n-\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n+\tif (!repo_config_get_string(repo, \"status.showuntrackedfiles\", &val) &&\n+\t !strcmp(val, \"all\"))\n \t\treturn 0;\n-\t} else {\n-\t\t/*\n-\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n-\t\t * If the value is \"none\" then it really doesn't matter.\n-\t\t */\n-\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n-\t}\n+\n+\t/*\n+\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n+\t * If the value is \"none\" then it really doesn't matter.\n+\t */\n+\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n }\n \n-static void new_untracked_cache(struct index_state *istate, unsigned flags)\n+static void new_untracked_cache(struct index_state *istate, int flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n-\tuc->dir_flags = flags;\n+\tuc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate);\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n@@ -2782,13 +2781,11 @@ static void new_untracked_cache(struct index_state *istate, unsigned flags)\n void add_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n-\t\tnew_untracked_cache(istate,\n-\t\t\t\t configured_default_dir_flags(istate->repo));\n+\t\tnew_untracked_cache(istate, -1);\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate,\n-\t\t\t\t\t configured_default_dir_flags(istate->repo));\n+\t\t\tnew_untracked_cache(istate, -1);\n \t\t}\n \t}\n }\n@@ -2866,7 +2863,7 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t * the current effective flags don't match the configured\n \t * flags.\n \t */\n-\tif (dir->flags != configured_default_dir_flags(istate->repo))\n+\tif (dir->flags != new_untracked_cache_flags(istate))\n \t\treturn NULL;\n \n \t/*\n"},{"id":"450067","messageId":"CAPMMpohqPa3bqnPLKJ851JJFQhbF_ZA+hAGuE_+0z+BMF2yz0Q@mail.gmail.com","threadId":"55990","inReplyTo":"220228.86y21v3wyj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-02T08:47:22Z","receivedAt":"2022-03-02T08:47:41Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Feb 28, 2022 at 3:08 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> In reviewing this I found the addition of very long lines & indentation\n> made this a bit harder to read. I came up with the below which should be\n> squashable on top, and perhaps makes this easier to read.\n>\n> I.e. the one caller that needs custom flags passes them, others pass -1\n> now.\n>\n> For throwaway \"char *\" variables we usually use \"char *p\", \"char *val\"\n> or whatever. A \"status_untracked_files_config_value\" is a bit much for\n> something function local whose body is <10 lines of code.\n>\n> And if you drop the \"int config_outcome\" + rename the variable the\n> repo_config_get_string() fits on one line:\n>\n\nThank you, this looks much cleaner!\n\nOne question I have about this patch itself is why you changed the arg to\nnew_untracked_cache_flags / configured_default_dir_flags from \"repo\"\nback to \"istate\" / whether that was intentional; I had understood Junio's\nrecommendation that the function get the minimum it needed (the repo\nin this case) as very sensible... especially now that we need to pass in\nthat state less often.\n\nAnother question is whether I should add a comment in\nnew_untracked_cache_flags explaining the new meaning of \"-1\" for\nthe flags argument, or whether this is sufficiently standard in this\nproject or in C generally that I should leave it implied.\n\nA third question is whether you have an opinion on the better approach\nto getting these changes merged: does it make more sense to argue\nthat this is an improvement overall, and that the case of configured\n\"all\" and argument-provided \"normal\", which will now have worse\nperformance, can and should be ignored, *or* will it be easier to get\nthis reviewed/merged if the new behavior is made conditional upon a\nnew configuration key?\n\nLast question is about protocol/workflow in this project: When I squash\nyour proposal in, should I add an extra\n\"Signed-Off-By: Ævar Arnfjörð Bjarmason \" line,\nor just include the changes with mine?? (I've read SubmittingPatches\nbut don't really understand the whole signing off business; I do\nunderstand that as your changes improve mine directly, they should\nindeed be \"squashed\" in, rather than added as an extra commit)\n"},{"id":"452551","messageId":"98a4d8f35c5abfcda5590f7e511f7b576000fd5a.1648553134.git.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v5.git.1648553134.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] untracked-cache: test untracked-cache-bypassing behavior with -uall","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-29T11:25:33Z","receivedAt":"2022-03-29T11:25:49Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n'--untracked-files=normal', and it gets ignored when\n'--untracked-files=all' is specified instead.\n\nAdd explicit tests for this behavior.\n\nSigned-off-by: Tao Klerks \n---\n t/t7063-status-untracked-cache.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex ca90ee805e7..c44b70b96e4 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -190,6 +190,36 @@ test_expect_success 'untracked cache after second status' '\n \ttest_cmp ../dump.expect ../actual\n '\n \n+cat >../status_uall.expect <../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status -uall --porcelain >../actual &&\n+\tiuc status -uall --porcelain >../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <../actual &&\n+\ttest_cmp ../dump.expect ../actual\n+'\n+\n test_expect_success 'modify in root directory, one dir invalidation' '\n \t: >four &&\n \ttest-tool chmtime =-240 four &&\n-- \ngitgitgadget\n\n"},{"id":"452552","messageId":"pull.985.v5.git.1648553134.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v4.git.1645974782256.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] Support untracked cache with '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-29T11:25:32Z","receivedAt":"2022-03-29T11:25:52Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Make it possible for users of the -uall flag to git status, either by\npreference or by need (eg UI tooling), to benefit from the untracked cache\nwhen they set their 'status.showuntrackedfiles' config setting to 'all'.\nThis is especially useful for large repos in Windows, where without\nuntracked cache \"git status\" times can easily be 5x higher, and GUI tooling\ntypically does use -uall.\n\nIn this fifth version, split the change into two patches - one to introduce\ntests of the current untracked-cache-skipping behavior when -uall is\nspecified, and then new tests checking the new behavior with\n'status.showuntrackedfiles=all'.\n\nMy two remaining questions with respect to this patchset are:\n\n 1. Does it make sense to do this as a simple enhancement as proposed here,\n or would people be more comfortable with a new configuration option,\n given the potential for worse performance under specific (and. I\n believe, vanishingly rare) circumstances?\n 2. If it makes sense to envision a future where a single untracked cache\n structure can support both -uall and -unormal, where should this\n possible future be alluded to, if anywhere?\n\nTao Klerks (2):\n untracked-cache: test untracked-cache-bypassing behavior with -uall\n untracked-cache: support '--untracked-files=all' if configured\n\n dir.c | 85 +++++++++++++++++------\n t/t7063-status-untracked-cache.sh | 108 ++++++++++++++++++++++++++++++\n 2 files changed, 174 insertions(+), 19 deletions(-)\n\n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-985%2FTaoK%2Ftaok-untracked-cache-with-uall-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-985/TaoK/taok-untracked-cache-with-uall-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/985\n\nRange-diff vs v4:\n\n -: ----------- > 1: 98a4d8f35c5 untracked-cache: test untracked-cache-bypassing behavior with -uall\n 1: 5da418e5c60 ! 2: f60d2c6e36c untracked-cache: support '--untracked-files=all' if configured\n @@ Commit message\n untracked-cache: support '--untracked-files=all' if configured\n \n Untracked cache was originally designed to only work with\n - '-untracked-files=normal', but this causes performance issues for UI\n + '--untracked-files=normal', but this causes performance issues for UI\n tooling that wants to see \"all\" on a frequent basis. On the other hand,\n the conditions that prevented applicability to the \"all\" mode no\n longer seem to apply.\n @@ dir.c: static void set_untracked_ident(struct untracked_cache *uc)\n }\n \n -static void new_untracked_cache(struct index_state *istate)\n -+static unsigned configured_default_dir_flags(struct repository *repo)\n ++static unsigned new_untracked_cache_flags(struct index_state *istate)\n +{\n ++\tstruct repository *repo = istate->repo;\n ++\tchar *val;\n ++\n +\t/*\n +\t * This logic is coordinated with the setting of these flags in\n +\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n +\t * of the config setting in commit.c#git_status_config()\n +\t */\n -+\tchar *status_untracked_files_config_value;\n -+\tint config_outcome = repo_config_get_string(repo,\n -+\t\t\t\t\t\t \"status.showuntrackedfiles\",\n -+\t\t\t\t\t\t &status_untracked_files_config_value);\n -+\tif (!config_outcome && !strcmp(status_untracked_files_config_value, \"all\")) {\n ++\tif (!repo_config_get_string(repo, \"status.showuntrackedfiles\", &val) &&\n ++\t !strcmp(val, \"all\"))\n +\t\treturn 0;\n -+\t} else {\n -+\t\t/*\n -+\t\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n -+\t\t * If the value is \"none\" then it really doesn't matter.\n -+\t\t */\n -+\t\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n -+\t}\n ++\n ++\t/*\n ++\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n ++\t * If the value is \"none\" then it really doesn't matter.\n ++\t */\n ++\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n +}\n +\n -+static void new_untracked_cache(struct index_state *istate, unsigned flags)\n ++static void new_untracked_cache(struct index_state *istate, int flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n -\t/* should be the same flags used by git-status */\n -\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n -+\tuc->dir_flags = flags;\n ++\tuc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate);\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n @@ dir.c: static void new_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n -\t\tnew_untracked_cache(istate);\n -+\t\tnew_untracked_cache(istate,\n -+\t\t\t\t configured_default_dir_flags(istate->repo));\n ++\t\tnew_untracked_cache(istate, -1);\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n -\t\t\tnew_untracked_cache(istate);\n -+\t\t\tnew_untracked_cache(istate,\n -+\t\t\t\t\t configured_default_dir_flags(istate->repo));\n ++\t\t\tnew_untracked_cache(istate, -1);\n \t\t}\n \t}\n }\n @@ dir.c: 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 +\t\t\t\t\t\t\t int base_len,\n +\t\t\t\t\t\t\t const struct pathspec *pathspec,\n +\t\t\t\t\t\t\t struct index_state *istate)\n @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_st\n +\t * the current effective flags don't match the configured\n +\t * flags.\n +\t */\n -+\tif (dir->flags != configured_default_dir_flags(istate->repo))\n ++\tif (dir->flags != new_untracked_cache_flags(istate))\n +\t\treturn NULL;\n +\n +\t/*\n @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_st\n +\t\tdir->untracked = istate->untracked;\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 +\n + ## t/t7063-status-untracked-cache.sh ##\n +@@ t/t7063-status-untracked-cache.sh: test_expect_success 'untracked cache remains after bypass' '\n + \ttest_cmp ../dump.expect ../actual\n + '\n \n -@@ dir.c: 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 ++test_expect_success 'if -uall is configured, untracked cache gets populated by default' '\n ++\ttest_config status.showuntrackedfiles all &&\n ++\t: >../trace.output &&\n ++\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n ++\tgit status --porcelain >../actual &&\n ++\tiuc status --porcelain >../status.iuc &&\n ++\ttest_cmp ../status_uall.expect ../status.iuc &&\n ++\ttest_cmp ../status_uall.expect ../actual &&\n ++\tget_relevant_traces ../trace.output ../trace.relevant &&\n ++\tcat >../trace.expect <../dump_uall.expect <../actual &&\n ++\ttest_cmp ../dump_uall.expect ../actual\n ++'\n ++\n ++test_expect_success 'if -uall is configured, untracked cache is used by default' '\n ++\ttest_config status.showuntrackedfiles all &&\n ++\t: >../trace.output &&\n ++\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n ++\tgit status --porcelain >../actual &&\n ++\tiuc status --porcelain >../status.iuc &&\n ++\ttest_cmp ../status_uall.expect ../status.iuc &&\n ++\ttest_cmp ../status_uall.expect ../actual &&\n ++\tget_relevant_traces ../trace.output ../trace.relevant &&\n ++\tcat >../trace.expect <../trace.output &&\n ++\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n ++\tgit status -unormal --porcelain >../actual &&\n ++\tiuc status -unormal --porcelain >../status.iuc &&\n ++\ttest_cmp ../status.expect ../status.iuc &&\n ++\ttest_cmp ../status.expect ../actual &&\n ++\tget_relevant_traces ../trace.output ../trace.relevant &&\n ++\tcat >../trace.expect <four &&\n + \ttest-tool chmtime =-240 four &&\n\n-- \ngitgitgadget\n"},{"id":"452553","messageId":"f60d2c6e36c3218f9b19d7ce62a090d7d6e0e7f6.1648553134.git.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v5.git.1648553134.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-29T11:25:34Z","receivedAt":"2022-03-29T11:25:54Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n'--untracked-files=normal', but this causes performance issues for UI\ntooling that wants to see \"all\" on a frequent basis. On the other hand,\nthe conditions that prevented applicability to the \"all\" mode no\nlonger seem to apply.\n\nThe disqualification of untracked cache has a particularly significant\nimpact on Windows with the defaulted fscache, where the introduction\nof fsmonitor can make the first and costly directory-iteration happen\nin untracked file detection, single-threaded, rather than in\npreload-index on multiple threads. Improving the performance of a\n\"normal\" 'git status' run with fsmonitor can make\n'git status --untracked-files=all' perform much worse.\n\nTo partially address this, align the supported directory flags for the\nstored untracked cache data with the git config. If a user specifies\nan '--untracked-files=' commandline parameter that does not align with\ntheir 'status.showuntrackedfiles' config value, then the untracked\ncache will be ignored - as it is for other unsupported situations like\nwhen a pathspec is specified.\n\nIf the previously stored flags no longer match the current\nconfiguration, but the currently-applicable flags do match the current\nconfiguration, then discard the previously stored untracked cache\ndata.\n\nFor most users there will be no change in behavior. Users who need\n'--untracked-files=all' to perform well will now have the option of\nsetting \"status.showuntrackedfiles\" to \"all\" for better / more\nconsistent performance.\n\nUsers who need '--untracked-files=all' to perform well for their\ntooling AND prefer to avoid the verbosity of \"all\" when running\ngit status explicitly without options... are out of luck for now (no\nchange).\n\nUsers who have the \"status.showuntrackedfiles\" config set to \"all\"\nand yet frequently explicitly call\n'git status --untracked-files=normal' (and use the untracked cache)\nare the only ones who will be disadvantaged by this change. Their\n\"--untracked-files=normal\" calls will, after this change, no longer\nuse the untracked cache.\n\nSigned-off-by: Tao Klerks \n---\n dir.c | 85 ++++++++++++++++++++++++-------\n t/t7063-status-untracked-cache.sh | 78 ++++++++++++++++++++++++++++\n 2 files changed, 144 insertions(+), 19 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex f2b0f242101..c5b513ddd2a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2747,13 +2747,33 @@ static void set_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n-static void new_untracked_cache(struct index_state *istate)\n+static unsigned new_untracked_cache_flags(struct index_state *istate)\n+{\n+\tstruct repository *repo = istate->repo;\n+\tchar *val;\n+\n+\t/*\n+\t * This logic is coordinated with the setting of these flags in\n+\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n+\t * of the config setting in commit.c#git_status_config()\n+\t */\n+\tif (!repo_config_get_string(repo, \"status.showuntrackedfiles\", &val) &&\n+\t !strcmp(val, \"all\"))\n+\t\treturn 0;\n+\n+\t/*\n+\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n+\t * If the value is \"none\" then it really doesn't matter.\n+\t */\n+\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+}\n+\n+static void new_untracked_cache(struct index_state *istate, int flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n-\t/* should be the same flags used by git-status */\n-\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tuc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate);\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n@@ -2762,11 +2782,11 @@ static void new_untracked_cache(struct index_state *istate)\n void add_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n-\t\tnew_untracked_cache(istate);\n+\t\tnew_untracked_cache(istate, -1);\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate);\n+\t\t\tnew_untracked_cache(istate, -1);\n \t\t}\n \t}\n }\n@@ -2781,9 +2801,9 @@ void remove_untracked_cache(struct index_state *istate)\n }\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 struct index_state *istate)\n+\t\t\t\t\t\t\t int base_len,\n+\t\t\t\t\t\t\t const struct pathspec *pathspec,\n+\t\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@@ -2814,17 +2834,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n \n-\t/* Different set of flags may produce different results */\n-\tif (dir->flags != dir->untracked->dir_flags ||\n-\t /*\n-\t * See treat_directory(), case index_nonexistent. Without\n-\t * this flag, we may need to also cache .git file content\n-\t * for the resolve_gitlink_ref() call, which we don't.\n-\t */\n-\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n-\t /* We don't support collecting ignore files */\n-\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n-\t\t\t DIR_COLLECT_IGNORED)))\n+\t/* We don't support collecting ignore files */\n+\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n+\t\t\tDIR_COLLECT_IGNORED))\n \t\treturn NULL;\n \n \t/*\n@@ -2847,6 +2859,41 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n+\t/*\n+\t * We don't support using or preparing the untracked cache if\n+\t * the current effective flags don't match the configured\n+\t * flags.\n+\t */\n+\tif (dir->flags != new_untracked_cache_flags(istate))\n+\t\treturn NULL;\n+\n+\t/*\n+\t * If the untracked structure we received does not have the same flags\n+\t * as configured, but the configured flags do match the effective flags,\n+\t * then we need to reset / create a new \"untracked\" structure to match\n+\t * the new config.\n+\t * Keeping the saved and used untracked cache in-line with the\n+\t * configuration provides an opportunity for frequent users of\n+\t * \"git status -uall\" to leverage the untracked cache by aligning their\n+\t * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n+\t * \"normal\" as appropriate), where previously this option was\n+\t * incompatible with untracked cache and *consistently* caused\n+\t * surprisingly bad performance (with fscache and fsmonitor enabled) on\n+\t * Windows.\n+\t *\n+\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n+\t * to not be as bound up with the desired output in a given run,\n+\t * and instead iterated through and stored enough information to\n+\t * correctly serve both \"modes\", then users could get peak performance\n+\t * with or without '-uall' regardless of their\n+\t * \"status.showuntrackedfiles\" config.\n+\t */\n+\tif (dir->flags != dir->untracked->dir_flags) {\n+\t\tfree_untracked_cache(istate->untracked);\n+\t\tnew_untracked_cache(istate, dir->flags);\n+\t\tdir->untracked = istate->untracked;\n+\t}\n+\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, \"\");\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex c44b70b96e4..b52fd937156 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -220,6 +220,84 @@ test_expect_success 'untracked cache remains after bypass' '\n \ttest_cmp ../dump.expect ../actual\n '\n \n+test_expect_success 'if -uall is configured, untracked cache gets populated by default' '\n+\ttest_config status.showuntrackedfiles all &&\n+\t: >../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status --porcelain >../actual &&\n+\tiuc status --porcelain >../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <../dump_uall.expect <../actual &&\n+\ttest_cmp ../dump_uall.expect ../actual\n+'\n+\n+test_expect_success 'if -uall is configured, untracked cache is used by default' '\n+\ttest_config status.showuntrackedfiles all &&\n+\t: >../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status --porcelain >../actual &&\n+\tiuc status --porcelain >../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status -unormal --porcelain >../actual &&\n+\tiuc status -unormal --porcelain >../status.iuc &&\n+\ttest_cmp ../status.expect ../status.iuc &&\n+\ttest_cmp ../status.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <four &&\n \ttest-tool chmtime =-240 four &&\n-- \ngitgitgadget\n"},{"id":"452584","messageId":"xmqq5ynwwvli.fsf@gitster.g","threadId":"55990","inReplyTo":"98a4d8f35c5abfcda5590f7e511f7b576000fd5a.1648553134.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 1/2] untracked-cache: test untracked-cache-bypassing behavior with -uall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-29T16:51:21Z","receivedAt":"2022-03-29T16:51:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" writes:\n\n> +# Bypassing the untracked cache here is not desirable, but it expected\n> +# in the current implementation\n\nIf that is the case, it is much more desirable to squash it into a\nsingle [2/2] patch so that the desirable working is documented (so\nfuture breakage can be caught), the reviewers can read what the\nintended behaviour is more easily (so we do not have to be confused\nby this one saying \"expect success\"), make it easier to cherry-pick\nthe fix and test in the same patch elsewhere, and the existing\nbreakage can easily be caught by applying only the test part of the\npatch.\n\nThanks.\n\n> +test_expect_success 'untracked cache is bypassed with -uall' '\n> +\t: >../trace.output &&\n> +\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n> +\tgit status -uall --porcelain >../actual &&\n> +\tiuc status -uall --porcelain >../status.iuc &&\n> +\ttest_cmp ../status_uall.expect ../status.iuc &&\n> +\ttest_cmp ../status_uall.expect ../actual &&\n> +\tget_relevant_traces ../trace.output ../trace.relevant &&\n> +\tcat >../trace.expect < + ....path:\n> +EOF\n> +\ttest_cmp ../trace.expect ../trace.relevant\n> +'\n> +\n> +test_expect_success 'untracked cache remains after bypass' '\n> +\ttest-tool dump-untracked-cache >../actual &&\n> +\ttest_cmp ../dump.expect ../actual\n> +'\n> +\n> test_expect_success 'modify in root directory, one dir invalidation' '\n> \t: >four &&\n> \ttest-tool chmtime =-240 four &&\n"},{"id":"452594","messageId":"xmqqsfr0u01k.fsf@gitster.g","threadId":"55990","inReplyTo":"f60d2c6e36c3218f9b19d7ce62a090d7d6e0e7f6.1648553134.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-29T17:43:35Z","receivedAt":"2022-03-29T17:43:43Z","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> Untracked cache was originally designed to only work with\n> '--untracked-files=normal', but this causes performance issues for UI\n> tooling that wants to see \"all\" on a frequent basis. On the other hand,\n> the conditions that prevented applicability to the \"all\" mode no\n> longer seem to apply.\n\nThere is a slight leap in logic flow before \", but this causes\" that\nforces readers read the above twice and guess what is missing. I am\nguessing that\n\n ... was designed to only work with \"--untracked-files=normal\",\n and is bypassed when \"--untracked-files=all\" request, but this\n causes ...\n\nis what you meant to say.\n\nThe claim in the last sentence \"... no longer seem to apply\" is\nthrown at the readers without much rationale. Either justify it\nmore solidly or discard the claim?\n\n> The disqualification of untracked cache has a particularly significant\n> impact on Windows with the defaulted fscache, where the introduction\n> of fsmonitor can make the first and costly directory-iteration happen\n> in untracked file detection, single-threaded, rather than in\n> preload-index on multiple threads. Improving the performance of a\n> \"normal\" 'git status' run with fsmonitor can make\n> 'git status --untracked-files=all' perform much worse.\n\nThe last sentence is a bit hard to parse and very hard to reason\nabout. Do you mean to say \"--untracked-files=all is slower when the\nuntracked cache is in use, so the performance of 'git status' may be\nimproved for '--untracked-files=normal' when the untracked cache is\nused, it hurts when 'git status --untracked-files=all' is run\"?\n\n> To partially address this, align the supported directory flags for the\n> stored untracked cache data with the git config. If a user specifies\n> an '--untracked-files=' commandline parameter that does not align with\n> their 'status.showuntrackedfiles' config value, then the untracked\n> cache will be ignored - as it is for other unsupported situations like\n> when a pathspec is specified.\n>\n> If the previously stored flags no longer match the current\n> configuration, but the currently-applicable flags do match the current\n> configuration, then discard the previously stored untracked cache\n> data.\n\nLet me follow what actually happens in the patch aloud.\n\n> -static void new_untracked_cache(struct index_state *istate)\n> +static unsigned new_untracked_cache_flags(struct index_state *istate)\n> +{\n> +\tstruct repository *repo = istate->repo;\n> +\tchar *val;\n> +\n> +\t/*\n> +\t * This logic is coordinated with the setting of these flags in\n> +\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n> +\t * of the config setting in commit.c#git_status_config()\n> +\t */\n> +\tif (!repo_config_get_string(repo, \"status.showuntrackedfiles\", &val) &&\n> +\t !strcmp(val, \"all\"))\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n> +\t * If the value is \"none\" then it really doesn't matter.\n> +\t */\n> +\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> +}\n\nThis _guesses_ the user preference from the configuration.\n\n> +static void new_untracked_cache(struct index_state *istate, int flags)\n> {\n> \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n> \tstrbuf_init(&uc->ident, 100);\n> \tuc->exclude_per_dir = \".gitignore\";\n> -\t/* should be the same flags used by git-status */\n> -\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> +\tuc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate);\n\nWe use unsigned for the flag word unless there is a reason not to,\nbut this function wants to use top-bit as a signal to \"guess from\nconfig\". The caller either dictates what bits to set, or we guess.\n\nAnd the created uc records the flags.\n\n> @@ -2762,11 +2782,11 @@ static void new_untracked_cache(struct index_state *istate)\n> void add_untracked_cache(struct index_state *istate)\n> {\n> \tif (!istate->untracked) {\n> -\t\tnew_untracked_cache(istate);\n> +\t\tnew_untracked_cache(istate, -1);\n\nWe are newly creating, so \"-1\" (guess from config) may be the most\nappropriate (unless the caller of add_untracked_cache() already\nknows what operation it is using for, that is).\n\n> \t} else {\n> \t\tif (!ident_in_untracked(istate->untracked)) {\n\nFound an existing one but need to recreate.\n\n> \t\t\tfree_untracked_cache(istate->untracked);\n> -\t\t\tnew_untracked_cache(istate);\n> +\t\t\tnew_untracked_cache(istate, -1);\n\nIn this case, is it likely to give us a better guess to read the\nconfiguration, or copy from the existing untracked file? My gut\nfeeling says it would be the latter, and if that is correct, then\nwe might want\n\n+\t \t\tint old_flags = istate->untracked->dir_flags;\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate);\n+\t\t\tnew_untracked_cache(istate, old_flags);\n\ninstead? I dunno.\n\n> @@ -2781,9 +2801,9 @@ void remove_untracked_cache(struct index_state *istate)\n> }\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 struct index_state *istate)\n> +\t\t\t\t\t\t\t int base_len,\n> +\t\t\t\t\t\t\t const struct pathspec *pathspec,\n> +\t\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\nIs this just re-indenting? Not very welcome, but OK.\n\n> @@ -2814,17 +2834,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> \tif (base_len || (pathspec && pathspec->nr))\n> \t\treturn NULL;\n> \n> -\t/* Different set of flags may produce different results */\n> -\tif (dir->flags != dir->untracked->dir_flags ||\n> -\t /*\n> -\t * See treat_directory(), case index_nonexistent. Without\n> -\t * this flag, we may need to also cache .git file content\n> -\t * for the resolve_gitlink_ref() call, which we don't.\n> -\t */\n> -\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n> -\t /* We don't support collecting ignore files */\n> -\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> -\t\t\t DIR_COLLECT_IGNORED)))\n> +\t/* We don't support collecting ignore files */\n> +\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> +\t\t\tDIR_COLLECT_IGNORED))\n> \t\treturn NULL;\n> \n> \t/*\n> @@ -2847,6 +2859,41 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> \t\treturn NULL;\n> \t}\n> \n> +\t/*\n> +\t * We don't support using or preparing the untracked cache if\n> +\t * the current effective flags don't match the configured\n> +\t * flags.\n> +\t */\n\nIs that what we want? What happens when your user does this:\n\n - set showuntrackedfiles to \"normal\"\n - generate the untracked cache\n - set showuntrackedfiles to \"all\"\n\nAnd now the user wants to make a request that wants to see flags\nthat are suitable for \"normal\".\n\nThe best case would be for the untracked cache to know what flags\nwere in use when it was generated, so that in the above sequence,\neven though the current value of configuration variable and the\ncurrent request from the command line contradict each other, we\ncan notice that the untracked cache data is suitable for the current\nrequest and the right thing happens.\n\n> +\tif (dir->flags != new_untracked_cache_flags(istate))\n> +\t\treturn NULL;\n\nBut this only pays attention to what is in the configuration? It\nseems to me that it is being too pessimistic, but perhaps it is\nnecessary for correctness somehow?\n\nThanks.\n\n"},{"id":"452640","messageId":"CAPMMpoigpEi9mTET1r9sbSdw92kUXw9cRQPdUv-O179j3+_wAQ@mail.gmail.com","threadId":"55990","inReplyTo":"xmqq5ynwwvli.fsf@gitster.g","subject":"Re: [PATCH v5 1/2] untracked-cache: test untracked-cache-bypassing behavior with -uall","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-30T04:46:52Z","receivedAt":"2022-03-30T04:47:11Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, Mar 29, 2022 at 6:51 PM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > +# Bypassing the untracked cache here is not desirable, but it expected\n> > +# in the current implementation\n>\n> If that is the case, it is much more desirable to squash it into a\n> single [2/2] patch so that the desirable working is documented (so\n> future breakage can be caught), the reviewers can read what the\n> intended behaviour is more easily (so we do not have to be confused\n> by this one saying \"expect success\"), make it easier to cherry-pick\n> the fix and test in the same patch elsewhere, and the existing\n> breakage can easily be caught by applying only the test part of the\n> patch.\n\nSorry, I'm not completely sure whether my comment was misleading, or\nwhether I'm misunderstanding your feedback.\n\nThe test added here does not test \"desirable\" behavior from an\nend-user functional perspective, but it does test behavior that is\nworking \"as-designed\" as of many years ago.\n\nThe intent in adding this test is to ensure that if/when someone\nchanges this behavior down the line, they be forced to understand the\nimplications (eg: the current untracked cache structure does not store\nthe same data for a -uall run as for a -unormal run, and so using the\ndata from one in another would lead to output correctness errors).\n\nImportantly, the behavior that this test is exercising *is not\nchanged* by the 2/2 \"improvement\" patch; this is why I separated it\nout - it's a change that I feel we should make either way, and could\neven be its own independent patch. It's in this series as it adds\nrelevant context and I am later adding similar tests for the\nnew/additional behavior.\n\nGiven the (I believe) misunderstanding, a better comment would be\nsomething like:\n\n# Bypassing the untracked cache here is not desirable from an\n# end-user perspective, but is expected in the current implementation.\n# The untracked cache data stored for a -unormal run cannot be\n# correctly used in a -uall run - it would yield incorrect output.\n\nDoes that make more sense?\n"},{"id":"452668","messageId":"xmqqwngb9yzd.fsf@gitster.g","threadId":"55990","inReplyTo":"CAPMMpoigpEi9mTET1r9sbSdw92kUXw9cRQPdUv-O179j3+_wAQ@mail.gmail.com","subject":"Re: [PATCH v5 1/2] untracked-cache: test untracked-cache-bypassing behavior with -uall","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-30T16:39:02Z","receivedAt":"2022-03-30T16:39:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks writes:\n\n> Sorry, I'm not completely sure whether my comment was misleading, or\n> whether I'm misunderstanding your feedback.\n>\n> The test added here does not test \"desirable\" behavior from an\n> end-user functional perspective, but it does test behavior that is\n> working \"as-designed\" as of many years ago.\n\nIs it \"as-designed\", or just \"left buggy\"?\n\nI somehow had an impression that you meant the latter, and it would\nbe our aspirational goal to eventually fix it. And for such case,\nit would be better to write the test body to show what the command \nshould do, which would make the test fail with today's Git, and mark\nit as test_expect_failure (which is not an ideal mechanism to prepare\nfor a future fix, but that is what we have now and should use, until\na better alternative being worked on is finished).\n\nBut if it is \"as-designed, some users may find it suboptimal or\nconfusing or with any other negative adjectives, but it is too late\nto change now and more importantly it is unthinkable to change it\nbecause existing tools and users do depend on the current behaviour\",\nthen what you did is perfectly fine.\n\n> The intent in adding this test is to ensure that if/when someone\n> changes this behavior down the line, they be forced to understand the\n> implications.\n\nThat, too.\n\nThanks.\n"},{"id":"452699","messageId":"CAPMMpog4AcDT_TpPxBaOUfT__gSLufF-EBr=c6__2W4NhhsyVw@mail.gmail.com","threadId":"55990","inReplyTo":"xmqqsfr0u01k.fsf@gitster.g","subject":"Re: [PATCH v5 2/2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-30T19:59:23Z","receivedAt":"2022-03-30T19:59:44Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":" the\n\nOn Tue, Mar 29, 2022 at 7:43 PM Junio C Hamano wrote:\n>\n> \"Tao Klerks via GitGitGadget\" writes:\n>\n> > From: Tao Klerks \n> >\n> > Untracked cache was originally designed to only work with\n> > '--untracked-files=normal', but this causes performance issues for UI\n> > tooling that wants to see \"all\" on a frequent basis. On the other hand,\n> > the conditions that prevented applicability to the \"all\" mode no\n> > longer seem to apply.\n>\n> There is a slight leap in logic flow before \", but this causes\" that\n> forces readers read the above twice and guess what is missing. I am\n> guessing that\n>\n> ... was designed to only work with \"--untracked-files=normal\",\n> and is bypassed when \"--untracked-files=all\" request, but this\n> causes ...\n>\n> is what you meant to say.\n\nYep, will fix.\n\n>\n> The claim in the last sentence \"... no longer seem to apply\" is\n> thrown at the readers without much rationale. Either justify it\n> more solidly or discard the claim?\n>\n\nI can add references to previous conversation threads around the\ntopic, and add the word \"experimentally\".\n\n> > The disqualification of untracked cache has a particularly significant\n> > impact on Windows with the defaulted fscache, where the introduction\n> > of fsmonitor can make the first and costly directory-iteration happen\n> > in untracked file detection, single-threaded, rather than in\n> > preload-index on multiple threads. Improving the performance of a\n> > \"normal\" 'git status' run with fsmonitor can make\n> > 'git status --untracked-files=all' perform much worse.\n>\n> The last sentence is a bit hard to parse and very hard to reason\n> about. Do you mean to say \"--untracked-files=all is slower when the\n> untracked cache is in use, so the performance of 'git status' may be\n> improved for '--untracked-files=normal' when the untracked cache is\n> used, it hurts when 'git status --untracked-files=all' is run\"?\n\nSomething like that. The case where untracked cache bypassing (or\ndisabling) has a huge impact is on windows, and involves fscache and\nfsmonitor:\n* Untracked cache saves some work under qualifying conditions\n* If qualifying conditions are not met (eg when -uall), we do the work\nanyway as though untracked cache were disabled.\n* fscache optimises directory-iteration on windows, with an N-thread\npreload strategy. This is enabled by default, and improves windows\n\"git status\" performance transparently from \"abominably slow\" to just\n\"slow\".\n* fsmonitor, when combined with untracked cache, eliminates recursive\ndirectory-iteration in favor of using a persistent in-memory model of\nwhat's changed on disk (to compare to the index and untracked cache)\n* When fsmonitor is enabled and used, the fscache preload strategy is\n(obviously) disabled\n* When fsmonitor is enabled and untracked cache is disabled or\nbypassed, the otherwise-ubiquitous \"fscache preload\" does not happen,\nand the untracked file search (directory iteration) takes... forever.\n\nIn other words: In windows, where fscache behavior is critical for\nlarge repos and defaulted-on, when fsmonitor is also enabled which\ngenerally further improves things but disqualifies fscache preload,\nthe bypassing of untracked cache causes a huge performance impact. A\n\"-uall\" run can take a minute, where a \"-unormal\" run takes only a\nsecond, because the enumeration of untracked files without using the\nuntracked cache data means a full directory iteration, not optimized\nby fscache. On the other hand, removing fsmonitor and untracked cache\naltogether, and leaving just the fscache optimization to do its job\nconsistently, results in consistent middle-of-the-road performance in\nthe order of 10 seconds in this example.\n\nEnabling fsmonitor in this situation causes -uall to perform\npathologically badly, because of untracked cache bypassing\n(interacting with the fscache-preload-bypassing of fsmonitor).\nHowever, the benefits of dropping status time from 10 seconds to 1\nsecond make fsmonitor very attractive. On the other hand, the\npathological performance with -uall is a problem if you use GUIs that\n*require* -uall. Therefore, fixing untracked cache to be compatible\nwith -uall is critical to large repos on windows where the ideal\nexperience would be fscache working, fsmonitor working, untracked\ncache working, and GUIs able to use -uall without the user waiting a\nminute or more and sometimes timing out.\n\nThis problem sounds like an edge-case when I describe the chain of\ninteractions, but is actually very common among large-repo windows\nusers. Windows users are typically GUI users. GUIs typically use\n-uall, as git's default \"hide contents of untracked folders\" behavior\nis obviously limiting.\n\nThis patch doesn't actually change anything I've described by default\n- but it makes it *possible* to get good/consistent performance with\n-uall (with fscache, with fsmonitor, with untracked cache), *if* you\nconfigure status.showuntrackedfiles=all.\n\nNow, coming back to the paragraph in question; maybe, instead of\ntrying to summarize this windows-fscache-fsmonitor-untrackedcache\ninteraction, I should be a little more vague. Would this work?\n\n--\nWhen 'git status' runs without using the untracked cache, on a large\nrepo, on windows, with fsmonitor, it can run very slowly. This can\nmake GUIs that need to use \"-uall\" (and therefore currently bypass\nuntracked cache) unusable when fsmonitor is enabled, on such large\nrepos.\n--\n\n\n> Let me follow what actually happens in the patch aloud.\n>\n[...]\n> This _guesses_ the user preference from the configuration.\n>\n\nYes - the proposal, in this patch, is that the untracked cache\nusability/compatibility be aligned with the configuration. We are\nadding an extra meaning to the \"status.showuntrackedfiles\"\nconfiguration, such that it not only indicates what you want to have\nhappen when you don't specify \"-u\" to git status, but it *also*\nindicates what kind of status output the untracked cache should be\ntargeting/supporting.\n\n> > +static void new_untracked_cache(struct index_state *istate, int flags)\n> > {\n> > struct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n> > strbuf_init(&uc->ident, 100);\n> > uc->exclude_per_dir = \".gitignore\";\n> > - /* should be the same flags used by git-status */\n> > - uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n> > + uc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate);\n>\n> We use unsigned for the flag word unless there is a reason not to,\n> but this function wants to use top-bit as a signal to \"guess from\n> config\". The caller either dictates what bits to set, or we guess.\n>\n> And the created uc records the flags.\n>\n\nYep - although the word \"guess\" may be slightly misleading here. The\nproposed semantics are such that if the existing untracked cache flags\nare inconsistent with the config, we *discard* the\nno-longer-consistent-with-config untracked cache; so it's not so much\na guess as a mandate.\n\n> > @@ -2762,11 +2782,11 @@ static void new_untracked_cache(struct index_state *istate)\n> > void add_untracked_cache(struct index_state *istate)\n> > {\n> > if (!istate->untracked) {\n> > - new_untracked_cache(istate);\n> > + new_untracked_cache(istate, -1);\n>\n> We are newly creating, so \"-1\" (guess from config) may be the most\n> appropriate (unless the caller of add_untracked_cache() already\n> knows what operation it is using for, that is).\n>\n> > } else {\n> > if (!ident_in_untracked(istate->untracked)) {\n>\n> Found an existing one but need to recreate.\n>\n> > free_untracked_cache(istate->untracked);\n> > - new_untracked_cache(istate);\n> > + new_untracked_cache(istate, -1);\n>\n> In this case, is it likely to give us a better guess to read the\n> configuration, or copy from the existing untracked file? My gut\n> feeling says it would be the latter, and if that is correct, then\n> we might want\n>\n> + int old_flags = istate->untracked->dir_flags;\n> free_untracked_cache(istate->untracked);\n> - new_untracked_cache(istate);\n> + new_untracked_cache(istate, old_flags);\n>\n> instead? I dunno.\n>\n\nAs I noted above, we later consider that an untracked cache with flags\nthat are inconsistent with the current config is an invalid untracked\ncache, and discard it; so setting the flags based on config is the\nright thing (the only useful thing) to do.\n\n\n> > @@ -2781,9 +2801,9 @@ void remove_untracked_cache(struct index_state *istate)\n> > }\n> >\n> > static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,\n> > - int base_len,\n> > - const struct pathspec *pathspec,\n> > - struct index_state *istate)\n> > + int base_len,\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>\n> Is this just re-indenting? Not very welcome, but OK.\n>\n\nWill fix; I think there was a previous version in which these lines\nactually changed.\n\n> > @@ -2814,17 +2834,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> > if (base_len || (pathspec && pathspec->nr))\n> > return NULL;\n> >\n> > - /* Different set of flags may produce different results */\n> > - if (dir->flags != dir->untracked->dir_flags ||\n> > - /*\n> > - * See treat_directory(), case index_nonexistent. Without\n> > - * this flag, we may need to also cache .git file content\n> > - * for the resolve_gitlink_ref() call, which we don't.\n> > - */\n> > - !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n> > - /* We don't support collecting ignore files */\n> > - (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> > - DIR_COLLECT_IGNORED)))\n> > + /* We don't support collecting ignore files */\n> > + if (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n> > + DIR_COLLECT_IGNORED))\n> > return NULL;\n> >\n> > /*\n> > @@ -2847,6 +2859,41 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n> > return NULL;\n> > }\n> >\n> > + /*\n> > + * We don't support using or preparing the untracked cache if\n> > + * the current effective flags don't match the configured\n> > + * flags.\n> > + */\n>\n> Is that what we want? What happens when your user does this:\n>\n> - set showuntrackedfiles to \"normal\"\n> - generate the untracked cache\n> - set showuntrackedfiles to \"all\"\n>\n> And now the user wants to make a request that wants to see flags\n> that are suitable for \"normal\".\n>\n> The best case would be for the untracked cache to know what flags\n> were in use when it was generated, so that in the above sequence,\n> even though the current value of configuration variable and the\n> current request from the command line contradict each other, we\n> can notice that the untracked cache data is suitable for the current\n> request and the right thing happens.\n>\n> > + if (dir->flags != new_untracked_cache_flags(istate))\n> > + return NULL;\n>\n> But this only pays attention to what is in the configuration? It\n> seems to me that it is being too pessimistic, but perhaps it is\n> necessary for correctness somehow?\n\nThe logic in the current patch is:\n* If the configuration doesn't match the request, ignore untracked\ncache completely (exit the UC logic)\n* If the configuration doesn't match the current untracked cache data,\ndiscard the existing untracked cache data (and later recreate it if\neverything else works out)\n\nI think your proposal is something like:\n* If the current untracked cache data doesn't match the request\n** If the current untracked cache data doesn't match the\nconfiguration, then discard the existing untracked cache (and later\nrecreate it if everything else works out)\n** Otherwise, ignore untracked cache completely (exit the UC logic)\n* (otherwise, if current untracked cache data *does* match the\nrequest, assume it's good enough to keep using for now - this is the\nnew behavior)\n\nI find the latter slightly harder to understand and explain, but I'm\nreasonably certain it addresses the case you identified above without\ncompromising correctness in any other cases - it can only have a\npositive impact on performance. I'll have a go at making that change\ntoday.\n"},{"id":"452771","messageId":"CAPMMpoitmyhZZSzELFVSFd2GUKQweB7-c1JQQtaem1QvP48mZA@mail.gmail.com","threadId":"55990","inReplyTo":"xmqqwngb9yzd.fsf@gitster.g","subject":"Re: [PATCH v5 1/2] untracked-cache: test untracked-cache-bypassing behavior with -uall","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-31T05:15:14Z","receivedAt":"2022-03-31T05:15:40Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Mar 30, 2022 at 6:39 PM Junio C Hamano wrote:\n>\n> Tao Klerks writes:\n>\n> > Sorry, I'm not completely sure whether my comment was misleading, or\n> > whether I'm misunderstanding your feedback.\n> >\n> > The test added here does not test \"desirable\" behavior from an\n> > end-user functional perspective, but it does test behavior that is\n> > working \"as-designed\" as of many years ago.\n>\n> Is it \"as-designed\", or just \"left buggy\"?\n>\n> I somehow had an impression that you meant the latter, and it would\n> be our aspirational goal to eventually fix it. And for such case,\n> it would be better to write the test body to show what the command\n> should do, which would make the test fail with today's Git, and mark\n> it as test_expect_failure (which is not an ideal mechanism to prepare\n> for a future fix, but that is what we have now and should use, until\n> a better alternative being worked on is finished).\n>\n> But if it is \"as-designed, some users may find it suboptimal or\n> confusing or with any other negative adjectives, but it is too late\n> to change now and more importantly it is unthinkable to change it\n> because existing tools and users do depend on the current behaviour\",\n> then what you did is perfectly fine.\n>\n\nHeh, the answer is in-between: there certainly is an aspiration that\nsomeone figure out how to have the single untracked cache structure\nsafely support both -uall and -unormal modes with the same data... but\nit's not clear that this is possible, and it's certainly not planned.\n\nI don't feel comfortable saying \"we should fix this\", and that's not\nthe intent of this test - its intent is to say \"this is how it is\n(currently) designed to work. If you accidentally change it, be sure\nthat you are changing the design, not just accidentally changing\nbehavior and potentially/probably introducing a bug\".\n\nI'll try with this updated comment, using the term \"current design\" to\nmake the distinction:\n\n# Bypassing the untracked cache here is not desirable from an\n# end-user perspective, but is expected in the current design.\n# The untracked cache data stored for a -unormal run cannot be\n# correctly used in a -uall run - it would yield incorrect output.\n"},{"id":"452800","messageId":"pull.985.v6.git.1648742535.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v5.git.1648553134.gitgitgadget@gmail.com","subject":"[PATCH v6 0/2] Support untracked cache with '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-31T16:02:13Z","receivedAt":"2022-03-31T16:02:23Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Make it possible for users of the -uall flag to git status, either by\npreference or by need (eg UI tooling), to benefit from the untracked cache\nwhen they set their 'status.showuntrackedfiles' config setting to 'all'.\nThis is especially useful for large repos in Windows, where without\nuntracked cache \"git status\" times can easily be 5x higher, and GUI tooling\ntypically does use -uall.\n\nIn this sixth version, clarify the main commit message and some of the code\ncomments, and adjust the logic slightly such that an existing untracked\ncache structure, when consistent with the requested flags in the current\nrun, can be reused even though the current config would set/store/use the\nother set of flags on a new untracked cache structure.\n\nI'm comfortable with this patch as-is, but am still interested in any\nthoughts as to whether it makes sense and is likely to be accepted to do\nthis as a simple enhancement as proposed here, or whether people be more\ncomfortable with a new configuration option, given the potential for worse\nperformance under specific (and, I believe, vanishingly rare) circumstances.\n\nTao Klerks (2):\n untracked-cache: test untracked-cache-bypassing behavior with -uall\n untracked-cache: support '--untracked-files=all' if configured\n\n dir.c | 88 ++++++++++++++++++-----\n t/t7063-status-untracked-cache.sh | 113 ++++++++++++++++++++++++++++++\n 2 files changed, 185 insertions(+), 16 deletions(-)\n\n\nbase-commit: 805e0a68082a217f0112db9ee86a022227a9c81b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-985%2FTaoK%2Ftaok-untracked-cache-with-uall-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-985/TaoK/taok-untracked-cache-with-uall-v6\nPull-Request: https://github.com/gitgitgadget/git/pull/985\n\nRange-diff vs v5:\n\n 1: 98a4d8f35c5 ! 1: f27f018493a untracked-cache: test untracked-cache-bypassing behavior with -uall\n @@ Commit message\n '--untracked-files=normal', and it gets ignored when\n '--untracked-files=all' is specified instead.\n \n - Add explicit tests for this behavior.\n + Add explicit tests for this known as-designed behavior.\n \n Signed-off-by: Tao Klerks \n \n @@ t/t7063-status-untracked-cache.sh: test_expect_success 'untracked cache after se\n +?? three\n +EOF\n +\n -+# Bypassing the untracked cache here is not desirable, but it expected\n -+# in the current implementation\n ++# Bypassing the untracked cache here is not desirable from an\n ++# end-user perspective, but is expected in the current design.\n ++# The untracked cache data stored for a -unormal run cannot be\n ++# correctly used in a -uall run - it would yield incorrect output.\n +test_expect_success 'untracked cache is bypassed with -uall' '\n +\t: >../trace.output &&\n +\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n 2: f60d2c6e36c ! 2: 0095ec49c5f untracked-cache: support '--untracked-files=all' if configured\n @@ Commit message\n untracked-cache: support '--untracked-files=all' if configured\n \n Untracked cache was originally designed to only work with\n - '--untracked-files=normal', but this causes performance issues for UI\n - tooling that wants to see \"all\" on a frequent basis. On the other hand,\n - the conditions that prevented applicability to the \"all\" mode no\n - longer seem to apply.\n + \"--untracked-files=normal\", and is bypassed when\n + \"--untracked-files=all\" is requested, but this causes performance\n + issues for UI tooling that wants to see \"all\" on a frequent basis.\n \n - The disqualification of untracked cache has a particularly significant\n - impact on Windows with the defaulted fscache, where the introduction\n - of fsmonitor can make the first and costly directory-iteration happen\n - in untracked file detection, single-threaded, rather than in\n - preload-index on multiple threads. Improving the performance of a\n - \"normal\" 'git status' run with fsmonitor can make\n - 'git status --untracked-files=all' perform much worse.\n + On the other hand, the conditions that altogether prevented\n + applicability to the \"all\" mode no longer seem to apply, after\n + several major refactors in recent years; this possibility was\n + discussed in\n + 81153d02-8e7a-be59-e709-e90cd5906f3a@jeffhostetler.com and\n + CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com,\n + and somewhat confirmed experimentally by several users using a\n + version of this patch to use untracked cache with -uall for about a\n + year.\n +\n + When 'git status' runs without using the untracked cache, on a large\n + repo, on windows, with fsmonitor, it can run very slowly. This can\n + make GUIs that need to use \"-uall\" (and therefore currently bypass\n + untracked cache) unusable when fsmonitor is enabled, on such large\n + repos.\n \n To partially address this, align the supported directory flags for the\n stored untracked cache data with the git config. If a user specifies\n @@ dir.c: static void new_untracked_cache(struct index_state *istate)\n \t\t}\n \t}\n }\n -@@ dir.c: void remove_untracked_cache(struct index_state *istate)\n - }\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 struct index_state *istate)\n -+\t\t\t\t\t\t\t int base_len,\n -+\t\t\t\t\t\t\t const struct pathspec *pathspec,\n -+\t\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 @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n @@ dir.c: static struct untracked_cache_dir *validate_untracked_cache(struct dir_st\n \t\treturn NULL;\n \t}\n \n -+\t/*\n -+\t * We don't support using or preparing the untracked cache if\n -+\t * the current effective flags don't match the configured\n -+\t * flags.\n -+\t */\n -+\tif (dir->flags != new_untracked_cache_flags(istate))\n -+\t\treturn NULL;\n -+\n +\t/*\n +\t * If the untracked structure we received does not have the same flags\n -+\t * as configured, but the configured flags do match the effective flags,\n -+\t * then we need to reset / create a new \"untracked\" structure to match\n -+\t * the new config.\n -+\t * Keeping the saved and used untracked cache in-line with the\n -+\t * configuration provides an opportunity for frequent users of\n -+\t * \"git status -uall\" to leverage the untracked cache by aligning their\n -+\t * configuration (setting \"status.showuntrackedfiles\" to \"all\" or\n -+\t * \"normal\" as appropriate), where previously this option was\n -+\t * incompatible with untracked cache and *consistently* caused\n -+\t * surprisingly bad performance (with fscache and fsmonitor enabled) on\n -+\t * Windows.\n -+\t *\n -+\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n -+\t * to not be as bound up with the desired output in a given run,\n -+\t * and instead iterated through and stored enough information to\n -+\t * correctly serve both \"modes\", then users could get peak performance\n -+\t * with or without '-uall' regardless of their\n -+\t * \"status.showuntrackedfiles\" config.\n ++\t * as requested in this run, we're going to need to either discard the\n ++\t * existing structure (and potentially later recreate), or bypass the\n ++\t * untracked cache mechanism for this run.\n +\t */\n +\tif (dir->flags != dir->untracked->dir_flags) {\n -+\t\tfree_untracked_cache(istate->untracked);\n -+\t\tnew_untracked_cache(istate, dir->flags);\n -+\t\tdir->untracked = istate->untracked;\n ++\t\t/*\n ++\t\t * If the untracked structure we received does not have the same flags\n ++\t\t * as configured, then we need to reset / create a new \"untracked\"\n ++\t\t * structure to match the new config.\n ++\t\t *\n ++\t\t * Keeping the saved and used untracked cache consistent with the\n ++\t\t * configuration provides an opportunity for frequent users of\n ++\t\t * \"git status -uall\" to leverage the untracked cache by aligning their\n ++\t\t * configuration - setting \"status.showuntrackedfiles\" to \"all\" or\n ++\t\t * \"normal\" as appropriate.\n ++\t\t *\n ++\t\t * Previously using -uall (or setting \"status.showuntrackedfiles\" to\n ++\t\t * \"all\") was incompatible with untracked cache and *consistently*\n ++\t\t * caused surprisingly bad performance (with fscache and fsmonitor\n ++\t\t * enabled) on Windows.\n ++\t\t *\n ++\t\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n ++\t\t * to not be as bound up with the desired output in a given run,\n ++\t\t * and instead iterated through and stored enough information to\n ++\t\t * correctly serve both \"modes\", then users could get peak performance\n ++\t\t * with or without '-uall' regardless of their\n ++\t\t * \"status.showuntrackedfiles\" config.\n ++\t\t */\n ++\t\tif (dir->untracked->dir_flags != new_untracked_cache_flags(istate)) {\n ++\t\t\tfree_untracked_cache(istate->untracked);\n ++\t\t\tnew_untracked_cache(istate, dir->flags);\n ++\t\t\tdir->untracked = istate->untracked;\n ++\t\t}\n ++\t\telse {\n ++\t\t\t/*\n ++\t\t\t * Current untracked cache data is consistent with config, but not\n ++\t\t\t * usable in this request/run; just bypass untracked cache.\n ++\t\t\t */\n ++\t\t\treturn NULL;\n ++\t\t}\n +\t}\n +\n \tif (!dir->untracked->root) {\n @@ t/t7063-status-untracked-cache.sh: test_expect_success 'untracked cache remains\n +\ttest_cmp ../trace.expect ../trace.relevant\n +'\n +\n -+# Bypassing the untracked cache here is not desirable, but it expected\n -+# in the current implementation\n ++# Bypassing the untracked cache here is not desirable from an\n ++# end-user perspective, but is expected in the current design.\n ++# The untracked cache data stored for a -all run cannot be\n ++# correctly used in a -unormal run - it would yield incorrect\n ++# output.\n +test_expect_success 'if -uall is configured, untracked cache is bypassed with -unormal' '\n +\ttest_config status.showuntrackedfiles all &&\n +\t: >../trace.output &&\n\n-- \ngitgitgadget\n"},{"id":"452801","messageId":"f27f018493afd56a1eacb2008a7284e9e262f73e.1648742535.git.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v6.git.1648742535.gitgitgadget@gmail.com","subject":"[PATCH v6 1/2] untracked-cache: test untracked-cache-bypassing behavior with -uall","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-31T16:02:14Z","receivedAt":"2022-03-31T16:02:25Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n'--untracked-files=normal', and it gets ignored when\n'--untracked-files=all' is specified instead.\n\nAdd explicit tests for this known as-designed behavior.\n\nSigned-off-by: Tao Klerks \n---\n t/t7063-status-untracked-cache.sh | 32 +++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex ca90ee805e7..b89be8dc6d6 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -190,6 +190,38 @@ test_expect_success 'untracked cache after second status' '\n \ttest_cmp ../dump.expect ../actual\n '\n \n+cat >../status_uall.expect <../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status -uall --porcelain >../actual &&\n+\tiuc status -uall --porcelain >../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <../actual &&\n+\ttest_cmp ../dump.expect ../actual\n+'\n+\n test_expect_success 'modify in root directory, one dir invalidation' '\n \t: >four &&\n \ttest-tool chmtime =-240 four &&\n-- \ngitgitgadget\n\n"},{"id":"452802","messageId":"0095ec49c5f3a3fc60c8fa0487dbeb04b0e1385d.1648742535.git.gitgitgadget@gmail.com","threadId":"55990","inReplyTo":"pull.985.v6.git.1648742535.gitgitgadget@gmail.com","subject":"[PATCH v6 2/2] untracked-cache: support '--untracked-files=all' if configured","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-31T16:02:15Z","receivedAt":"2022-03-31T16:02:30Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks \n\nUntracked cache was originally designed to only work with\n\"--untracked-files=normal\", and is bypassed when\n\"--untracked-files=all\" is requested, but this causes performance\nissues for UI tooling that wants to see \"all\" on a frequent basis.\n\nOn the other hand, the conditions that altogether prevented\napplicability to the \"all\" mode no longer seem to apply, after\nseveral major refactors in recent years; this possibility was\ndiscussed in\n81153d02-8e7a-be59-e709-e90cd5906f3a@jeffhostetler.com and\nCABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com,\nand somewhat confirmed experimentally by several users using a\nversion of this patch to use untracked cache with -uall for about a\nyear.\n\nWhen 'git status' runs without using the untracked cache, on a large\nrepo, on windows, with fsmonitor, it can run very slowly. This can\nmake GUIs that need to use \"-uall\" (and therefore currently bypass\nuntracked cache) unusable when fsmonitor is enabled, on such large\nrepos.\n\nTo partially address this, align the supported directory flags for the\nstored untracked cache data with the git config. If a user specifies\nan '--untracked-files=' commandline parameter that does not align with\ntheir 'status.showuntrackedfiles' config value, then the untracked\ncache will be ignored - as it is for other unsupported situations like\nwhen a pathspec is specified.\n\nIf the previously stored flags no longer match the current\nconfiguration, but the currently-applicable flags do match the current\nconfiguration, then discard the previously stored untracked cache\ndata.\n\nFor most users there will be no change in behavior. Users who need\n'--untracked-files=all' to perform well will now have the option of\nsetting \"status.showuntrackedfiles\" to \"all\" for better / more\nconsistent performance.\n\nUsers who need '--untracked-files=all' to perform well for their\ntooling AND prefer to avoid the verbosity of \"all\" when running\ngit status explicitly without options... are out of luck for now (no\nchange).\n\nUsers who have the \"status.showuntrackedfiles\" config set to \"all\"\nand yet frequently explicitly call\n'git status --untracked-files=normal' (and use the untracked cache)\nare the only ones who will be disadvantaged by this change. Their\n\"--untracked-files=normal\" calls will, after this change, no longer\nuse the untracked cache.\n\nSigned-off-by: Tao Klerks \n---\n dir.c | 88 +++++++++++++++++++++++++------\n t/t7063-status-untracked-cache.sh | 81 ++++++++++++++++++++++++++++\n 2 files changed, 153 insertions(+), 16 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex f2b0f242101..26c4d141ab9 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2747,13 +2747,33 @@ static void set_untracked_ident(struct untracked_cache *uc)\n \tstrbuf_addch(&uc->ident, 0);\n }\n \n-static void new_untracked_cache(struct index_state *istate)\n+static unsigned new_untracked_cache_flags(struct index_state *istate)\n+{\n+\tstruct repository *repo = istate->repo;\n+\tchar *val;\n+\n+\t/*\n+\t * This logic is coordinated with the setting of these flags in\n+\t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n+\t * of the config setting in commit.c#git_status_config()\n+\t */\n+\tif (!repo_config_get_string(repo, \"status.showuntrackedfiles\", &val) &&\n+\t !strcmp(val, \"all\"))\n+\t\treturn 0;\n+\n+\t/*\n+\t * The default, if \"all\" is not set, is \"normal\" - leading us here.\n+\t * If the value is \"none\" then it really doesn't matter.\n+\t */\n+\treturn DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+}\n+\n+static void new_untracked_cache(struct index_state *istate, int flags)\n {\n \tstruct untracked_cache *uc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, 100);\n \tuc->exclude_per_dir = \".gitignore\";\n-\t/* should be the same flags used by git-status */\n-\tuc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n+\tuc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate);\n \tset_untracked_ident(uc);\n \tistate->untracked = uc;\n \tistate->cache_changed |= UNTRACKED_CHANGED;\n@@ -2762,11 +2782,11 @@ static void new_untracked_cache(struct index_state *istate)\n void add_untracked_cache(struct index_state *istate)\n {\n \tif (!istate->untracked) {\n-\t\tnew_untracked_cache(istate);\n+\t\tnew_untracked_cache(istate, -1);\n \t} else {\n \t\tif (!ident_in_untracked(istate->untracked)) {\n \t\t\tfree_untracked_cache(istate->untracked);\n-\t\t\tnew_untracked_cache(istate);\n+\t\t\tnew_untracked_cache(istate, -1);\n \t\t}\n \t}\n }\n@@ -2814,17 +2834,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \tif (base_len || (pathspec && pathspec->nr))\n \t\treturn NULL;\n \n-\t/* Different set of flags may produce different results */\n-\tif (dir->flags != dir->untracked->dir_flags ||\n-\t /*\n-\t * See treat_directory(), case index_nonexistent. Without\n-\t * this flag, we may need to also cache .git file content\n-\t * for the resolve_gitlink_ref() call, which we don't.\n-\t */\n-\t !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||\n-\t /* We don't support collecting ignore files */\n-\t (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n-\t\t\t DIR_COLLECT_IGNORED)))\n+\t/* We don't support collecting ignore files */\n+\tif (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |\n+\t\t\tDIR_COLLECT_IGNORED))\n \t\treturn NULL;\n \n \t/*\n@@ -2847,6 +2859,50 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\treturn NULL;\n \t}\n \n+\t/*\n+\t * If the untracked structure we received does not have the same flags\n+\t * as requested in this run, we're going to need to either discard the\n+\t * existing structure (and potentially later recreate), or bypass the\n+\t * untracked cache mechanism for this run.\n+\t */\n+\tif (dir->flags != dir->untracked->dir_flags) {\n+\t\t/*\n+\t\t * If the untracked structure we received does not have the same flags\n+\t\t * as configured, then we need to reset / create a new \"untracked\"\n+\t\t * structure to match the new config.\n+\t\t *\n+\t\t * Keeping the saved and used untracked cache consistent with the\n+\t\t * configuration provides an opportunity for frequent users of\n+\t\t * \"git status -uall\" to leverage the untracked cache by aligning their\n+\t\t * configuration - setting \"status.showuntrackedfiles\" to \"all\" or\n+\t\t * \"normal\" as appropriate.\n+\t\t *\n+\t\t * Previously using -uall (or setting \"status.showuntrackedfiles\" to\n+\t\t * \"all\") was incompatible with untracked cache and *consistently*\n+\t\t * caused surprisingly bad performance (with fscache and fsmonitor\n+\t\t * enabled) on Windows.\n+\t\t *\n+\t\t * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage\n+\t\t * to not be as bound up with the desired output in a given run,\n+\t\t * and instead iterated through and stored enough information to\n+\t\t * correctly serve both \"modes\", then users could get peak performance\n+\t\t * with or without '-uall' regardless of their\n+\t\t * \"status.showuntrackedfiles\" config.\n+\t\t */\n+\t\tif (dir->untracked->dir_flags != new_untracked_cache_flags(istate)) {\n+\t\t\tfree_untracked_cache(istate->untracked);\n+\t\t\tnew_untracked_cache(istate, dir->flags);\n+\t\t\tdir->untracked = istate->untracked;\n+\t\t}\n+\t\telse {\n+\t\t\t/*\n+\t\t\t * Current untracked cache data is consistent with config, but not\n+\t\t\t * usable in this request/run; just bypass untracked cache.\n+\t\t\t */\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\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, \"\");\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex b89be8dc6d6..9936cc329ec 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -222,6 +222,87 @@ test_expect_success 'untracked cache remains after bypass' '\n \ttest_cmp ../dump.expect ../actual\n '\n \n+test_expect_success 'if -uall is configured, untracked cache gets populated by default' '\n+\ttest_config status.showuntrackedfiles all &&\n+\t: >../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status --porcelain >../actual &&\n+\tiuc status --porcelain >../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <../dump_uall.expect <../actual &&\n+\ttest_cmp ../dump_uall.expect ../actual\n+'\n+\n+test_expect_success 'if -uall is configured, untracked cache is used by default' '\n+\ttest_config status.showuntrackedfiles all &&\n+\t: >../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status --porcelain >../actual &&\n+\tiuc status --porcelain >../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../status.iuc &&\n+\ttest_cmp ../status_uall.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <../trace.output &&\n+\tGIT_TRACE2_PERF=\"$TRASH_DIRECTORY/trace.output\" \\\n+\tgit status -unormal --porcelain >../actual &&\n+\tiuc status -unormal --porcelain >../status.iuc &&\n+\ttest_cmp ../status.expect ../status.iuc &&\n+\ttest_cmp ../status.expect ../actual &&\n+\tget_relevant_traces ../trace.output ../trace.relevant &&\n+\tcat >../trace.expect <four &&\n \ttest-tool chmtime =-240 four &&\n-- \ngitgitgadget\n"}]}