{"thread":{"id":"47761","subject":"[PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","startedAt":"2018-02-05T19:57:44Z","lastAt":"2018-03-01T12:35:39Z","messageCount":18,"participants":["Ben Peart","Junio C Hamano","Brandon Williams","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"338239","messageId":"20180205195619.31064-1-benpeart@microsoft.com","threadId":"47761","inReplyTo":null,"subject":"[PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Ben Peart","fromEmail":"benpeart@microsoft.com","sentAt":"2018-02-05T19:56:19Z","receivedAt":"2018-02-05T19:57:44Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"The untracked cache saves its current state in the UNTR index extension.\nCurrently, _any_ change to that state causes the index to be flagged as dirty\nand written out to disk.  Unfortunately, the cost to write out the index can\nexceed the savings gained by using the untracked cache.  Since it is a cache\nthat can be updated from the current state of the working directory, there is\nno functional requirement that the index be written out for every change to the\nuntracked cache.\n\nUpdate the untracked cache logic so that it no longer forces the index to be\nwritten to disk except in the case where the extension is being turned on or\noff.  When some other git command requires the index to be written to disk, the\nuntracked cache will take advantage of that to save it's updated state as well.\nThis results in a performance win when looked at over common sequences of git\ncommands (ie such as a status followed by add, commit, etc).\n\nAfter this patch, all the logic to track statistics for the untracked cache\ncould be removed as it is only used by debug tracing used to debug the untracked\ncache.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n\nNotes:\n    Base Ref: master\n    Web-Diff: https://github.com/benpeart/git/commit/20c2e8d787\n    Checkout: git fetch https://github.com/benpeart/git untracked-cache-v1 && git checkout 20c2e8d787\n\n dir.c                             | 3 ++-\n t/t7063-status-untracked-cache.sh | 3 +++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7c4b45e30e..da93374f0c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2297,7 +2297,8 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\t\t\t dir->untracked->gitignore_invalidated,\n \t\t\t\t dir->untracked->dir_invalidated,\n \t\t\t\t dir->untracked->dir_opened);\n-\t\tif (dir->untracked == istate->untracked &&\n+\t\tif (getenv(\"GIT_TEST_UNTRACKED_CACHE\") &&\n+\t\t\tdir->untracked == istate->untracked &&\n \t\t    (dir->untracked->dir_opened ||\n \t\t     dir->untracked->gitignore_invalidated ||\n \t\t     dir->untracked->dir_invalidated))\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex e5fb892f95..6ef520e823 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -14,6 +14,9 @@ test_description='test untracked cache'\n # See <20160803174522.5571-1-pclouds@gmail.com> if you want to know\n # more.\n \n+GIT_TEST_UNTRACKED_CACHE=true\n+export GIT_TEST_UNTRACKED_CACHE\n+\n sync_mtime () {\n \tfind . -type d -ls >/dev/null\n }\n\nbase-commit: 5be1f00a9a701532232f57958efab4be8c959a29\n-- \n2.15.0.windows.1\n\n"},{"id":"338252","messageId":"xmqqd11j9mx2.fsf@gitster-ct.c.googlers.com","threadId":"47761","inReplyTo":"20180205195619.31064-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-05T20:55:21Z","receivedAt":"2018-02-05T20:55:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <benpeart@microsoft.com> writes:\n\n> The untracked cache saves its current state in the UNTR index extension.\n> Currently, _any_ change to that state causes the index to be flagged as dirty\n> and written out to disk.  Unfortunately, the cost to write out the index can\n> exceed the savings gained by using the untracked cache.  Since it is a cache\n> that can be updated from the current state of the working directory, there is\n> no functional requirement that the index be written out for every change to the\n> untracked cache.\n>\n> Update the untracked cache logic so that it no longer forces the index to be\n> written to disk except in the case where the extension is being turned on or\n> off.  When some other git command requires the index to be written to disk, the\n> untracked cache will take advantage of that to save it's updated state as well.\n> This results in a performance win when looked at over common sequences of git\n> commands (ie such as a status followed by add, commit, etc).\n>\n> After this patch, all the logic to track statistics for the untracked cache\n> could be removed as it is only used by debug tracing used to debug the untracked\n> cache.\n>\n> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> ---\n>\n\nOK, so in other words (note: not a suggestion to use different\nwording in the log message; just making sure I got the motivation\nbehind this change correctly), without this new environment\nvariable, changes to untracked cache alone (due to observed changes\nin the filesystem) does not count as \"in-core index changed so we\nneed to write it back to the disk\".\n\nThat makes sense to me.\n\nIs it envisioned that we want to have similar but different \"testing\nonly\" behaviour around this area?  If not, this environment variable\nsounds more like \"force-flush untracked cache\", not \"test untracked\ncache\", to me.\n\n> +GIT_TEST_UNTRACKED_CACHE=true\n> +export GIT_TEST_UNTRACKED_CACHE\n> +\n>  sync_mtime () {\n>  \tfind . -type d -ls >/dev/null\n>  }\n>\n> base-commit: 5be1f00a9a701532232f57958efab4be8c959a29\n"},{"id":"338260","messageId":"20180205215805.GA90084@google.com","threadId":"47761","inReplyTo":"20180205195619.31064-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-02-05T21:58:05Z","receivedAt":"2018-02-05T21:58:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/05, Ben Peart wrote:\n> The untracked cache saves its current state in the UNTR index extension.\n> Currently, _any_ change to that state causes the index to be flagged as dirty\n> and written out to disk.  Unfortunately, the cost to write out the index can\n> exceed the savings gained by using the untracked cache.  Since it is a cache\n> that can be updated from the current state of the working directory, there is\n> no functional requirement that the index be written out for every change to the\n> untracked cache.\n> \n> Update the untracked cache logic so that it no longer forces the index to be\n> written to disk except in the case where the extension is being turned on or\n> off.  When some other git command requires the index to be written to disk, the\n> untracked cache will take advantage of that to save it's updated state as well.\n> This results in a performance win when looked at over common sequences of git\n> commands (ie such as a status followed by add, commit, etc).\n> \n> After this patch, all the logic to track statistics for the untracked cache\n> could be removed as it is only used by debug tracing used to debug the untracked\n> cache.\n\nSo we don't need to update it every time because its just a cache\nand if its inaccurate between status calls that's ok?  So only\noperations like add and commit will actually write out the untracked\ncache (as a part of writing out the index).  Sounds ok.\n\nWhat benefit is there to using the untracked cache then?  Sounds like\nyou should just turn it off instead?\n(I'm sure this is a naive question :D )\n\n> \n> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n> ---\n> \n> Notes:\n>     Base Ref: master\n>     Web-Diff: https://github.com/benpeart/git/commit/20c2e8d787\n>     Checkout: git fetch https://github.com/benpeart/git untracked-cache-v1 && git checkout 20c2e8d787\n> \n>  dir.c                             | 3 ++-\n>  t/t7063-status-untracked-cache.sh | 3 +++\n>  2 files changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/dir.c b/dir.c\n> index 7c4b45e30e..da93374f0c 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2297,7 +2297,8 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n>  \t\t\t\t dir->untracked->gitignore_invalidated,\n>  \t\t\t\t dir->untracked->dir_invalidated,\n>  \t\t\t\t dir->untracked->dir_opened);\n> -\t\tif (dir->untracked == istate->untracked &&\n> +\t\tif (getenv(\"GIT_TEST_UNTRACKED_CACHE\") &&\n> +\t\t\tdir->untracked == istate->untracked &&\n>  \t\t    (dir->untracked->dir_opened ||\n>  \t\t     dir->untracked->gitignore_invalidated ||\n>  \t\t     dir->untracked->dir_invalidated))\n> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\n> index e5fb892f95..6ef520e823 100755\n> --- a/t/t7063-status-untracked-cache.sh\n> +++ b/t/t7063-status-untracked-cache.sh\n> @@ -14,6 +14,9 @@ test_description='test untracked cache'\n>  # See <20160803174522.5571-1-pclouds@gmail.com> if you want to know\n>  # more.\n>  \n> +GIT_TEST_UNTRACKED_CACHE=true\n> +export GIT_TEST_UNTRACKED_CACHE\n> +\n>  sync_mtime () {\n>  \tfind . -type d -ls >/dev/null\n>  }\n> \n> base-commit: 5be1f00a9a701532232f57958efab4be8c959a29\n> -- \n> 2.15.0.windows.1\n> \n\n-- \nBrandon Williams\n"},{"id":"338465","messageId":"6ff2ecb4-7e37-3b06-3722-cccec2329934@gmail.com","threadId":"47761","inReplyTo":"xmqqd11j9mx2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-06T01:39:43Z","receivedAt":"2018-02-06T01:39:48Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/5/2018 3:55 PM, Junio C Hamano wrote:\n> Ben Peart <benpeart@microsoft.com> writes:\n> \n>> The untracked cache saves its current state in the UNTR index extension.\n>> Currently, _any_ change to that state causes the index to be flagged as dirty\n>> and written out to disk.  Unfortunately, the cost to write out the index can\n>> exceed the savings gained by using the untracked cache.  Since it is a cache\n>> that can be updated from the current state of the working directory, there is\n>> no functional requirement that the index be written out for every change to the\n>> untracked cache.\n>>\n>> Update the untracked cache logic so that it no longer forces the index to be\n>> written to disk except in the case where the extension is being turned on or\n>> off.  When some other git command requires the index to be written to disk, the\n>> untracked cache will take advantage of that to save it's updated state as well.\n>> This results in a performance win when looked at over common sequences of git\n>> commands (ie such as a status followed by add, commit, etc).\n>>\n>> After this patch, all the logic to track statistics for the untracked cache\n>> could be removed as it is only used by debug tracing used to debug the untracked\n>> cache.\n>>\n>> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n>> ---\n>>\n> \n> OK, so in other words (note: not a suggestion to use different\n> wording in the log message; just making sure I got the motivation\n> behind this change correctly), without this new environment\n> variable, changes to untracked cache alone (due to observed changes\n> in the filesystem) does not count as \"in-core index changed so we\n> need to write it back to the disk\".\n> \n\nCorrect\n\n> That makes sense to me.\n> \n> Is it envisioned that we want to have similar but different \"testing\n> only\" behaviour around this area?  If not, this environment variable\n> sounds more like \"force-flush untracked cache\", not \"test untracked\n> cache\", to me.\n> \n\nMany of the tests make a change and then verify that the on disk \nstructure was updated correctly.  This was the simplest way to keep \nthose tests functioning. I don't imagine this would be used for anything \nother than enabling the tests.  I hate naming so am happy to name it \nwhich ever you think is best. :)\n\n>> +GIT_TEST_UNTRACKED_CACHE=true\n>> +export GIT_TEST_UNTRACKED_CACHE\n>> +\n>>   sync_mtime () {\n>>   \tfind . -type d -ls >/dev/null\n>>   }\n>>\n>> base-commit: 5be1f00a9a701532232f57958efab4be8c959a29\n"},{"id":"338467","messageId":"6fb43664-7546-7865-0488-8ed6292d77a6@gmail.com","threadId":"47761","inReplyTo":"20180205215805.GA90084@google.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-06T01:48:00Z","receivedAt":"2018-02-06T01:48:05Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/5/2018 4:58 PM, Brandon Williams wrote:\n> On 02/05, Ben Peart wrote:\n>> The untracked cache saves its current state in the UNTR index extension.\n>> Currently, _any_ change to that state causes the index to be flagged as dirty\n>> and written out to disk.  Unfortunately, the cost to write out the index can\n>> exceed the savings gained by using the untracked cache.  Since it is a cache\n>> that can be updated from the current state of the working directory, there is\n>> no functional requirement that the index be written out for every change to the\n>> untracked cache.\n>>\n>> Update the untracked cache logic so that it no longer forces the index to be\n>> written to disk except in the case where the extension is being turned on or\n>> off.  When some other git command requires the index to be written to disk, the\n>> untracked cache will take advantage of that to save it's updated state as well.\n>> This results in a performance win when looked at over common sequences of git\n>> commands (ie such as a status followed by add, commit, etc).\n>>\n>> After this patch, all the logic to track statistics for the untracked cache\n>> could be removed as it is only used by debug tracing used to debug the untracked\n>> cache.\n> \n> So we don't need to update it every time because its just a cache\n> and if its inaccurate between status calls that's ok?  So only\n> operations like add and commit will actually write out the untracked\n> cache (as a part of writing out the index).  Sounds ok.\n> \n> What benefit is there to using the untracked cache then?  Sounds like\n> you should just turn it off instead?\n> (I'm sure this is a naive question :D )\n\nThe parts of the untracked cache that have not changed since the \nextension was updated are still cached and valid.  Only those \ndirectories that have changes will need to be checked.\n\nWith the old behavior, making a change in dir1/, then calling status \nwould update the dir1/ untracked cache entry, flag the index as dirty \nand write it out.  On the next status, git would detect that no changes \nhave been made and use the cached data for dir1/.\n\nWith the new behavior, making a change in dir1/, then calling status \nwould update the dir1/ untracked cache entry but not write it out. On \nthe next status, git would detect the change in dir1/ again and update \nthe untracked cache.  All of the other cached entries are still valid \nand the cache would be used for them.  The updated cache entry for dir1/ \nwould not get persisted to disk until something that required the index \nto be written out.\n\nThe behavior is correct in both cases.  You just don't get the benefit \nof the updated cache for the dir1/ entry until the index is persisted \nagain.  What you gain in exchange is that you don't have to write out \nthe index which is (typically) a lot more expensive than checking dir1/ \nfor changes.\n\n> \n>>\n>> Signed-off-by: Ben Peart <benpeart@microsoft.com>\n>> ---\n>>\n>> Notes:\n>>      Base Ref: master\n>>      Web-Diff: https://github.com/benpeart/git/commit/20c2e8d787\n>>      Checkout: git fetch https://github.com/benpeart/git untracked-cache-v1 && git checkout 20c2e8d787\n>>\n>>   dir.c                             | 3 ++-\n>>   t/t7063-status-untracked-cache.sh | 3 +++\n>>   2 files changed, 5 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/dir.c b/dir.c\n>> index 7c4b45e30e..da93374f0c 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -2297,7 +2297,8 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n>>   \t\t\t\t dir->untracked->gitignore_invalidated,\n>>   \t\t\t\t dir->untracked->dir_invalidated,\n>>   \t\t\t\t dir->untracked->dir_opened);\n>> -\t\tif (dir->untracked == istate->untracked &&\n>> +\t\tif (getenv(\"GIT_TEST_UNTRACKED_CACHE\") &&\n>> +\t\t\tdir->untracked == istate->untracked &&\n>>   \t\t    (dir->untracked->dir_opened ||\n>>   \t\t     dir->untracked->gitignore_invalidated ||\n>>   \t\t     dir->untracked->dir_invalidated))\n>> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\n>> index e5fb892f95..6ef520e823 100755\n>> --- a/t/t7063-status-untracked-cache.sh\n>> +++ b/t/t7063-status-untracked-cache.sh\n>> @@ -14,6 +14,9 @@ test_description='test untracked cache'\n>>   # See <20160803174522.5571-1-pclouds@gmail.com> if you want to know\n>>   # more.\n>>   \n>> +GIT_TEST_UNTRACKED_CACHE=true\n>> +export GIT_TEST_UNTRACKED_CACHE\n>> +\n>>   sync_mtime () {\n>>   \tfind . -type d -ls >/dev/null\n>>   }\n>>\n>> base-commit: 5be1f00a9a701532232f57958efab4be8c959a29\n>> -- \n>> 2.15.0.windows.1\n>>\n> \n"},{"id":"338503","messageId":"CACsJy8DLP=j-h3knwX9zOpejAfUbv1YJwfB-iw4476oy0hTfxg@mail.gmail.com","threadId":"47761","inReplyTo":"6fb43664-7546-7865-0488-8ed6292d77a6@gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-06T12:27:07Z","receivedAt":"2018-02-06T12:27:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 6, 2018 at 8:48 AM, Ben Peart <peartben@gmail.com> wrote:\n> With the new behavior, making a change in dir1/, then calling status would\n> update the dir1/ untracked cache entry but not write it out. On the next\n> status, git would detect the change in dir1/ again and update the untracked\n\nThing only missing piece here I would add is, this dir1/ detection is\ndone by watchman. We have to contact watchman and ask the set of\nchanged paths since $TIME where $TIME is the last time we updated\nuntracked cache and invalidate those paths in core. Then it should\nwork correctly. I checked the watchman query in the fsmonitor hook and\nI think it's correct so far.\n\nExcept one point. What if you activate fsmonitor, write down the\nfsmonitor_last_update field there but _not_ create UNTR extension for\nthe same timestamp? UNTR extension is created only when\nread_directory() is executed and we don't do that in every command. I\nhaven't checked fsmonitor.c carefully, maybe I'm missing something.\n\nOne thing I'd like to point out in this patch is untracked cache will\nstart degrading if you just make the initial UNTR extension once and\ndon't ever update it again. Dirty paths hit slow path and will start\npoking the disk. If somebody accidentally change a minor thing in\nevery single directory (worst case scenario), the untracked cache\nbecomes useless. We need a threshold or something to start repairing\nUNTR extension if it gets damaged too much.\n\nIf you rely on random index updates (e.g. the main entries got updated\nand .git/index must be updated) to write UNTR extension down together.\nPlease don't do that, at least not this way. cache_changed mask should\nreflect all dirty parts in .git/index. If UNTR extension is not marked\nupdated, it's legit to just skip generating/writing it down (e.g. if I\nkept the old UNTR extension from the last time I read .git/index\naround in memory)\n\n> cache.  All of the other cached entries are still valid and the cache would\n> be used for them.  The updated cache entry for dir1/ would not get persisted\n> to disk until something that required the index to be written out.\n>\n> The behavior is correct in both cases.  You just don't get the benefit of\n> the updated cache for the dir1/ entry until the index is persisted again.\n> What you gain in exchange is that you don't have to write out the index\n> which is (typically) a lot more expensive than checking dir1/ for changes.\n\nThis is another thing that bugs me. I know you're talking about huge\nindex files, but at what size should we start this sort of\noptimization? Writing down a few MBs on linux is cheap enough that I\nwon't bother optimizing (and I get my UNTR extension repaired all the\ntime, so reduced lstat calls and stuff). This \"typically\" only comes\nat certain size, what size?\n-- \nDuy\n"},{"id":"338504","messageId":"CACsJy8C+9Det0zF4+HZ6TL36aCFgboEh4=3yrEk5WvyUD30Drw@mail.gmail.com","threadId":"47761","inReplyTo":"CACsJy8DLP=j-h3knwX9zOpejAfUbv1YJwfB-iw4476oy0hTfxg@mail.gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-06T12:55:38Z","receivedAt":"2018-02-06T12:56:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 6, 2018 at 7:27 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Tue, Feb 6, 2018 at 8:48 AM, Ben Peart <peartben@gmail.com> wrote:\n>> With the new behavior, making a change in dir1/, then calling status would\n>> update the dir1/ untracked cache entry but not write it out. On the next\n>> status, git would detect the change in dir1/ again and update the untracked\n>\n> Thing only missing piece here I would add is, this dir1/ detection is\n> done by watchman. We have to contact watchman and ask the set of\n> changed paths since $TIME where $TIME is the last time we updated\n> untracked cache and invalidate those paths in core. Then it should\n> work correctly. I checked the watchman query in the fsmonitor hook and\n> I think it's correct so far.\n\nNo I think I'm wrong. And worse, I think the interaction between FSNM\nand UNTR extension is broken. This is partly from me guessing how\nfsmonitor works so I may be double-wrong.\n\nUNTR extension is supposed to cover the untracked state at timestamp\n$X. Where $X is stored in FSNM extension. Correct?\n\nWhen fsmonitor is used and read_directory() is executed (at timestamp\n$Y), we ask watchman \"what's new on worktree since $X?\"). We get the\nlist, we invalidate relevant directories so we can see new untracked\nentries (or lack of old untracked entries). We write FSNM with\ntimestamp $Y down. We may or may not write UNTR down because of this\npatch, but let's suppose we do. All is good. UNTR now records the\nstate at $Y. FSNM says $Y. This is how it works (when there's no bugs)\n\nUNTR extension is only updated when read_directory() is called. It\ndoes not always happen. FSNM is updated all the time (almost at every\ngit command since most of them needs to read index, many write it\ndown) with new timestamp. Suppose FSNM now records timestamp $Z, UNTR\nstill records the state at $Y because in the last index update,\nread_directory() is not executed. 4 files have been added between $Y\nand $Z. When we ask watchman \"what's new since $Z?\" we will not see\nthose files, so we don't invalidate relevant directories and\nread_directory() will not see those files.\n\nWhat am I missing?\n-- \nDuy\n"},{"id":"338513","messageId":"xmqqbmh2894e.fsf@gitster-ct.c.googlers.com","threadId":"47761","inReplyTo":"CACsJy8DLP=j-h3knwX9zOpejAfUbv1YJwfB-iw4476oy0hTfxg@mail.gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-06T14:50:57Z","receivedAt":"2018-02-06T14:51:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Please don't do that, at least not this way. cache_changed mask should\n> reflect all dirty parts in .git/index. If UNTR extension is not marked\n> updated, it's legit to just skip generating/writing it down (e.g. if I\n> kept the old UNTR extension from the last time I read .git/index\n> around in memory)\n\nExcellent point.  Thanks for mentioning this (it crossed my mind\nwhen I responded but I forgot to metion in my message).\n\n"},{"id":"338604","messageId":"CACsJy8CS6QwAc9=i9JhW3RhLqYsfPM_OWSJ-DCvEAftOKDWs2w@mail.gmail.com","threadId":"47761","inReplyTo":"CACsJy8C+9Det0zF4+HZ6TL36aCFgboEh4=3yrEk5WvyUD30Drw@mail.gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-07T10:59:13Z","receivedAt":"2018-02-07T10:59:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 6, 2018 at 7:55 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Tue, Feb 6, 2018 at 7:27 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> On Tue, Feb 6, 2018 at 8:48 AM, Ben Peart <peartben@gmail.com> wrote:\n>>> With the new behavior, making a change in dir1/, then calling status would\n>>> update the dir1/ untracked cache entry but not write it out. On the next\n>>> status, git would detect the change in dir1/ again and update the untracked\n>>\n>> Thing only missing piece here I would add is, this dir1/ detection is\n>> done by watchman. We have to contact watchman and ask the set of\n>> changed paths since $TIME where $TIME is the last time we updated\n>> untracked cache and invalidate those paths in core. Then it should\n>> work correctly. I checked the watchman query in the fsmonitor hook and\n>> I think it's correct so far.\n>\n> No I think I'm wrong. And worse, I think the interaction between FSNM\n> and UNTR extension is broken. This is partly from me guessing how\n> fsmonitor works so I may be double-wrong.\n>\n> UNTR extension is supposed to cover the untracked state at timestamp\n> $X. Where $X is stored in FSNM extension. Correct?\n>\n> When fsmonitor is used and read_directory() is executed (at timestamp\n> $Y), we ask watchman \"what's new on worktree since $X?\"). We get the\n> list, we invalidate relevant directories so we can see new untracked\n> entries (or lack of old untracked entries). We write FSNM with\n> timestamp $Y down. We may or may not write UNTR down because of this\n> patch, but let's suppose we do. All is good. UNTR now records the\n> state at $Y. FSNM says $Y. This is how it works (when there's no bugs)\n>\n> UNTR extension is only updated when read_directory() is called. It\n> does not always happen. FSNM is updated all the time (almost at every\n\nI was indeed doubly wrong. When FSMN is read, it does make calls to\ninvalidate stuff in untracked cache data structure. If the index is\nwritten down (with updated FSMN extension and timestamp) then the UNTR\nextension, which is generated from in-core untracked data structure,\nalso reflects the damages by the changed paths so next time even if\nthose changed paths are not reported again (between $Y and $Z below),\nit's fine.\n\nAll is good in the world again :)\n\n> git command since most of them needs to read index, many write it\n> down) with new timestamp. Suppose FSNM now records timestamp $Z, UNTR\n> still records the state at $Y because in the last index update,\n> read_directory() is not executed. 4 files have been added between $Y\n> and $Z. When we ask watchman \"what's new since $Z?\" we will not see\n> those files, so we don't invalidate relevant directories and\n> read_directory() will not see those files.\n-- \nDuy\n"},{"id":"338622","messageId":"900d87a2-97e2-a018-70ff-610c707e93de@gmail.com","threadId":"47761","inReplyTo":"CACsJy8CS6QwAc9=i9JhW3RhLqYsfPM_OWSJ-DCvEAftOKDWs2w@mail.gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-07T13:46:48Z","receivedAt":"2018-02-07T13:46:56Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/7/2018 5:59 AM, Duy Nguyen wrote:\n> On Tue, Feb 6, 2018 at 7:55 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> On Tue, Feb 6, 2018 at 7:27 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>>> On Tue, Feb 6, 2018 at 8:48 AM, Ben Peart <peartben@gmail.com> wrote:\n>>>> With the new behavior, making a change in dir1/, then calling status would\n>>>> update the dir1/ untracked cache entry but not write it out. On the next\n>>>> status, git would detect the change in dir1/ again and update the untracked\n>>>\n>>> Thing only missing piece here I would add is, this dir1/ detection is\n>>> done by watchman. We have to contact watchman and ask the set of\n>>> changed paths since $TIME where $TIME is the last time we updated\n>>> untracked cache and invalidate those paths in core. Then it should\n>>> work correctly. I checked the watchman query in the fsmonitor hook and\n>>> I think it's correct so far.\n>>\n>> No I think I'm wrong. And worse, I think the interaction between FSNM\n>> and UNTR extension is broken. This is partly from me guessing how\n>> fsmonitor works so I may be double-wrong.\n>>\n>> UNTR extension is supposed to cover the untracked state at timestamp\n>> $X. Where $X is stored in FSNM extension. Correct?\n>>\n>> When fsmonitor is used and read_directory() is executed (at timestamp\n>> $Y), we ask watchman \"what's new on worktree since $X?\"). We get the\n>> list, we invalidate relevant directories so we can see new untracked\n>> entries (or lack of old untracked entries). We write FSNM with\n>> timestamp $Y down. We may or may not write UNTR down because of this\n>> patch, but let's suppose we do. All is good. UNTR now records the\n>> state at $Y. FSNM says $Y. This is how it works (when there's no bugs)\n>>\n>> UNTR extension is only updated when read_directory() is called. It\n>> does not always happen. FSNM is updated all the time (almost at every\n> \n> I was indeed doubly wrong. When FSMN is read, it does make calls to\n> invalidate stuff in untracked cache data structure. If the index is\n> written down (with updated FSMN extension and timestamp) then the UNTR\n> extension, which is generated from in-core untracked data structure,\n> also reflects the damages by the changed paths so next time even if\n> those changed paths are not reported again (between $Y and $Z below),\n> it's fine.\n> \n> All is good in the world again :)\n\nThank you for looking into this closely.  It's always good to have \nsomeone else examine the logic to make sure there aren't errors that \nwere missed.  Sorry my responses have been slow, my day job has had me \nbusy lately.\n\n> \n>> git command since most of them needs to read index, many write it\n>> down) with new timestamp. Suppose FSNM now records timestamp $Z, UNTR\n>> still records the state at $Y because in the last index update,\n>> read_directory() is not executed. 4 files have been added between $Y\n>> and $Z. When we ask watchman \"what's new since $Z?\" we will not see\n>> those files, so we don't invalidate relevant directories and\n>> read_directory() will not see those files.\n"},{"id":"338625","messageId":"0039c71c-cefd-4950-aa7c-ffbb7cf66e49@gmail.com","threadId":"47761","inReplyTo":"CACsJy8DLP=j-h3knwX9zOpejAfUbv1YJwfB-iw4476oy0hTfxg@mail.gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-07T14:13:28Z","receivedAt":"2018-02-07T14:13:36Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/6/2018 7:27 AM, Duy Nguyen wrote:\n> \n> This is another thing that bugs me. I know you're talking about huge\n> index files, but at what size should we start this sort of\n> optimization? Writing down a few MBs on linux is cheap enough that I\n> won't bother optimizing (and I get my UNTR extension repaired all the\n> time, so reduced lstat calls and stuff). This \"typically\" only comes\n> at certain size, what size?\n> \n\nIt's important to identify what we're trading off here.  With the \nproposed optimization, we'll end up doing less writes of the index but \npotentially more lstat calls.  Even with a small index, writing the \nindex is much more expensive than calling lstat on a file.  Exactly how \nmuch more expensive depends on a lot of variables but even with a SSD \nits still orders of magnitude difference.\n\nThat means we could potentially lstat hundreds or thousands of files and \nstill come out ahead.  Given the untracked cache works at the directory \nlevel, the lstat cost actually scales with the number of directories \nthat have had changes (ie changing a 2nd file in the same directory \ndoesn't add any additional cost).  In \"typical\" workflows, developers \ndon't often change hundreds of files across different directories so \nwe'd \"typically\" still come out ahead.\n\nWe have internal performance tests based on common developer sequences \nof commands (status, edit a file, status, add, status, commit, log, \npush, etc) that show that having the untracked cache turned on actually \nmakes these sequences slower.  At the time, we didn't have time to \ninvestigate why so we just turned off the untracked cache.\n\nWhen writing the fsmonitor patch series which can utilize the untracked \ncache, I did some investigation into why the untracked cache was slowing \nthings down in these tests and discovered the cause was the additional \nindex writes.  That is what led to this patch.\n\nI've been sitting on it for months now because I didn't have the time to \nwrite some performance tests for the git perf framework to demonstrate \nthe problem and how this helps solve it.  With the discussion about the \nfsmonitor extension, I thought this might be a good time to send it out \nthere.\n\nIf you have the cycles, time a typical series of commands with and \nwithout the untracked cache (ie don't just call status over and over in \na loop) and you should see the same results.  Given my limited time \nright now, I'm OK putting this on the back burner again until a git perf \ntest can be written to ensure it actually speeds things up as advertised.\n"},{"id":"338693","messageId":"20180208103356.GA30524@sigill.intra.peff.net","threadId":"47761","inReplyTo":"20180205195619.31064-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-08T10:33:56Z","receivedAt":"2018-02-08T10:34:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 05, 2018 at 02:56:19PM -0500, Ben Peart wrote:\n\n> diff --git a/dir.c b/dir.c\n> index 7c4b45e30e..da93374f0c 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2297,7 +2297,8 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n>  \t\t\t\t dir->untracked->gitignore_invalidated,\n>  \t\t\t\t dir->untracked->dir_invalidated,\n>  \t\t\t\t dir->untracked->dir_opened);\n> -\t\tif (dir->untracked == istate->untracked &&\n> +\t\tif (getenv(\"GIT_TEST_UNTRACKED_CACHE\") &&\n\nA minor nit, but please use something like:\n\n  if (git_env_bool(\"GIT_TEST_UNTRACKED_CACHE\", 0) && ...\n\nso that:\n\n  GIT_TEST_UNTRACKED_CACHE=false\n\ndoes what one might expect, and not the opposite.\n\nTwo other thoughts:\n\n  - it may be worth memo-izing it with a static variable to avoid\n    repeatedly calling the possibly-slow getenv()\n\n  - I agree with the sentiment elsewhere that something like\n    GIT_FORCE_UNTRACKED_CACHE is probably a better name\n\n(The idea itself seems sound to me, but it's not really my area).\n\n-Peff\n"},{"id":"339075","messageId":"CACsJy8A2=tWpiBOBxmTLHXm6bvjGCdoDEuJEy7PewvnzEQi2Qg@mail.gmail.com","threadId":"47761","inReplyTo":"0039c71c-cefd-4950-aa7c-ffbb7cf66e49@gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-12T10:20:52Z","receivedAt":"2018-02-12T10:21:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 7, 2018 at 9:13 PM, Ben Peart <peartben@gmail.com> wrote:\n>\n>\n> On 2/6/2018 7:27 AM, Duy Nguyen wrote:\n>>\n>>\n>> This is another thing that bugs me. I know you're talking about huge\n>> index files, but at what size should we start this sort of\n>> optimization? Writing down a few MBs on linux is cheap enough that I\n>> won't bother optimizing (and I get my UNTR extension repaired all the\n>> time, so reduced lstat calls and stuff). This \"typically\" only comes\n>> at certain size, what size?\n>>\n>\n> It's important to identify what we're trading off here.  With the proposed\n> optimization, we'll end up doing less writes of the index but potentially\n> more lstat calls.  Even with a small index, writing the index is much more\n> expensive than calling lstat on a file.  Exactly how much more expensive\n> depends on a lot of variables but even with a SSD its still orders of\n> magnitude difference.\n\nKeep in mind it's not just about lstat() calls. Processing ignore\npatterns also takes a big chunk of CPU, especially when you have more\nthan a couple ignore rules.\n\nI'm not convinced that writing index files is that expensive for small\nfiles. I don't know about Windows, with Linux it seems fast to me.\nActually, for small scale repos, performance probably does not differ\nmuch either.\n\n> That means we could potentially lstat hundreds or thousands of files and\n> still come out ahead.  Given the untracked cache works at the directory\n> level, the lstat cost actually scales with the number of directories that\n> have had changes (ie changing a 2nd file in the same directory doesn't add\n> any additional cost).  In \"typical\" workflows, developers don't often change\n> hundreds of files across different directories so we'd \"typically\" still\n> come out ahead.\n\nI agree. But we must deal with the bad case when someone\n\"accidentally\" does that. We should not wait until them yell up \"it's\ntoo slow now\" and tell them to disable/enable untracked cache again.\n\nAnother case that could touches a lot of directories over time is\nswitch trees (e.g. \"git checkout master\" then \"checkout next\" or worse\n\"checkout v1.0\").\n\n> We have internal performance tests based on common developer sequences of\n> commands (status, edit a file, status, add, status, commit, log, push, etc)\n> that show that having the untracked cache turned on actually makes these\n> sequences slower.  At the time, we didn't have time to investigate why so we\n> just turned off the untracked cache.\n>\n> When writing the fsmonitor patch series which can utilize the untracked\n> cache, I did some investigation into why the untracked cache was slowing\n> things down in these tests and discovered the cause was the additional index\n> writes.  That is what led to this patch.\n>\n> I've been sitting on it for months now because I didn't have the time to\n> write some performance tests for the git perf framework to demonstrate the\n> problem and how this helps solve it.  With the discussion about the\n> fsmonitor extension, I thought this might be a good time to send it out\n> there.\n\nHopefully you have time to get some of those numbers :) A patch is\nmore convincing when it's backed by numbers. And I'm still not\nconvinced that never ever letting untracked cache be written to the\nindex again is a right thing to do [1].\n\n> If you have the cycles, time a typical series of commands with and without\n> the untracked cache (ie don't just call status over and over in a loop) and\n> you should see the same results.  Given my limited time right now, I'm OK\n> putting this on the back burner again until a git perf test can be written\n> to ensure it actually speeds things up as advertised.\n\n[1] Another case that we must _sometimes_ let untracked cache be\nwritten down is to correct its data. My last invalidation bug, for\nexample, could leave the untracked cache in a bad state, when you run\nwith new git then it should be able to correct the data and write\ndown. But if you don't allow writing down, the new 'git' will keep\nseeing the old errors and keep complaining.\n-- \nDuy\n"},{"id":"339086","messageId":"d1371a6c-2b30-515a-372e-4fea9bc09c43@gmail.com","threadId":"47761","inReplyTo":"CACsJy8A2=tWpiBOBxmTLHXm6bvjGCdoDEuJEy7PewvnzEQi2Qg@mail.gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-02-12T17:57:51Z","receivedAt":"2018-02-12T17:58:06Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 2/12/2018 5:20 AM, Duy Nguyen wrote:\n> On Wed, Feb 7, 2018 at 9:13 PM, Ben Peart <peartben@gmail.com> wrote:\n>>\n>> On 2/6/2018 7:27 AM, Duy Nguyen wrote:\n>>>\n>>> This is another thing that bugs me. I know you're talking about huge\n>>> index files, but at what size should we start this sort of\n>>> optimization? Writing down a few MBs on linux is cheap enough that I\n>>> won't bother optimizing (and I get my UNTR extension repaired all the\n>>> time, so reduced lstat calls and stuff). This \"typically\" only comes\n>>> at certain size, what size?\n>>>\n>> It's important to identify what we're trading off here.  With the proposed\n>> optimization, we'll end up doing less writes of the index but potentially\n>> more lstat calls.  Even with a small index, writing the index is much more\n>> expensive than calling lstat on a file.  Exactly how much more expensive\n>> depends on a lot of variables but even with a SSD its still orders of\n>> magnitude difference.\n> Keep in mind it's not just about lstat() calls. Processing ignore\n> patterns also takes a big chunk of CPU, especially when you have more\n> than a couple ignore rules.\n\nYes, I'm very familiar with the cost of the exclude list pattern \nmatching code.  I've rewritten it to use a hashmap (for those patterns \nwhere it is possible) that dramatically speeds that aspect up as we tend \nto abuse it pretty heavily (~60K entries on average).\n\n>\n> I'm not convinced that writing index files is that expensive for small\n> files. I don't know about Windows, with Linux it seems fast to me.\n> Actually, for small scale repos, performance probably does not differ\n> much either.\n\nI agree.  For small scale repos, none of this is significant enough to \nmatter.  Untracked caching should help most as your working directory \nstarts to get large.\n\n>> That means we could potentially lstat hundreds or thousands of files and\n>> still come out ahead.  Given the untracked cache works at the directory\n>> level, the lstat cost actually scales with the number of directories that\n>> have had changes (ie changing a 2nd file in the same directory doesn't add\n>> any additional cost).  In \"typical\" workflows, developers don't often change\n>> hundreds of files across different directories so we'd \"typically\" still\n>> come out ahead.\n> I agree. But we must deal with the bad case when someone\n> \"accidentally\" does that. We should not wait until them yell up \"it's\n> too slow now\" and tell them to disable/enable untracked cache again.\n>\n> Another case that could touches a lot of directories over time is\n> switch trees (e.g. \"git checkout master\" then \"checkout next\" or worse\n> \"checkout v1.0\").\n\nYou're example above makes me wonder if you understand what my patch is \ndoing.  If the index is flagged as dirty for _any_ reason, the entire \nindex is written to disk with the latest information - including the \nupdated untracked cache and all other extensions.  So in your checkout \nexamples above, the index will still get written to disk with the \nupdated untracked cache extension.  There would be zero change in \nbehavior or performance.  The _only_ time the index is not written to \ndisk after my patch is if there were no other changes to the index.  In \nmy experience, that is only status calls.\n\nTo suffer any degradation in the untracked cache would be if a user \nedited a bunch of files across multiple directories and called status \nrepeatedly.  As soon as they did add, checkout, rm, rebase, etc (ie most \ngit commands other than status) the index would get flagged as dirty and \nthe latest untracked cache extension would get written to disk.\n\n>> We have internal performance tests based on common developer sequences of\n>> commands (status, edit a file, status, add, status, commit, log, push, etc)\n>> that show that having the untracked cache turned on actually makes these\n>> sequences slower.  At the time, we didn't have time to investigate why so we\n>> just turned off the untracked cache.\n>>\n>> When writing the fsmonitor patch series which can utilize the untracked\n>> cache, I did some investigation into why the untracked cache was slowing\n>> things down in these tests and discovered the cause was the additional index\n>> writes.  That is what led to this patch.\n>>\n>> I've been sitting on it for months now because I didn't have the time to\n>> write some performance tests for the git perf framework to demonstrate the\n>> problem and how this helps solve it.  With the discussion about the\n>> fsmonitor extension, I thought this might be a good time to send it out\n>> there.\n> Hopefully you have time to get some of those numbers :) A patch is\n> more convincing when it's backed by numbers. And I'm still not\n> convinced that never ever letting untracked cache be written to the\n> index again is a right thing to do [1].\n\nI agree that any performance patch should be accompanied by a \nperformance test to demonstrate it actually performs as promised.  I \nlooked for but didn't find a performance test for the untracked cache so \nwill have to create one from scratch.  In order to make that as \nrealistic as possible, it needs to simulate (as much as possible) \ntypical developer workflows, ie  create/edit files across multiple \ndirectories and then execute various common commands (status, add, \nstatus, add, status, rm, status, commit, log, rebase, etc) and time the \nperformance of that sequence of commands.  All doable, that just isn't \nsupported well in the performance framework yet so will take some time \n(much more than the actual patch :)).\n\n>> If you have the cycles, time a typical series of commands with and without\n>> the untracked cache (ie don't just call status over and over in a loop) and\n>> you should see the same results.  Given my limited time right now, I'm OK\n>> putting this on the back burner again until a git perf test can be written\n>> to ensure it actually speeds things up as advertised.\n> [1] Another case that we must _sometimes_ let untracked cache be\n> written down is to correct its data. My last invalidation bug, for\n> example, could leave the untracked cache in a bad state, when you run\n> with new git then it should be able to correct the data and write\n> down. But if you don't allow writing down, the new 'git' will keep\n> seeing the old errors and keep complaining.\n\n"},{"id":"339175","messageId":"CACsJy8BWh=e6_Sz20djd5ixxm_Pwa43OHNzXrBatm14k0P+qxQ@mail.gmail.com","threadId":"47761","inReplyTo":"d1371a6c-2b30-515a-372e-4fea9bc09c43@gmail.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-13T09:57:20Z","receivedAt":"2018-02-13T09:58:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 13, 2018 at 12:57 AM, Ben Peart <peartben@gmail.com> wrote:\n>> Another case that could touches a lot of directories over time is\n>> switch trees (e.g. \"git checkout master\" then \"checkout next\" or worse\n>> \"checkout v1.0\").\n>\n>\n> You're example above makes me wonder if you understand what my patch is\n> doing.  If the index is flagged as dirty for _any_ reason, the entire index\n> is written to disk with the latest information - including the updated\n> untracked cache and all other extensions.  So in your checkout examples\n> above, the index will still get written to disk with the updated untracked\n> cache extension.  There would be zero change in behavior or performance.\n> The _only_ time the index is not written to disk after my patch is if there\n> were no other changes to the index.  In my experience, that is only status\n> calls.\n\nThe untracked cache is updated and does get written down, but it's not\n\"repaired\" unless you have called read_directory() before the index is\nwritten. Though paths that hit untracked_cache_invalidate_path() will\ncontinue on slow path until you call read_directory() and write down.\nI don't think \"git checkout\" calls read_directory. There are some\ncommands, like \"git add\", that update the index and call\nread_directory() at the same time. So yes I was wrong, the untracked\ncache can be repaired sometimes, not never repaired.\n\nWe do try to improve performance at \"git checkout\" and a couple other\n\"slow\" commands though (e.g. repair cache tree), perhaps we can do the\nsame for untracked cache. Though the cost of updating untracked cache\nis potentially much higher than cache tree.\n-- \nDuy\n"},{"id":"340620","messageId":"xmqqk1uwkdp4.fsf@gitster-ct.c.googlers.com","threadId":"47761","inReplyTo":"20180208103356.GA30524@sigill.intra.peff.net","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T21:27:03Z","receivedAt":"2018-02-28T21:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> A minor nit, but please use something like:\n>\n>   if (git_env_bool(\"GIT_TEST_UNTRACKED_CACHE\", 0) && ...\n>\n> so that:\n>\n>   GIT_TEST_UNTRACKED_CACHE=false\n>\n> does what one might expect, and not the opposite.\n>\n> Two other thoughts:\n>\n>   - it may be worth memo-izing it with a static variable to avoid\n>     repeatedly calling the possibly-slow getenv()\n>\n>   - I agree with the sentiment elsewhere that something like\n>     GIT_FORCE_UNTRACKED_CACHE is probably a better name\n>\n> (The idea itself seems sound to me, but it's not really my area).\n\nSomehow this topic has been hanging without getting further\nattention for too long.  It's time to wrap it up and moving it\nforward.  How about this?\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Wed, 28 Feb 2018 13:21:09 -0800\nSubject: [PATCH] untracked cache: use git_env_bool() not getenv() for customization\n\nGIT_DISABLE_UNTRACKED_CACHE and GIT_TEST_UNTRACKED_CACHE are only\nsensed for their presense by using getenv(); use git_env_bool()\ninstead so that GIT_DISABLE_UNTRACKED_CACHE=false would work as\nnaïvely expected.\n\nAlso rename GIT_TEST_UNTRACKED_CACHE to GIT_FORCE_UNTRACKED_CACHE\nto express what it does more honestly.  Forcing its use may be one\nuseful thing to do while testing the feature, but testing does not\nhave to be the only use of the knob.\n\nWhile at it, avoid repeated calls to git_env_bool() by capturing the\nreturn value from the first call in a static variable.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n dir.c                             | 14 ++++++++++++--\n t/t7063-status-untracked-cache.sh |  4 ++--\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex da93374f0c..d445d77e62 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2164,8 +2164,13 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \t\t\t\t\t\t      const struct pathspec *pathspec)\n {\n \tstruct untracked_cache_dir *root;\n+\tstatic int untracked_cache_disabled = -1;\n \n-\tif (!dir->untracked || getenv(\"GIT_DISABLE_UNTRACKED_CACHE\"))\n+\tif (!dir->untracked)\n+\t\treturn NULL;\n+\tif (untracked_cache_disabled < 0)\n+\t\tuntracked_cache_disabled = git_env_bool(\"GIT_DISABLE_UNTRACKED_CACHE\", 0);\n+\tif (untracked_cache_disabled)\n \t\treturn NULL;\n \n \t/*\n@@ -2287,7 +2292,12 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t}\n \n \tif (dir->untracked) {\n+\t\tstatic int force_untracked_cache = -1;\n \t\tstatic struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS);\n+\n+\t\tif (force_untracked_cache < 0)\n+\t\t\tforce_untracked_cache =\n+\t\t\t\tgit_env_bool(\"GIT_FORCE_UNTRACKED_CACHE\", 0);\n \t\ttrace_printf_key(&trace_untracked_stats,\n \t\t\t\t \"node creation: %u\\n\"\n \t\t\t\t \"gitignore invalidation: %u\\n\"\n@@ -2297,7 +2307,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\t\t\t dir->untracked->gitignore_invalidated,\n \t\t\t\t dir->untracked->dir_invalidated,\n \t\t\t\t dir->untracked->dir_opened);\n-\t\tif (getenv(\"GIT_TEST_UNTRACKED_CACHE\") &&\n+\t\tif (force_untracked_cache &&\n \t\t\tdir->untracked == istate->untracked &&\n \t\t    (dir->untracked->dir_opened ||\n \t\t     dir->untracked->gitignore_invalidated ||\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 6ef520e823..9cb16ca36d 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -14,8 +14,8 @@ test_description='test untracked cache'\n # See <20160803174522.5571-1-pclouds@gmail.com> if you want to know\n # more.\n \n-GIT_TEST_UNTRACKED_CACHE=true\n-export GIT_TEST_UNTRACKED_CACHE\n+GIT_FORCE_UNTRACKED_CACHE=true\n+export GIT_FORCE_UNTRACKED_CACHE\n \n sync_mtime () {\n \tfind . -type d -ls >/dev/null\n-- \n2.16.2-325-g2fc74f41c5\n\n"},{"id":"340691","messageId":"20180301074245.GF31079@sigill.intra.peff.net","threadId":"47761","inReplyTo":"xmqqk1uwkdp4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-01T07:42:45Z","receivedAt":"2018-03-01T07:42:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 28, 2018 at 01:27:03PM -0800, Junio C Hamano wrote:\n\n> Somehow this topic has been hanging without getting further\n> attention for too long.  It's time to wrap it up and moving it\n> forward.  How about this?\n> \n> -- >8 --\n> From: Junio C Hamano <gitster@pobox.com>\n> Date: Wed, 28 Feb 2018 13:21:09 -0800\n> Subject: [PATCH] untracked cache: use git_env_bool() not getenv() for customization\n> [...]\n\nThis looks good to me. Thanks for tying up the loose end.\n\n-Peff\n"},{"id":"340722","messageId":"a43a4408-6889-772a-58bf-c1b2feb7cf51@gmail.com","threadId":"47761","inReplyTo":"20180301074245.GF31079@sigill.intra.peff.net","subject":"Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-03-01T12:35:28Z","receivedAt":"2018-03-01T12:35:39Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 3/1/2018 2:42 AM, Jeff King wrote:\n> On Wed, Feb 28, 2018 at 01:27:03PM -0800, Junio C Hamano wrote:\n> \n>> Somehow this topic has been hanging without getting further\n>> attention for too long.  It's time to wrap it up and moving it\n>> forward.  How about this?\n>>\n>> -- >8 --\n>> From: Junio C Hamano <gitster@pobox.com>\n>> Date: Wed, 28 Feb 2018 13:21:09 -0800\n>> Subject: [PATCH] untracked cache: use git_env_bool() not getenv() for customization\n>> [...]\n> \n> This looks good to me. Thanks for tying up the loose end.\n> \n> -Peff\n> \n\nLooks good to me as well.  Thanks for fixing the environment variables.\n\nI'm having send-email issues so hope this one gets through.  Please \nignore my [PATCH V2] if it ever makes it through.\n\nBen\n"}]}