{"thread":{"id":"45902","subject":"[PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","startedAt":"2017-05-08T09:41:50Z","lastAt":"2017-05-18T17:31:04Z","messageCount":8,"participants":["Johannes Schindelin","Christian Couder","David Turner","Junio C Hamano","Ben Peart"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"319071","messageId":"cover.1494236457.git.johannes.schindelin@gmx.de","threadId":"45902","inReplyTo":null,"subject":"[PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-08T09:41:22Z","receivedAt":"2017-05-08T09:41:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I recently sent out a request for assistance, after noticing that the\nuntracked cache is simply thrown away after operations such as\n`git checkout` or `git reset --hard`:\n\nhttp://public-inbox.org/git/alpine.DEB.2.20.1705031202470.3480@virtualbox/\n\nDuy responded with some high-level reasoning that it should be possible\nto simply reuse the untracked cache data structure in the new index, as\nhe had a gut feeling that \"we do invalidation right\".\n\nI did not have time to back that up by a thorough analysis of the code,\nbut it turns out that it is unnecessary: Ben Peart pointed me to a patch\nof Dave Turner's that was submitted as part of the watchman series,\naddressing the very issue about which I was concerned.\n\nAnd I trust Dave to have validated the idea that the untracked cache\ninvalidation \"is done right\" even when we simply move the pointer to a\ndifferent index_state struct than originally.\n\nSeeing as the untracked cache being dropped unceremoniously when it\nshould not be dropped, in a surprising number of operations, I think it\nis a sensible change, and important, too, and independent enough from\nthe watchman patches to merit being separated out and applied pretty\nsoon.\n\nSo what I did was simply to drop the two lines from this patch that\nreferred to index_state fields added by Dave's watchman patch series.\n\nPlease do not mistake this for a sign that I am disinterested in\nwatchman support, far from it... stay tuned ;-)\n\nOh, and I adjusted Dave's email address. Dave, is that okay?\n\nAs we are in a feature freeze phase, I was debating whether to send out\nthis patch now or later.\n\nHaving thought about it for quite a bit, I am now convinced that this\npatch fixes a bug in the untracked cache feature that is so critical as\nto render it useless: if you\n\n- have to switch between branches frequently, or\n- rebase frequently (which calls `git reset --hard`), or\n- stash frequently (which calls `git reset --hard`),\n\nit is as if you had not enabled the untracked cache at all. Even worse,\nGit will do a ton of work to recreate the untracked cache and to store\nit as an index extension, *just* to throw the untracked away in the end.\n\n\nDavid Turner (1):\n unpack-trees: preserve index extensions\n\n cache.h | 1 +\n read-cache.c | 6 ++++++\n t/t7063-status-untracked-cache.sh | 22 ++++++++++++++++++++++\n unpack-trees.c | 1 +\n 4 files changed, 30 insertions(+)\n\n\nbase-commit: 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74\nPublished-As: https://github.com/dscho/git/releases/tag/preserve-untracked-cache-v1\nFetch-It-Via: git fetch https://github.com/dscho/git preserve-untracked-cache-v1\n\n-- \n2.12.2.windows.2.800.gede8f145e06\n\n"},{"id":"319072","messageId":"52008ec9f5b6c459f596052836cd88addb7167db.1494236457.git.johannes.schindelin@gmx.de","threadId":"45902","inReplyTo":"cover.1494236457.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/1] unpack-trees: preserve index extensions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-08T09:41:42Z","receivedAt":"2017-05-08T09:42:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: David Turner \n\nMake git checkout (and other unpack_tree operations) preserve the\nuntracked cache. This is valuable for two reasons:\n\n1. Often, an unpack_tree operation will not touch large parts of the\nworking tree, and thus most of the untracked cache will continue to be\nvalid.\n\n2. Even if the untracked cache were entirely invalidated by such an\noperation, the user has signaled their intention to have such a cache,\nand we don't want to throw it away.\n\n[jes: backed out the watchman-specific parts]\n\nSigned-off-by: David Turner \nSigned-off-by: Ben Peart \nSigned-off-by: Johannes Schindelin \n---\n cache.h | 1 +\n read-cache.c | 6 ++++++\n t/t7063-status-untracked-cache.sh | 22 ++++++++++++++++++++++\n unpack-trees.c | 1 +\n 4 files changed, 30 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex e1f0e182ad0..d41336dfd5d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -597,6 +597,7 @@ extern int read_index_unmerged(struct index_state *);\n #define CLOSE_LOCK\t\t(1 << 1)\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n extern int discard_index(struct index_state *);\n+extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n extern int unmerged_index(const struct index_state *);\n extern int verify_path(const char *path);\n extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);\ndiff --git a/read-cache.c b/read-cache.c\nindex 0d0081a11b8..79827a4d710 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2628,3 +2628,9 @@ void stat_validity_update(struct stat_validity *sv, int fd)\n \t\tfill_stat_data(sv->sd, &st);\n \t}\n }\n+\n+void move_index_extensions(struct index_state *dst, struct index_state *src)\n+{\n+\tdst->untracked = src->untracked;\n+\tsrc->untracked = NULL;\n+}\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 0667bd9dd3e..e5fb892f957 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -661,4 +661,26 @@ test_expect_success 'test ident field is working' '\n \ttest_i18ncmp ../expect ../err\n '\n \n+test_expect_success 'untracked cache survives a checkout' '\n+\tgit commit --allow-empty -m empty &&\n+\ttest-dump-untracked-cache >../before &&\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout -b other_branch &&\n+\ttest-dump-untracked-cache >../after &&\n+\ttest_cmp ../before ../after &&\n+\ttest_commit test &&\n+\ttest-dump-untracked-cache >../before &&\n+\tgit checkout master &&\n+\ttest-dump-untracked-cache >../after &&\n+\ttest_cmp ../before ../after\n+'\n+\n+test_expect_success 'untracked cache survives a commit' '\n+\ttest-dump-untracked-cache >../before &&\n+\tgit add done/two &&\n+\tgit commit -m commit &&\n+\ttest-dump-untracked-cache >../after &&\n+\ttest_cmp ../before ../after\n+'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex aa15111fefc..17117bd0fda 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1391,6 +1391,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t\t\t\t\t WRITE_TREE_SILENT |\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\n \t\t}\n+\t\tmove_index_extensions(&o->result, o->dst_index);\n \t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n \t} else {\n-- \n2.12.2.windows.2.800.gede8f145e06\n"},{"id":"319074","messageId":"CAP8UFD3qoeQE5ZbFHGLE-_B4rOr3X2m-WPB7rb8QAbZA6zMRxg@mail.gmail.com","threadId":"45902","inReplyTo":"cover.1494236457.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-05-08T10:12:28Z","receivedAt":"2017-05-08T10:12:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"(Adding Dave in Cc as it looks like he is involved.)\n\nOn Mon, May 8, 2017 at 11:41 AM, Johannes Schindelin\n wrote:\n> I recently sent out a request for assistance, after noticing that the\n> untracked cache is simply thrown away after operations such as\n> `git checkout` or `git reset --hard`:\n>\n> http://public-inbox.org/git/alpine.DEB.2.20.1705031202470.3480@virtualbox/\n>\n> Duy responded with some high-level reasoning that it should be possible\n> to simply reuse the untracked cache data structure in the new index, as\n> he had a gut feeling that \"we do invalidation right\".\n>\n> I did not have time to back that up by a thorough analysis of the code,\n> but it turns out that it is unnecessary: Ben Peart pointed me to a patch\n> of Dave Turner's that was submitted as part of the watchman series,\n> addressing the very issue about which I was concerned.\n>\n> And I trust Dave to have validated the idea that the untracked cache\n> invalidation \"is done right\" even when we simply move the pointer to a\n> different index_state struct than originally.\n>\n> Seeing as the untracked cache being dropped unceremoniously when it\n> should not be dropped, in a surprising number of operations, I think it\n> is a sensible change, and important, too, and independent enough from\n> the watchman patches to merit being separated out and applied pretty\n> soon.\n>\n> So what I did was simply to drop the two lines from this patch that\n> referred to index_state fields added by Dave's watchman patch series.\n>\n> Please do not mistake this for a sign that I am disinterested in\n> watchman support, far from it... stay tuned ;-)\n>\n> Oh, and I adjusted Dave's email address. Dave, is that okay?\n>\n> As we are in a feature freeze phase, I was debating whether to send out\n> this patch now or later.\n>\n> Having thought about it for quite a bit, I am now convinced that this\n> patch fixes a bug in the untracked cache feature that is so critical as\n> to render it useless: if you\n>\n> - have to switch between branches frequently, or\n> - rebase frequently (which calls `git reset --hard`), or\n> - stash frequently (which calls `git reset --hard`),\n>\n> it is as if you had not enabled the untracked cache at all. Even worse,\n> Git will do a ton of work to recreate the untracked cache and to store\n> it as an index extension, *just* to throw the untracked away in the end.\n>\n>\n> David Turner (1):\n> unpack-trees: preserve index extensions\n>\n> cache.h | 1 +\n> read-cache.c | 6 ++++++\n> t/t7063-status-untracked-cache.sh | 22 ++++++++++++++++++++++\n> unpack-trees.c | 1 +\n> 4 files changed, 30 insertions(+)\n>\n>\n> base-commit: 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74\n> Published-As: https://github.com/dscho/git/releases/tag/preserve-untracked-cache-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git preserve-untracked-cache-v1\n>\n> --\n> 2.12.2.windows.2.800.gede8f145e06\n>\n"},{"id":"319085","messageId":"2c0be7c0a8ce4f5ea49dfaf3f8511bc0@exmbdft7.ad.twosigma.com","threadId":"45902","inReplyTo":"CAP8UFD3qoeQE5ZbFHGLE-_B4rOr3X2m-WPB7rb8QAbZA6zMRxg@mail.gmail.com","subject":"RE: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-05-08T15:58:12Z","receivedAt":"2017-05-08T15:58:29Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"Can you actually keep the email address as my Twopensource one? I want to make sure that Twitter, my employer at the time, gets credit for this work (just as I want to make sure that my current employer, Two Sigma, gets credit for my current work).\n\nPlease feel free to add Signed-off-by: David Turner in case that makes tracking easier.\n\nThanks.\n\nWRT the actual patch, I want to note that past me did not do a great job here. The tests do not correctly check that the post-checkout untracked cache is still valid after a checkout. For example, let's say that previously, the directory foo was entirely untracked (but it contained a file bar), but after the checkout, there is a file foo/baz. Does the untracked cache need to get updated? \n\nUnfortunately, the untracked cache is very unlikely to make it to the top of my priority list any time soon, so I won't be able to correct this test (and, if necessary, correct the code). But I would strongly suggest that the test be improved before this code is merged.\n\nThanks for CCing me.\n\n> -----Original Message-----\n> From: Christian Couder [mailto:christian.couder@gmail.com]\n> Sent: Monday, May 8, 2017 6:12 AM\n> To: Johannes Schindelin \n> Cc: git ; Junio C Hamano ; Nguyễn\n> Thái Ngọc Duy ; Ben Peart ;\n> David Turner \n> Subject: Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --\n> hard, etc\n> \n> (Adding Dave in Cc as it looks like he is involved.)\n> \n> On Mon, May 8, 2017 at 11:41 AM, Johannes Schindelin\n> wrote:\n> > I recently sent out a request for assistance, after noticing that the\n> > untracked cache is simply thrown away after operations such as `git\n> > checkout` or `git reset --hard`:\n> >\n> > http://public-inbox.org/git/alpine.DEB.2.20.1705031202470.3480@virtual\n> > box/\n> >\n> > Duy responded with some high-level reasoning that it should be\n> > possible to simply reuse the untracked cache data structure in the new\n> > index, as he had a gut feeling that \"we do invalidation right\".\n> >\n> > I did not have time to back that up by a thorough analysis of the\n> > code, but it turns out that it is unnecessary: Ben Peart pointed me to\n> > a patch of Dave Turner's that was submitted as part of the watchman\n> > series, addressing the very issue about which I was concerned.\n> >\n> > And I trust Dave to have validated the idea that the untracked cache\n> > invalidation \"is done right\" even when we simply move the pointer to a\n> > different index_state struct than originally.\n> >\n> > Seeing as the untracked cache being dropped unceremoniously when it\n> > should not be dropped, in a surprising number of operations, I think\n> > it is a sensible change, and important, too, and independent enough\n> > from the watchman patches to merit being separated out and applied\n> > pretty soon.\n> >\n> > So what I did was simply to drop the two lines from this patch that\n> > referred to index_state fields added by Dave's watchman patch series.\n> >\n> > Please do not mistake this for a sign that I am disinterested in\n> > watchman support, far from it... stay tuned ;-)\n> >\n> > Oh, and I adjusted Dave's email address. Dave, is that okay?\n> >\n> > As we are in a feature freeze phase, I was debating whether to send\n> > out this patch now or later.\n> >\n> > Having thought about it for quite a bit, I am now convinced that this\n> > patch fixes a bug in the untracked cache feature that is so critical\n> > as to render it useless: if you\n> >\n> > - have to switch between branches frequently, or\n> > - rebase frequently (which calls `git reset --hard`), or\n> > - stash frequently (which calls `git reset --hard`),\n> >\n> > it is as if you had not enabled the untracked cache at all. Even\n> > worse, Git will do a ton of work to recreate the untracked cache and\n> > to store it as an index extension, *just* to throw the untracked away in the\n> end.\n> >\n> >\n> > David Turner (1):\n> > unpack-trees: preserve index extensions\n> >\n> > cache.h | 1 +\n> > read-cache.c | 6 ++++++\n> > t/t7063-status-untracked-cache.sh | 22 ++++++++++++++++++++++\n> > unpack-trees.c | 1 +\n> > 4 files changed, 30 insertions(+)\n> >\n> >\n> > base-commit: 4fa66c85f11bc5a541462ca5ae3246aa0ce02e74\n> > Published-As:\n> > https://github.com/dscho/git/releases/tag/preserve-untracked-cache-v1\n> > Fetch-It-Via: git fetch https://github.com/dscho/git\n> > preserve-untracked-cache-v1\n> >\n> > --\n> > 2.12.2.windows.2.800.gede8f145e06\n> >\n"},{"id":"319167","messageId":"xmqqy3u639n4.fsf@gitster.mtv.corp.google.com","threadId":"45902","inReplyTo":"2c0be7c0a8ce4f5ea49dfaf3f8511bc0@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T05:02:39Z","receivedAt":"2017-05-09T05:02:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> Can you actually keep the email address as my Twopensource one? I want to make sure that Twitter, my employer at the time, gets credit for this work (just as I want to make sure that my current employer, Two Sigma, gets credit for my current work).\n>\n> Please feel free to add Signed-off-by: David Turner in case that makes tracking easier.\n>\n> Thanks.\n>\n> WRT the actual patch, I want to note that past me did not do a\n> great job here. The tests do not correctly check that the\n> post-checkout untracked cache is still valid after a checkout.\n> For example, let's say that previously, the directory foo was\n> entirely untracked (but it contained a file bar), but after the\n> checkout, there is a file foo/baz. Does the untracked cache need\n> to get updated?\n>\n> Unfortunately, the untracked cache is very unlikely to make it to\n> the top of my priority list any time soon, so I won't be able to\n> correct this test (and, if necessary, correct the code). But I\n> would strongly suggest that the test be improved before this code\n> is merged.\n>\n> Thanks for CCing me.\n\nI will try to find time to tweak what was sent to the list here to\nreflect your affiliations better, but marked with DONTMERGE waiting\nfor the necessary updates you mentioned above, so that this change\nis not forgotten. It may turn out to be that copying from src to\ndst like the patch does is all that is needed, or the cache may need\nfurther invalidation when the copying happens, and I haven't got a\ngood feeling that anybody who are familiar with the codepath vetted\nthe correctness from seeing the discussion from sidelines (yet).\n\nThanks.\n"},{"id":"319185","messageId":"1e21e5c4-3440-a82f-b6c5-1a3e00439054@gmail.com","threadId":"45902","inReplyTo":"xmqqy3u639n4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-09T12:51:11Z","receivedAt":"2017-05-09T12:51:20Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\nOn 5/9/2017 1:02 AM, Junio C Hamano wrote:\n> David Turner writes:\n>\n>> Can you actually keep the email address as my Twopensource one? I want to make sure that Twitter, my employer at the time, gets credit for this work (just as I want to make sure that my current employer, Two Sigma, gets credit for my current work).\n>>\n>> Please feel free to add Signed-off-by: David Turner in case that makes tracking easier.\n>>\n>> Thanks.\n>>\n>> WRT the actual patch, I want to note that past me did not do a\n>> great job here. The tests do not correctly check that the\n>> post-checkout untracked cache is still valid after a checkout.\n>> For example, let's say that previously, the directory foo was\n>> entirely untracked (but it contained a file bar), but after the\n>> checkout, there is a file foo/baz. Does the untracked cache need\n>> to get updated?\n>>\n>> Unfortunately, the untracked cache is very unlikely to make it to\n>> the top of my priority list any time soon, so I won't be able to\n>> correct this test (and, if necessary, correct the code). But I\n>> would strongly suggest that the test be improved before this code\n>> is merged.\n>>\n>> Thanks for CCing me.\n> I will try to find time to tweak what was sent to the list here to\n> reflect your affiliations better, but marked with DONTMERGE waiting\n> for the necessary updates you mentioned above, so that this change\n> is not forgotten. It may turn out to be that copying from src to\n> dst like the patch does is all that is needed, or the cache may need\n> further invalidation when the copying happens, and I haven't got a\n> good feeling that anybody who are familiar with the codepath vetted\n> the correctness from seeing the discussion from sidelines (yet).\n>\n> Thanks.\n\nI've been looking into similar issues with the cache associated with \nusing a file system monitor (aka Watchman) \n(https://github.com/git-for-windows/git/compare/master...benpeart:fsmonitor) \nto speed updating the index to reflect changes in the working directory.\n\nI can take a look and see if this patch results in valid results and \nreply to the thread or resubmit as needed.\n\nBen\n"},{"id":"320171","messageId":"f497712e-0608-704f-6fbb-06124ab8a9b7@gmail.com","threadId":"45902","inReplyTo":"1e21e5c4-3440-a82f-b6c5-1a3e00439054@gmail.com","subject":"Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-05-18T16:58:10Z","receivedAt":"2017-05-18T16:58:59Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/9/2017 8:51 AM, Ben Peart wrote:\n>\n> On 5/9/2017 1:02 AM, Junio C Hamano wrote:\n>> David Turner writes:\n>>\n>>> Can you actually keep the email address as my Twopensource one? I\n>>> want to make sure that Twitter, my employer at the time, gets credit\n>>> for this work (just as I want to make sure that my current employer,\n>>> Two Sigma, gets credit for my current work).\n>>>\n>>> Please feel free to add Signed-off-by: David Turner\n>>> in case that makes tracking easier.\n>>>\n>>> Thanks.\n>>>\n>>> WRT the actual patch, I want to note that past me did not do a\n>>> great job here. The tests do not correctly check that the\n>>> post-checkout untracked cache is still valid after a checkout.\n>>> For example, let's say that previously, the directory foo was\n>>> entirely untracked (but it contained a file bar), but after the\n>>> checkout, there is a file foo/baz. Does the untracked cache need\n>>> to get updated?\n>>>\n>>> Unfortunately, the untracked cache is very unlikely to make it to\n>>> the top of my priority list any time soon, so I won't be able to\n>>> correct this test (and, if necessary, correct the code). But I\n>>> would strongly suggest that the test be improved before this code\n>>> is merged.\n>>>\n>>> Thanks for CCing me.\n>> I will try to find time to tweak what was sent to the list here to\n>> reflect your affiliations better, but marked with DONTMERGE waiting\n>> for the necessary updates you mentioned above, so that this change\n>> is not forgotten. It may turn out to be that copying from src to\n>> dst like the patch does is all that is needed, or the cache may need\n>> further invalidation when the copying happens, and I haven't got a\n>> good feeling that anybody who are familiar with the codepath vetted\n>> the correctness from seeing the discussion from sidelines (yet).\n>>\n>> Thanks.\n>\n> I've been looking into similar issues with the cache associated with\n> using a file system monitor (aka Watchman)\n> (https://github.com/git-for-windows/git/compare/master...benpeart:fsmonitor)\n> to speed updating the index to reflect changes in the working directory.\n>\n> I can take a look and see if this patch results in valid results and\n> reply to the thread or resubmit as needed.\n>\n> Ben\n\nTLDR: the patch looks good from my perspective but I'd like the experts \nto weigh in as well.\n\nAfter digging into the untracked cache code and thinking about whether \nit is reasonable to copy the cache from the old index to the new index \nin unpack_trees() I believe the answer is \"yes.\" I'm not the expert in \nthis code so I'll outline my reasoning here and hopefully the real \nexperts can review it and see if I've missed something.\n\nThe interesting part of the untracked cache for this discussion is the \nlist of untracked_cache_dir structures. Because each directory cache \nentry contains stat_data (esp ctime and mtime) for that directory - the \nexisting logic will detect if that directory has had any changes made in \nit since the cache entry was saved. It doesn't really care when, why, \nor how the change was made, just if one has happened.\n\nI then tried to think of ways that this logic could be broken (like \nDavid's example above) but was unsuccessful in coming up with any. This \nmakes sense because the untracked cache obviously has to correctly \ndetect _any_ change so really doesn't care whether it's cached state was \ninitially saved before or after a call to unpack_trees().\n\nEven scenarios of creating files in sub-directories of sub-directories \nworks because eventually, either is a directory or file is created in a \ncached directory entry which will change the mtime of that directory and \ninvalidate that part of the cache.\n\nUltimately, it is this behavior of saving the mtime of each cached \ndirectory that makes this all work as each entry can be \nvalidated/invalidated separately from all the rest and independently \nfrom the index from which they came.\n\n\nOnce I did the code examination and thinking exercise, I wanted to test \nit out and see if the theory held up. I started out with some manual \ntesting (esp of the scenario David mentioned) and then wrote a couple of \nadditional tests all of which passed.\n\nI then ran all existing git tests with the patch applied and they all \npassed. This only really tells us that it didn't break anything because \nuntracked cache is turned off by default but it does show us that it \nstill passes the untracked cache specific test cases (as they obviously \nturn it on).\n\nI then modified the test_create_repo() function in test-lib-functions.sh \nto turn on the untracked cache feature after creating the test repo and \nran all the tests again twice - the first time without the patch and \nagain with the patch). This run is more interesting because it is \ntesting that having the untracked cache turned (with and without the \npatch) on doesn't break anything.\n\nThere were two test scripts that had failures:\n\nt7063-status-untracked-cache.sh failed the test \"not ok 1 - \ncore.untrackedCache is unset\" This is actually a positive result \nbecause it is showing that I successfully turned on the untracked cache \nfeature.\n\nt1700-split-index.sh failed several tests in both runs (with and without \npatch) and upon examining the tests and their failures they are to be \nexpected and do not indicate any bug. Specifically, the failures were \ncaused because the tests check the sha of the index against a hard coded \nvalue in the test script. Because the untracked cache is turned on, the \nsha of the index does not match that hard coded value. I edited several \nof the tests to update the sha they are checking against to match the \nsha actually generated and the tests pass.\n\nIn the end, both my code examination and all the testing I was able to \ndo give me some confidence that the patch will produce valid results. \nHowever, I'm not the expert in this area so I'd like the experts to \nweigh in on any potential issues this patch may cause that I've missed.\n\nThanks,\n\nBen\n"},{"id":"320176","messageId":"30a3f113e3d14274b2482a869326848c@exmbdft7.ad.twosigma.com","threadId":"45902","inReplyTo":"f497712e-0608-704f-6fbb-06124ab8a9b7@gmail.com","subject":"RE: [PATCH 0/1] Preserve the untracked cache across checkout, reset --hard, etc","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-05-18T17:30:52Z","receivedAt":"2017-05-18T17:31:04Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Ben Peart [mailto:peartben@gmail.com]\n> Sent: Thursday, May 18, 2017 12:58 PM\n> To: Junio C Hamano ; David Turner\n> \n> Cc: 'Christian Couder' ; Johannes Schindelin\n> ; git ; Nguyễn Thái Ngọc\n> Duy ; Ben Peart \n> Subject: Re: [PATCH 0/1] Preserve the untracked cache across checkout, reset --\n> hard, etc\n> \n> \n> \n> On 5/9/2017 8:51 AM, Ben Peart wrote:\n> >\n> > On 5/9/2017 1:02 AM, Junio C Hamano wrote:\n> >> David Turner writes:\n> >>\n> >>> Can you actually keep the email address as my Twopensource one? I\n> >>> want to make sure that Twitter, my employer at the time, gets credit\n> >>> for this work (just as I want to make sure that my current employer,\n> >>> Two Sigma, gets credit for my current work).\n> >>>\n> >>> Please feel free to add Signed-off-by: David Turner\n> >>> in case that makes tracking easier.\n> >>>\n> >>> Thanks.\n> >>>\n> >>> WRT the actual patch, I want to note that past me did not do a great\n> >>> job here. The tests do not correctly check that the post-checkout\n> >>> untracked cache is still valid after a checkout.\n> >>> For example, let's say that previously, the directory foo was\n> >>> entirely untracked (but it contained a file bar), but after the\n> >>> checkout, there is a file foo/baz. Does the untracked cache need to\n> >>> get updated?\n> >>>\n> >>> Unfortunately, the untracked cache is very unlikely to make it to\n> >>> the top of my priority list any time soon, so I won't be able to\n> >>> correct this test (and, if necessary, correct the code). But I\n> >>> would strongly suggest that the test be improved before this code is\n> >>> merged.\n> >>>\n> >>> Thanks for CCing me.\n> >> I will try to find time to tweak what was sent to the list here to\n> >> reflect your affiliations better, but marked with DONTMERGE waiting\n> >> for the necessary updates you mentioned above, so that this change is\n> >> not forgotten. It may turn out to be that copying from src to dst\n> >> like the patch does is all that is needed, or the cache may need\n> >> further invalidation when the copying happens, and I haven't got a\n> >> good feeling that anybody who are familiar with the codepath vetted\n> >> the correctness from seeing the discussion from sidelines (yet).\n> >>\n> >> Thanks.\n> >\n> > I've been looking into similar issues with the cache associated with\n> > using a file system monitor (aka Watchman)\n> > (https://github.com/git-for-windows/git/compare/master...benpeart:fsmo\n> > nitor) to speed updating the index to reflect changes in the working\n> > directory.\n> >\n> > I can take a look and see if this patch results in valid results and\n> > reply to the thread or resubmit as needed.\n> >\n> > Ben\n> \n> TLDR: the patch looks good from my perspective but I'd like the experts to weigh\n> in as well.\n\nThanks for looking into this. I'm glad to learn that I got it right the first time, \nalthough I still wish I had been more assiduous about testing back then. \n\n> After digging into the untracked cache code and thinking about whether it is\n> reasonable to copy the cache from the old index to the new index in\n> unpack_trees() I believe the answer is \"yes.\" I'm not the expert in this code so I'll\n> outline my reasoning here and hopefully the real experts can review it and see if\n> I've missed something.\n> \n> The interesting part of the untracked cache for this discussion is the list of\n> untracked_cache_dir structures. Because each directory cache entry contains\n> stat_data (esp ctime and mtime) for that directory - the existing logic will detect\n> if that directory has had any changes made in it since the cache entry was saved.\n> It doesn't really care when, why, or how the change was made, just if one has\n> happened.\n> \n> I then tried to think of ways that this logic could be broken (like David's example\n> above) but was unsuccessful in coming up with any. This makes sense because\n> the untracked cache obviously has to correctly detect _any_ change so really\n> doesn't care whether it's cached state was initially saved before or after a call to\n> unpack_trees().\n\nIt looks like unpack_trees calls (somewhere -- I didn't investigate the full call chain) \nuntracked_cache_invalidate_entry. It looks like my patch adds the move *after* \nany invalidation, though, so I think this is OK.\n\n> Even scenarios of creating files in sub-directories of sub-directories works\n> because eventually, either is a directory or file is created in a cached directory\n> entry which will change the mtime of that directory and invalidate that part of\n> the cache.\n> \n> Ultimately, it is this behavior of saving the mtime of each cached directory that\n> makes this all work as each entry can be validated/invalidated separately from\n> all the rest and independently from the index from which they came.\n> \n> \n> Once I did the code examination and thinking exercise, I wanted to test it out\n> and see if the theory held up. I started out with some manual testing (esp of the\n> scenario David mentioned) and then wrote a couple of additional tests all of\n> which passed.\n> \n> I then ran all existing git tests with the patch applied and they all passed. This\n> only really tells us that it didn't break anything because untracked cache is\n> turned off by default but it does show us that it still passes the untracked cache\n> specific test cases (as they obviously turn it on).\n> \n> I then modified the test_create_repo() function in test-lib-functions.sh to turn\n> on the untracked cache feature after creating the test repo and ran all the tests\n> again twice - the first time without the patch and again with the patch). This run\n> is more interesting because it is testing that having the untracked cache turned\n> (with and without the\n> patch) on doesn't break anything.\n> \n> There were two test scripts that had failures:\n> \n> t7063-status-untracked-cache.sh failed the test \"not ok 1 - core.untrackedCache\n> is unset\" This is actually a positive result because it is showing that I successfully\n> turned on the untracked cache feature.\n> \n> t1700-split-index.sh failed several tests in both runs (with and without\n> patch) and upon examining the tests and their failures they are to be expected\n> and do not indicate any bug. Specifically, the failures were caused because the\n> tests check the sha of the index against a hard coded value in the test script.\n> Because the untracked cache is turned on, the sha of the index does not match\n> that hard coded value. I edited several of the tests to update the sha they are\n> checking against to match the sha actually generated and the tests pass.\n> \n> In the end, both my code examination and all the testing I was able to do give\n> me some confidence that the patch will produce valid results.\n> However, I'm not the expert in this area so I'd like the experts to weigh in on any\n> potential issues this patch may cause that I've missed.\n\nThis testing seems sufficient to me, assuming that the new automated tests make it \ninto the patch. Thanks for finishing this up -- it had slipped my mind entirely.\n\n"}]}