{"thread":{"id":"32028","subject":"Bug: write-tree corrupts intent-to-add index state","startedAt":"2012-11-06T08:53:27Z","lastAt":"2012-12-15T02:52:52Z","messageCount":16,"participants":["Jonathon Mah","Nguyen Thai Ngoc Duy","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"202585","messageId":"3E62F933-76CD-4578-8684-21444EAA454F@JonathonMah.com","threadId":"32028","inReplyTo":null,"subject":"Bug: write-tree corrupts intent-to-add index state","fromName":"Jonathon Mah","fromEmail":"me@jonathonmah.com","sentAt":"2012-11-06T08:53:27Z","receivedAt":"2012-11-06T08:53:27Z","isPatch":false,"sender":{"key":"me@jonathonmah.com","avatar":"https://avatars.githubusercontent.com/u/2748?v=4"},"body":"Hi revisionaries,\n\nI came across what appears to be a bug while working today. I had several changes, both staged and unstaged, and two files that I had marked with intent-to-add (git add -N).\n\n$ git status -sb\n## Library-3.x...origin/Library-3.x [ahead 4]\nM Library/LIRootSource.m\n M Library/Library.xcodeproj/project.pbxproj\n[...]\n M Library/SceneKit/LISceneKitBookshelfScene.m\nAM Library/SceneKit/LISceneKitDisplayedMediumProxy.h\nAM Library/SceneKit/LISceneKitDisplayedMediumProxy.m\n\n(The last two files were added with -N.) I then attempted to stash my work, but that failed:\n\n$ git stash -k\nerror: Entry 'Library/SceneKit/LISceneKitDisplayedMediumProxy.h' not uptodate. Cannot merge.\nCannot save the current worktree state\n\nRe-checking the status showed that the intent-to-add files were now simply modified, yet they did not appear in git ls-tree -r HEAD.\n\n$ git status -sb\n## Library-3.x...origin/Library-3.x [ahead 4]\nM Library/LIRootSource.m\n M Library/Library.xcodeproj/project.pbxproj\n[...]\n M Library/SceneKit/LISceneKitBookshelfScene.m\n M Library/SceneKit/LISceneKitDisplayedMediumProxy.h\n M Library/SceneKit/LISceneKitDisplayedMediumProxy.m\n\nIt appears the index forgot that those files were new. So not only has the intent-to-add status been lost, but git status shows a file existing in neither HEAD nor the index as not-new-but-modified.\n\nTracing through it, I narrowed it down to git write-tree affecting the index state. As far as I can tell, it's incorrect for that command to change the index. I'm now out of my (shallow) depth in the git codebase, so I'm throwing it out there for someone to tackle (hopefully). I've attached a failing test case.\n\nAdditional notes for bug hunters:\n\n- I've only seen files in already-existing subdirectories lose intent-to-add status, whereas marking a file in the top-level as i-t-a is preserved during write-tree.\n\n- The 'git ls-files -s --debug' output doesn't change across the write-tree:\n\n100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tdir/baz\n ctime: 0:0\n mtime: 0:0\n dev: 0\tino: 0\n uid: 0\tgid: 0\n size: 0\tflags: 20004000\n\n\n---\n t/t2203-add-intent.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex ec35409..fcc67c0 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -62,5 +62,27 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n \tgit commit -a -m all\n '\n \n+test_expect_success 'i-t-a status unaffected by write-tree' '\n+\tgit reset --hard &&\n+\tmkdir dir &&\n+\techo frotz >dir/bar &&\n+\tgit add dir &&\n+\tgit commit -m \"establish dir\" &&\n+\techo fizfaz >foo &&\n+\techo fragz >dir/baz &&\n+\tgit add rezrov &&\n+\tgit add -N foo &&\n+\tgit add -N dir/baz &&\n+\tcat >expect <<-\\EOF &&\n+\tAM dir/baz\n+\tAM foo\n+\tEOF\n+\tgit status --untracked-files=no --porcelain >actual &&\n+\ttest_cmp actual expect &&\n+\tgit write-tree &&\n+\tgit status --untracked-files=no --porcelain >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n \n-- \n1.8.0\n\n\n\nJonathon Mah\nme@JonathonMah.com\n"},{"id":"202588","messageId":"20121106123734.GA11836@lanh","threadId":"32028","inReplyTo":"3E62F933-76CD-4578-8684-21444EAA454F@JonathonMah.com","subject":"Re: Bug: write-tree corrupts intent-to-add index state","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-11-06T12:37:34Z","receivedAt":"2012-11-06T12:37:34Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 06, 2012 at 12:53:27AM -0800, Jonathon Mah wrote:\n> It appears the index forgot that those files were new. So not only\n> has the intent-to-add status been lost, but git status shows a file\n> existing in neither HEAD nor the index as not-new-but-modified.\n>\n> Tracing through it, I narrowed it down to git write-tree affecting\n> the index state. As far as I can tell, it's incorrect for that\n> command to change the index. I'm now out of my (shallow) depth in\n> the git codebase, so I'm throwing it out there for someone to tackle\n> (hopefully). I've attached a failing test case.\n\nI played with your test a bit and found that if we skip the commit\nstep, the bug disappears. I checked and believe that i-t-a flag in\nindex is preserved, which leaves cache-tree as the culprit. If\ncache-tree is (incorrectly) valid, diff operations won't bother\nchecking index.\n\nWe used to not allow writing tree with i-t-a entries present. Then we\nallowed it, but forgot that we need to invalidate any paths that\ncontain i-t-a entries, otherwise cache-tree won't truly reflect\nindex.\n\nThe below patch seems to do the trick, but I'd rather do the\ninvalidation inside update_one() so we don't need to traverse over the\nindex again. I haven't succesfully put cache-tree.c back in my head\nagain to make it happen. Anybody is welcome to step up, or I'll finish\nit this weekend.\n\n-- 8< --\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 28ed657..30a8018 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -381,6 +381,9 @@ int cache_tree_update(struct cache_tree *it,\n \ti = update_one(it, cache, entries, \"\", 0, flags);\n \tif (i < 0)\n \t\treturn i;\n+\tfor (i = 0; i < entries; i++)\n+\t\tif (cache[i]->ce_flags & CE_INTENT_TO_ADD)\n+\t\t\tcache_tree_invalidate_path(it, cache[i]->name);\n \treturn 0;\n }\n \ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex ec35409..3ddbd89 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -62,5 +62,25 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n \tgit commit -a -m all\n '\n \n+test_expect_success 'cache-tree invalidates i-t-a paths' '\n+\tgit reset --hard &&\n+\tmkdir dir &&\n+\t: >dir/foo &&\n+\tgit add dir/foo &&\n+\tgit commit -m foo &&\n+\n+\t: >dir/bar &&\n+\tgit add -N dir/bar &&\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit write-tree >/dev/null &&\n+\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- 8< -- \n"},{"id":"202691","messageId":"1352459040-14452-1-git-send-email-pclouds@gmail.com","threadId":"32028","inReplyTo":"3E62F933-76CD-4578-8684-21444EAA454F@JonathonMah.com","subject":"[PATCH] cache-tree: invalidate i-t-a paths after writing trees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-11-09T11:04:00Z","receivedAt":"2012-11-09T11:04:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Intent-to-add entries used to forbid writing trees so it was not a\nproblem. After commit 3f6d56d (commit: ignore intent-to-add entries\ninstead of refusing - 2012-02-07), an index with i-t-a entries can\nwrite trees. However, the commit forgets to invalidate all paths\nleading to i-t-a entries. With fully valid cache-tree (e.g. after\ncommit or write-tree), diff operations may prefer cache-tree to index\nand not see i-t-a entries in the index, because cache-tree does not\nhave them.\n\nThe invalidation is done after calling update_one() in\ncache_tree_update() for simplicity because it's probably not worth the\ncomplexity of changing a recursive function and the performance\nbottleneck won't likely fall to this new loop, rather in\nwrite_sha1_file or hash_sha1_file. But this means that if update_one()\nhas new call sites, they must re-do the same what this commit does to\navoid the same fault.\n\nReported-by: Jonathon Mah \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache-tree.c | 3 +++\n t/t2203-add-intent.sh | 20 ++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 28ed657..30a8018 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -381,6 +381,9 @@ int cache_tree_update(struct cache_tree *it,\n \ti = update_one(it, cache, entries, \"\", 0, flags);\n \tif (i < 0)\n \t\treturn i;\n+\tfor (i = 0; i < entries; i++)\n+\t\tif (cache[i]->ce_flags & CE_INTENT_TO_ADD)\n+\t\t\tcache_tree_invalidate_path(it, cache[i]->name);\n \treturn 0;\n }\n \ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex ec35409..2a4a749 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -62,5 +62,25 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n \tgit commit -a -m all\n '\n \n+test_expect_success 'cache-tree invalidates i-t-a paths' '\n+\tgit reset --hard &&\n+\tmkdir dir &&\n+\t: >dir/foo &&\n+\tgit add dir/foo &&\n+\tgit commit -m foo &&\n+\n+\t: >dir/bar &&\n+\tgit add -N dir/bar &&\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit write-tree >/dev/null &&\n+\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n \n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"202692","messageId":"7vy5ibouo4.fsf@alter.siamese.dyndns.org","threadId":"32028","inReplyTo":"1352459040-14452-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] cache-tree: invalidate i-t-a paths after writing trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-09T11:57:47Z","receivedAt":"2012-11-09T11:57:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 28ed657..30a8018 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -381,6 +381,9 @@ int cache_tree_update(struct cache_tree *it,\n> \ti = update_one(it, cache, entries, \"\", 0, flags);\n> \tif (i < 0)\n> \t\treturn i;\n> +\tfor (i = 0; i < entries; i++)\n> +\t\tif (cache[i]->ce_flags & CE_INTENT_TO_ADD)\n> +\t\t\tcache_tree_invalidate_path(it, cache[i]->name);\n> \treturn 0;\n> }\n\nI notice there is another special case for CE_REMOVE but there is\nnothing that adjusts the cache-tree for these entries in the current\ncodebase.\n\nI suspect the original code before we (perhaps incorrectly) updated\nthe code not to error out upon I-T-A entries was fine only because\nwe do not attempt to fully populate the cache-tree during a merge in\nthe unpack-trees codepath, which will mark the index entries that\nare to be removed with CE_REMOVE in the resulting index.\n\nThe solution implemented with this patch will break if we start\nupdating the cache tree after a successful merge in unpack-trees, I\nsuspect.\n\nAn alternative might be to add a \"phoney\" bit next to \"used\" in the\ncache_tree structure, mark the cache tree as phoney when we skip an\nentry marked as CE_REMOVE or CE_ITA, and make the postprocessing\nloop this patch adds aware of that bit, instead of iterating over\nthe index entries; instead, it would recurse the resulting cache\ntree and invalidate parts of the tree that have subtrees with the\n\"phoney\" bit set, or something.\n"},{"id":"202747","messageId":"CACsJy8DEwpg0gY1o6gSB747W5fAYYxz97e-qnkQthSut3B7Eag@mail.gmail.com","threadId":"32028","inReplyTo":"7vy5ibouo4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] cache-tree: invalidate i-t-a paths after writing trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-11-10T11:04:32Z","receivedAt":"2012-11-10T11:04:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 9, 2012 at 6:57 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> diff --git a/cache-tree.c b/cache-tree.c\n>> index 28ed657..30a8018 100644\n>> --- a/cache-tree.c\n>> +++ b/cache-tree.c\n>> @@ -381,6 +381,9 @@ int cache_tree_update(struct cache_tree *it,\n>> i = update_one(it, cache, entries, \"\", 0, flags);\n>> if (i < 0)\n>> return i;\n>> + for (i = 0; i < entries; i++)\n>> + if (cache[i]->ce_flags & CE_INTENT_TO_ADD)\n>> + cache_tree_invalidate_path(it, cache[i]->name);\n>> return 0;\n>> }\n>\n> I notice there is another special case for CE_REMOVE but there is\n> nothing that adjusts the cache-tree for these entries in the current\n> codebase.\n>\n> I suspect the original code before we (perhaps incorrectly) updated\n> the code not to error out upon I-T-A entries was fine only because\n> we do not attempt to fully populate the cache-tree during a merge in\n> the unpack-trees codepath, which will mark the index entries that\n> are to be removed with CE_REMOVE in the resulting index.\n>\n> The solution implemented with this patch will break if we start\n> updating the cache tree after a successful merge in unpack-trees, I\n> suspect.\n\nI don't understand. I thought we handled CE_REMOVE correctly (i.e. no\nCE_REMOVE entries in cache tree even after a successful merge). Or\nshould we keep CE_REMOVE in cache tree after a successful merge?\n\n> An alternative might be to add a \"phoney\" bit next to \"used\" in the\n> cache_tree structure, mark the cache tree as phoney when we skip an\n> entry marked as CE_REMOVE or CE_ITA, and make the postprocessing\n> loop this patch adds aware of that bit, instead of iterating over\n> the index entries; instead, it would recurse the resulting cache\n> tree and invalidate parts of the tree that have subtrees with the\n> \"phoney\" bit set, or something.\n\nYeah, that sounds better.\n-- \nDuy\n"},{"id":"204303","messageId":"7vhao8neck.fsf@alter.siamese.dyndns.org","threadId":"32028","inReplyTo":"CACsJy8DEwpg0gY1o6gSB747W5fAYYxz97e-qnkQthSut3B7Eag@mail.gmail.com","subject":"Re: [PATCH] cache-tree: invalidate i-t-a paths after writing trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-30T00:06:51Z","receivedAt":"2012-11-30T00:06:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n>> An alternative might be to add a \"phoney\" bit next to \"used\" in the\n>> cache_tree structure, mark the cache tree as phoney when we skip an\n>> entry marked as CE_REMOVE or CE_ITA, and make the postprocessing\n>> loop this patch adds aware of that bit, instead of iterating over\n>> the index entries; instead, it would recurse the resulting cache\n>> tree and invalidate parts of the tree that have subtrees with the\n>> \"phoney\" bit set, or something.\n>\n> Yeah, that sounds better.\n\nDid anything happen to this topic after this?\n"},{"id":"204306","messageId":"CACsJy8BQO4G0YXf87jS+sEpd6+RW+UVULDEt80yzTiiisWS2-A@mail.gmail.com","threadId":"32028","inReplyTo":"7vhao8neck.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] cache-tree: invalidate i-t-a paths after writing trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-11-30T01:26:30Z","receivedAt":"2012-11-30T01:26:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 30, 2012 at 7:06 AM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n>>> An alternative might be to add a \"phoney\" bit next to \"used\" in the\n>>> cache_tree structure, mark the cache tree as phoney when we skip an\n>>> entry marked as CE_REMOVE or CE_ITA, and make the postprocessing\n>>> loop this patch adds aware of that bit, instead of iterating over\n>>> the index entries; instead, it would recurse the resulting cache\n>>> tree and invalidate parts of the tree that have subtrees with the\n>>> \"phoney\" bit set, or something.\n>>\n>> Yeah, that sounds better.\n>\n> Did anything happen to this topic after this?\n\nNot from my side because I forgot to mark this thread as a todo item\nand unsurprisingly forgot about it.\n-- \nDuy\n"},{"id":"204602","messageId":"1354939803-8466-1-git-send-email-pclouds@gmail.com","threadId":"32028","inReplyTo":"1352459040-14452-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] cache-tree: invalidate i-t-a paths after generating trees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-08T04:10:03Z","receivedAt":"2012-12-08T04:10:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Intent-to-add entries used to forbid writing trees so it was not a\nproblem. After commit 3f6d56d (commit: ignore intent-to-add entries\ninstead of refusing - 2012-02-07), we can generate trees from an index\nwith i-t-a entries.\n\nHowever, the commit forgets to invalidate all paths leading to i-t-a\nentries. With fully valid cache-tree (e.g. after commit or\nwrite-tree), diff operations may prefer cache-tree to index and not\nsee i-t-a entries in the index, because cache-tree does not have them.\n\nReported-by: Jonathon Mah \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Sorry for the late update, I have little time for git these days.\n It turns out that we don't need phony bit and another round for\n invalidation. We can do it in update_one.\n\n cache-tree.c | 30 ++++++++++++++++++++++++++----\n t/t2203-add-intent.sh | 20 ++++++++++++++++++++\n 2 files changed, 46 insertions(+), 4 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 28ed657..989a7ff 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,\n \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n \tint i;\n+\tint to_invalidate = 0;\n \n \tif (0 <= it->entry_count && has_sha1_file(it->sha1))\n \t\treturn it->entry_count;\n@@ -324,7 +325,13 @@ static int update_one(struct cache_tree *it,\n \t\t\tif (!sub)\n \t\t\t\tdie(\"cache-tree.c: '%.*s' in '%s' not found\",\n \t\t\t\t entlen, path + baselen, path);\n-\t\t\ti += sub->cache_tree->entry_count - 1;\n+\t\t\ti--; /* this entry is already counted in \"sub\" */\n+\t\t\tif (sub->cache_tree->entry_count < 0) {\n+\t\t\t\ti -= sub->cache_tree->entry_count;\n+\t\t\t\tto_invalidate = 1;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\ti += sub->cache_tree->entry_count;\n \t\t\tsha1 = sub->cache_tree->sha1;\n \t\t\tmode = S_IFDIR;\n \t\t}\n@@ -339,8 +346,23 @@ static int update_one(struct cache_tree *it,\n \t\t\t\tmode, sha1_to_hex(sha1), entlen+baselen, path);\n \t\t}\n \n-\t\tif (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))\n-\t\t\tcontinue; /* entry being removed or placeholder */\n+\t\t/*\n+\t\t * CE_REMOVE entries are removed before the index is\n+\t\t * written to disk. Skip them to remain consistent\n+\t\t * with the future on-disk index.\n+\t\t */\n+\t\tif (ce->ce_flags & CE_REMOVE)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * CE_INTENT_TO_ADD entries exist on on-disk index but\n+\t\t * they are not part of generated trees. Invalidate up\n+\t\t * to root to force cache-tree users to read elsewhere.\n+\t\t */\n+\t\tif (ce->ce_flags & CE_INTENT_TO_ADD) {\n+\t\t\tto_invalidate = 1;\n+\t\t\tcontinue;\n+\t\t}\n \n \t\tstrbuf_grow(&buffer, entlen + 100);\n \t\tstrbuf_addf(&buffer, \"%o %.*s%c\", mode, entlen, path + baselen, '\\0');\n@@ -360,7 +382,7 @@ static int update_one(struct cache_tree *it,\n \t}\n \n \tstrbuf_release(&buffer);\n-\tit->entry_count = i;\n+\tit->entry_count = to_invalidate ? -i : i;\n #if DEBUG\n \tfprintf(stderr, \"cache-tree update-one (%d ent, %d subtree) %s\\n\",\n \t\tit->entry_count, it->subtree_nr,\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex ec35409..2a4a749 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -62,5 +62,25 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n \tgit commit -a -m all\n '\n \n+test_expect_success 'cache-tree invalidates i-t-a paths' '\n+\tgit reset --hard &&\n+\tmkdir dir &&\n+\t: >dir/foo &&\n+\tgit add dir/foo &&\n+\tgit commit -m foo &&\n+\n+\t: >dir/bar &&\n+\tgit add -N dir/bar &&\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit write-tree >/dev/null &&\n+\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n \n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"204642","messageId":"7v7goqcsdy.fsf@alter.siamese.dyndns.org","threadId":"32028","inReplyTo":"1354939803-8466-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] cache-tree: invalidate i-t-a paths after generating trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-10T06:50:49Z","receivedAt":"2012-12-10T06:50:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 28ed657..989a7ff 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,\n> \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n> \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n> \tint i;\n> +\tint to_invalidate = 0;\n> \n> \tif (0 <= it->entry_count && has_sha1_file(it->sha1))\n> \t\treturn it->entry_count;\n> @@ -324,7 +325,13 @@ static int update_one(struct cache_tree *it,\n> \t\t\tif (!sub)\n> \t\t\t\tdie(\"cache-tree.c: '%.*s' in '%s' not found\",\n> \t\t\t\t entlen, path + baselen, path);\n> -\t\t\ti += sub->cache_tree->entry_count - 1;\n> +\t\t\ti--; /* this entry is already counted in \"sub\" */\n> +\t\t\tif (sub->cache_tree->entry_count < 0) {\n> +\t\t\t\ti -= sub->cache_tree->entry_count;\n> +\t\t\t\tto_invalidate = 1;\n> +\t\t\t}\n> +\t\t\telse\n> +\t\t\t\ti += sub->cache_tree->entry_count;\n\nHrm. update_one() is prepared to see a cache-tree whose entry count\nis zero (see the context lines in the previous hunk) and the\ninvariant for the rest of the code is \"if 0 <= entry_count, the\ncached tree is valid; invalid cache-tree has -1 in entry_count.\nMore importantly, entry_count negated does not in general express\nhow many entries there are in the subtree and does not tell us how\nmany index entries to skip.\n\n> @@ -339,8 +346,23 @@ static int update_one(struct cache_tree *it,\n> \t\t\t\tmode, sha1_to_hex(sha1), entlen+baselen, path);\n> \t\t}\n> \n> -\t\tif (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))\n> -\t\t\tcontinue; /* entry being removed or placeholder */\n> +\t\t/*\n> +\t\t * CE_REMOVE entries are removed before the index is\n> +\t\t * written to disk. Skip them to remain consistent\n> +\t\t * with the future on-disk index.\n> +\t\t */\n> +\t\tif (ce->ce_flags & CE_REMOVE)\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * CE_INTENT_TO_ADD entries exist on on-disk index but\n> +\t\t * they are not part of generated trees. Invalidate up\n> +\t\t * to root to force cache-tree users to read elsewhere.\n> +\t\t */\n> +\t\tif (ce->ce_flags & CE_INTENT_TO_ADD) {\n> +\t\t\tto_invalidate = 1;\n> +\t\t\tcontinue;\n> +\t\t}\n\nThanks for documenting these.\n\n> @@ -360,7 +382,7 @@ static int update_one(struct cache_tree *it,\n> \t}\n> \n> \tstrbuf_release(&buffer);\n> -\tit->entry_count = i;\n> +\tit->entry_count = to_invalidate ? -i : i;\n\nSee above. I am not fundamentally opposed to a change to redefine\nentry_count so that it always maintains how many index entries the\nsubtree covers, even for invalidated subtree, but I do not think\nthis patch alone is sufficient to maintain such invariant.\n\n> #if DEBUG\n> \tfprintf(stderr, \"cache-tree update-one (%d ent, %d subtree) %s\\n\",\n> \t\tit->entry_count, it->subtree_nr,\n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index ec35409..2a4a749 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -62,5 +62,25 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n> \tgit commit -a -m all\n> '\n> \n> +test_expect_success 'cache-tree invalidates i-t-a paths' '\n> +\tgit reset --hard &&\n> +\tmkdir dir &&\n> +\t: >dir/foo &&\n> +\tgit add dir/foo &&\n> +\tgit commit -m foo &&\n> +\n> +\t: >dir/bar &&\n> +\tgit add -N dir/bar &&\n> +\tgit diff --cached --name-only >actual &&\n> +\techo dir/bar >expect &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tgit write-tree >/dev/null &&\n> +\n> +\tgit diff --cached --name-only >actual &&\n> +\techo dir/bar >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"204645","messageId":"CACsJy8CW1UGgQcgHa11XP71XZGaTxytrGmJmrtdNvy=N7cUyMw@mail.gmail.com","threadId":"32028","inReplyTo":"7v7goqcsdy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] cache-tree: invalidate i-t-a paths after generating trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-10T11:53:02Z","receivedAt":"2012-12-10T11:53:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 10, 2012 at 1:50 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> diff --git a/cache-tree.c b/cache-tree.c\n>> index 28ed657..989a7ff 100644\n>> --- a/cache-tree.c\n>> +++ b/cache-tree.c\n>> @@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,\n>> int missing_ok = flags & WRITE_TREE_MISSING_OK;\n>> int dryrun = flags & WRITE_TREE_DRY_RUN;\n>> int i;\n>> + int to_invalidate = 0;\n>>\n>> if (0 <= it->entry_count && has_sha1_file(it->sha1))\n>> return it->entry_count;\n>> @@ -324,7 +325,13 @@ static int update_one(struct cache_tree *it,\n>> if (!sub)\n>> die(\"cache-tree.c: '%.*s' in '%s' not found\",\n>> entlen, path + baselen, path);\n>> - i += sub->cache_tree->entry_count - 1;\n>> + i--; /* this entry is already counted in \"sub\" */\n>> + if (sub->cache_tree->entry_count < 0) {\n>> + i -= sub->cache_tree->entry_count;\n>> + to_invalidate = 1;\n>> + }\n>> + else\n>> + i += sub->cache_tree->entry_count;\n>\n> Hrm. update_one() is prepared to see a cache-tree whose entry count\n> is zero (see the context lines in the previous hunk) and the\n> invariant for the rest of the code is \"if 0 <= entry_count, the\n> cached tree is valid; invalid cache-tree has -1 in entry_count.\n> More importantly, entry_count negated does not in general express\n> how many entries there are in the subtree and does not tell us how\n> many index entries to skip.\n\nYeah I use entry_count for two different things here. In the previous\n(unsent) version of the patch I had \"entry_count = -1\" right after \"i\n-= entry_count\"\n\n>> + if (sub->cache_tree->entry_count < 0) {\n>> + i -= sub->cache_tree->entry_count;\n>> + sub->cache_tree->entry_count = -1;\n>> + to_invalidate = 1;\n>> + }\n\n\nwhich makes it clearer that the use of negative entry count is only\nvalid within update_one. Then I changed my mind because it says\n'negative means \"invalid\"' in cache-tree.h. So, put \"entry_count = -1\"\nback or just add another field to struct cache_tree for this?\n-- \nDuy\n"},{"id":"204653","messageId":"7vip89bz4v.fsf@alter.siamese.dyndns.org","threadId":"32028","inReplyTo":"CACsJy8CW1UGgQcgHa11XP71XZGaTxytrGmJmrtdNvy=N7cUyMw@mail.gmail.com","subject":"Re: [PATCH v2] cache-tree: invalidate i-t-a paths after generating trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-10T17:22:40Z","receivedAt":"2012-12-10T17:22:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> Yeah I use entry_count for two different things here. In the previous\n> (unsent) version of the patch I had \"entry_count = -1\" right after \"i\n> -= entry_count\"\n>\n>>> + if (sub->cache_tree->entry_count < 0) {\n>>> + i -= sub->cache_tree->entry_count;\n>>> + sub->cache_tree->entry_count = -1;\n>>> + to_invalidate = 1;\n>>> + }\n>\n>\n> which makes it clearer that the use of negative entry count is only\n> valid within update_one. Then I changed my mind because it says\n> 'negative means \"invalid\"' in cache-tree.h.\n\nBut you make it to mean not just 'negative means invalid' but\n'negate it and you can learn how many index entries to skip to reach\nthe entry outside the subtree'. That is what I was wondering about.\n\nI do not think that new invariant does not hold in general; it may\nhold true while inside this function, but immediately after somebody\nelse calls cache_tree_invalidate_path() it won't be true anymore.\nLeaking the information to outside callers that can easily be\nmistaken as if it may mean something without any comment looks like\nwe are asking for trouble.\n\n> So, put \"entry_count = -1\"\n> back or just add another field to struct cache_tree for this?\n\nAs long as it is made clear with in-code comment that says \"negative\nentry count, when negated, will give us how many index entries are\ncovered by this invalid cache tree, only inside this function\", and\nsuch a negative entry_count is reset to -1 to avoid leaking out the\nvalue that will soon go stale to the outside world in the \"write\nthis tree out\" loop, I think the v2 patch is OK, if not ideal.\n\nIf we were to commit to keep the new invariant true throughout the\nsystem, on the other hand, I suspect that a boolean flag \"valid\" may\nhelp people who keep i-t-a entries around across write-tree calls.\nThe first if () statement in the update_one() function can check the\nvalidity, and you can say the cache tree is valid even if the\nsection in the index that is covered by the cache-tree contains\ni-t-a entries _after_ you wrote it out as a tree, no? That may make\nthe semantics a lot cleaner, I suspect.\n\nThe new semantics might be:\n\n * update_one() returns negative only when the section of the index\n given to it cannot be written out as a tree (i.e. not merged, or\n corrupt index);\n\n * update_one() returns the number of entries in the index covered\n by the tree, including i-t-a entries (but not REMOVED, as these\n entries will not be in the index after the cache-tree has been\n written out);\n\n * cache_tree->valid tells if the cache_tree->sha1 is valid; the\n section of the index the tree covers may or may not have i-t-a\n entries in it;\n\n * cache_tree->entry_count holds the number of index entries the\n tree covers, couting i-t-a entries. This field is only meaningful\n when cache_tree->valid is true.\n\nor something like that.\n\nI noticed that the recent \"ignore i-t-a only while writing trees\ninstead of erroring out\" broke 331fcb5 (git add --intent-to-add: do\nnot let an empty blob be committed by accident, 2008-11-28) in\nanother way, by the way. In verify_cache(), there is an unreachable\nelse clause to warn about \"not added yet\" entries that should have\nbeen removed but is left behind.\n"},{"id":"204816","messageId":"1355362747-13474-1-git-send-email-pclouds@gmail.com","threadId":"32028","inReplyTo":"7vip89bz4v.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-13T01:39:06Z","receivedAt":"2012-12-13T01:39:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Intent-to-add entries used to forbid writing trees so it was not a\nproblem. After commit 3f6d56d (commit: ignore intent-to-add entries\ninstead of refusing - 2012-02-07), we can generate trees from an index\nwith i-t-a entries.\n\nHowever, the commit forgets to invalidate all paths leading to i-t-a\nentries. With fully valid cache-tree (e.g. after commit or\nwrite-tree), diff operations may prefer cache-tree to index and not\nsee i-t-a entries in the index, because cache-tree does not have them.\n\nReported-by: Jonathon Mah \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n This version ensures that entry_count can only be >= -1 after\n update_one returns. Not ideal but good enough.\n\n cache-tree.c | 40 ++++++++++++++++++++++++++++++++++++----\n t/t2203-add-intent.sh | 20 ++++++++++++++++++++\n 2 files changed, 56 insertions(+), 4 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 28ed657..1fbc81a 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,\n \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n \tint i;\n+\tint to_invalidate = 0;\n \n \tif (0 <= it->entry_count && has_sha1_file(it->sha1))\n \t\treturn it->entry_count;\n@@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,\n \t\t\tif (!sub)\n \t\t\t\tdie(\"cache-tree.c: '%.*s' in '%s' not found\",\n \t\t\t\t entlen, path + baselen, path);\n-\t\t\ti += sub->cache_tree->entry_count - 1;\n+\t\t\ti--; /* this entry is already counted in \"sub\" */\n+\t\t\tif (sub->cache_tree->entry_count < 0) {\n+\t\t\t\ti -= sub->cache_tree->entry_count;\n+\t\t\t\tsub->cache_tree->entry_count = -1;\n+\t\t\t\tto_invalidate = 1;\n+\t\t\t}\n+\t\t\telse\n+\t\t\t\ti += sub->cache_tree->entry_count;\n \t\t\tsha1 = sub->cache_tree->sha1;\n \t\t\tmode = S_IFDIR;\n \t\t}\n@@ -339,8 +347,23 @@ static int update_one(struct cache_tree *it,\n \t\t\t\tmode, sha1_to_hex(sha1), entlen+baselen, path);\n \t\t}\n \n-\t\tif (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))\n-\t\t\tcontinue; /* entry being removed or placeholder */\n+\t\t/*\n+\t\t * CE_REMOVE entries are removed before the index is\n+\t\t * written to disk. Skip them to remain consistent\n+\t\t * with the future on-disk index.\n+\t\t */\n+\t\tif (ce->ce_flags & CE_REMOVE)\n+\t\t\tcontinue;\n+\n+\t\t/*\n+\t\t * CE_INTENT_TO_ADD entries exist on on-disk index but\n+\t\t * they are not part of generated trees. Invalidate up\n+\t\t * to root to force cache-tree users to read elsewhere.\n+\t\t */\n+\t\tif (ce->ce_flags & CE_INTENT_TO_ADD) {\n+\t\t\tto_invalidate = 1;\n+\t\t\tcontinue;\n+\t\t}\n \n \t\tstrbuf_grow(&buffer, entlen + 100);\n \t\tstrbuf_addf(&buffer, \"%o %.*s%c\", mode, entlen, path + baselen, '\\0');\n@@ -360,7 +383,7 @@ static int update_one(struct cache_tree *it,\n \t}\n \n \tstrbuf_release(&buffer);\n-\tit->entry_count = i;\n+\tit->entry_count = to_invalidate ? -i : i;\n #if DEBUG\n \tfprintf(stderr, \"cache-tree update-one (%d ent, %d subtree) %s\\n\",\n \t\tit->entry_count, it->subtree_nr,\n@@ -381,6 +404,15 @@ int cache_tree_update(struct cache_tree *it,\n \ti = update_one(it, cache, entries, \"\", 0, flags);\n \tif (i < 0)\n \t\treturn i;\n+\t/*\n+\t * update_one() uses negative entry_count as a way to mark an\n+\t * entry invalid _and_ pass the number of entries back to\n+\t * itself at the parent level. This is for internal use and\n+\t * should not be leaked out after the top-level update_one\n+\t * exits.\n+\t */\n+\tif (it->entry_count < 0)\n+\t\tit->entry_count = -1;\n \treturn 0;\n }\n \ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex ec35409..2a4a749 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -62,5 +62,25 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n \tgit commit -a -m all\n '\n \n+test_expect_success 'cache-tree invalidates i-t-a paths' '\n+\tgit reset --hard &&\n+\tmkdir dir &&\n+\t: >dir/foo &&\n+\tgit add dir/foo &&\n+\tgit commit -m foo &&\n+\n+\t: >dir/bar &&\n+\tgit add -N dir/bar &&\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit write-tree >/dev/null &&\n+\n+\tgit diff --cached --name-only >actual &&\n+\techo dir/bar >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n \n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"204817","messageId":"1355362747-13474-2-git-send-email-pclouds@gmail.com","threadId":"32028","inReplyTo":"1355362747-13474-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 2/2] cache-tree: remove dead i-t-a code in verify_cache()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-13T01:39:07Z","receivedAt":"2012-12-13T01:39:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This code is added in 331fcb5 (git add --intent-to-add: do not let an\nempty blob be committed by accident - 2008-11-28) to forbid committing\nwhen i-t-a entries are present. When we allow that, we forgot to\nremove this unreachable code.\n\nNoticed-by: Junio C Hamano \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache-tree.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 1fbc81a..3e79e27 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -166,12 +166,8 @@ static int verify_cache(struct cache_entry **cache,\n \t\t\t\tfprintf(stderr, \"...\\n\");\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (ce_stage(ce))\n-\t\t\t\tfprintf(stderr, \"%s: unmerged (%s)\\n\",\n-\t\t\t\t\tce->name, sha1_to_hex(ce->sha1));\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"%s: not added yet\\n\",\n-\t\t\t\t\tce->name);\n+\t\t\tfprintf(stderr, \"%s: unmerged (%s)\\n\",\n+\t\t\t\tce->name, sha1_to_hex(ce->sha1));\n \t\t}\n \t}\n \tif (funny)\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"204820","messageId":"7v4njq1ze7.fsf@alter.siamese.dyndns.org","threadId":"32028","inReplyTo":"1355362747-13474-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-13T02:04:00Z","receivedAt":"2012-12-13T02:04:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Intent-to-add entries used to forbid writing trees so it was not a\n> problem. After commit 3f6d56d (commit: ignore intent-to-add entries\n> instead of refusing - 2012-02-07), we can generate trees from an index\n> with i-t-a entries.\n>\n> However, the commit forgets to invalidate all paths leading to i-t-a\n> entries. With fully valid cache-tree (e.g. after commit or\n> write-tree), diff operations may prefer cache-tree to index and not\n> see i-t-a entries in the index, because cache-tree does not have them.\n>\n> Reported-by: Jonathon Mah \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> This version ensures that entry_count can only be >= -1 after\n> update_one returns. Not ideal but good enough.\n>\n> cache-tree.c | 40 ++++++++++++++++++++++++++++++++++++----\n> t/t2203-add-intent.sh | 20 ++++++++++++++++++++\n> 2 files changed, 56 insertions(+), 4 deletions(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 28ed657..1fbc81a 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,\n> \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n> \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n> \tint i;\n> +\tint to_invalidate = 0;\n> \n> \tif (0 <= it->entry_count && has_sha1_file(it->sha1))\n> \t\treturn it->entry_count;\n> @@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,\n> \t\t\tif (!sub)\n> \t\t\t\tdie(\"cache-tree.c: '%.*s' in '%s' not found\",\n> \t\t\t\t entlen, path + baselen, path);\n> -\t\t\ti += sub->cache_tree->entry_count - 1;\n> +\t\t\ti--; /* this entry is already counted in \"sub\" */\n\nHuh?\n\nThe \"-1\" in the original is the bog-standard compensation for the\nfor(;;i++) loop.\n\n> +\t\t\tif (sub->cache_tree->entry_count < 0) {\n> +\t\t\t\ti -= sub->cache_tree->entry_count;\n> +\t\t\t\tsub->cache_tree->entry_count = -1;\n> +\t\t\t\tto_invalidate = 1;\n> +\t\t\t}\n> +\t\t\telse\n> +\t\t\t\ti += sub->cache_tree->entry_count;\n\nWhile the rewritten version is not *wrong* per-se, I have a feeling\nthat it may be much easier to read if written like this:\n\n\tif (sub->cache_tree_entry_count < 0) {\n\t\tto_invalidate = 1;\n to_skip = 0 - sub->cache_tree_entry_count;\n\t\tsub->cache_tree_entry_count = -1;\n\t} else {\n\t\tto_skip = sub->cache_tree_entry_count;\n\t}\n i += to_skip - 1;\n\n> @@ -360,7 +383,7 @@ static int update_one(struct cache_tree *it,\n> \t}\n> \n> \tstrbuf_release(&buffer);\n> -\tit->entry_count = i;\n> +\tit->entry_count = to_invalidate ? -i : i;\n> #if DEBUG\n> \tfprintf(stderr, \"cache-tree update-one (%d ent, %d subtree) %s\\n\",\n> \t\tit->entry_count, it->subtree_nr,\n> @@ -381,6 +404,15 @@ int cache_tree_update(struct cache_tree *it,\n> \ti = update_one(it, cache, entries, \"\", 0, flags);\n> \tif (i < 0)\n> \t\treturn i;\n> +\t/*\n> +\t * update_one() uses negative entry_count as a way to mark an\n> +\t * entry invalid _and_ pass the number of entries back to\n> +\t * itself at the parent level. This is for internal use and\n> +\t * should not be leaked out after the top-level update_one\n> +\t * exits.\n> +\t */\n> +\tif (it->entry_count < 0)\n> +\t\tit->entry_count = -1;\n\nNice. I think what entry_count means immediately after update_one()\nreturned should be commented near the beginning of that function,\ntoo, though.\n\nThanks.\n"},{"id":"204843","messageId":"7vbodxztqu.fsf@alter.siamese.dyndns.org","threadId":"32028","inReplyTo":"1355362747-13474-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 2/2] cache-tree: remove dead i-t-a code in verify_cache()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-13T18:34:17Z","receivedAt":"2012-12-13T18:34:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Will replace the one in 'pu' with these two. Thanks.\n"},{"id":"204891","messageId":"CACsJy8BcLmGmxBUq3+4_Go4v1-HOFDOj9wi8A_-fbk-on4MX3A@mail.gmail.com","threadId":"32028","inReplyTo":"7v4njq1ze7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-15T02:52:52Z","receivedAt":"2012-12-15T02:52:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 13, 2012 at 9:04 AM, Junio C Hamano wrote:\n>> @@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,\n>> if (!sub)\n>> die(\"cache-tree.c: '%.*s' in '%s' not found\",\n>> entlen, path + baselen, path);\n>> - i += sub->cache_tree->entry_count - 1;\n>> + i--; /* this entry is already counted in \"sub\" */\n>\n> Huh?\n>\n> The \"-1\" in the original is the bog-standard compensation for the\n> for(;;i++) loop.\n\nExactly. It took me a while to figure out what \" - 1\" was for and I\nwanted to avoid that for other developers. Only I worded it badly.\nI'll replace the for loop with a while loop to make it clearer...\n\n>\n>> + if (sub->cache_tree->entry_count < 0) {\n>> + i -= sub->cache_tree->entry_count;\n>> + sub->cache_tree->entry_count = -1;\n>> + to_invalidate = 1;\n>> + }\n>> + else\n>> + i += sub->cache_tree->entry_count;\n>\n> While the rewritten version is not *wrong* per-se, I have a feeling\n> that it may be much easier to read if written like this:\n>\n> if (sub->cache_tree_entry_count < 0) {\n> to_invalidate = 1;\n> to_skip = 0 - sub->cache_tree_entry_count;\n> sub->cache_tree_entry_count = -1;\n> } else {\n> to_skip = sub->cache_tree_entry_count;\n> }\n> i += to_skip - 1;\n>\n\n..or this would be fine too. Which way to go?\n\nA while we're still at the cache tree\n\n> - if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))\n> - continue; /* entry being removed or placeholder */\n> + /*\n> + * CE_REMOVE entries are removed before the index is\n> + * written to disk. Skip them to remain consistent\n> + * with the future on-disk index.\n> + */\n> + if (ce->ce_flags & CE_REMOVE)\n> + continue;\n> +\n\nA CE_REMOVE entry is removed later from the index, however it is still\ncounted in entry_count. entry_count serves two purposes: to skip the\nnumber of processed entries in the in-memory index, and to record the\nnumber of entries in the on-disk index. These numbers do not match\nwhen CE_REMOVE is present. We have correct in-memory entry_count,\nwhich means incorrect on-disk entry_count in this case.\n\nI tested with an index that has a/b and a/c. The latter has CE_REMOVE.\nAfter writing cache tree I get:\n\n$ git ls-files --stage\n100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a/b\n$ ../test-dump-cache-tree\n878e27c626266ac04087a203e4bdd396dcf74763 (2 entries, 1 subtrees)\n878e27c626266ac04087a203e4bdd396dcf74763 #(ref) (1 entries, 1 subtrees)\n4277b6e69d25e5efa77c455340557b384a4c018a a/ (2 entries, 0 subtrees)\n4277b6e69d25e5efa77c455340557b384a4c018a #(ref) a/ (1 entries, 0 subtrees)\n\nIf I throw out that index, create a new one with a/b alone and write-tree, I get\n\n$ ../test-dump-cache-tree\n878e27c626266ac04087a203e4bdd396dcf74763 (1 entries, 1 subtrees)\n4277b6e69d25e5efa77c455340557b384a4c018a a/ (1 entries, 0 subtrees)\n\nShall we fix this too? I'm thinking of adding \"skip\" argument to update_one and\n\n i += sub->cache_tree->entry_count - 1;\n\nwould become\n\n i += sub->cache_tree->entry_count + skip - 1;\n\nand entry_count would always reflect on-disk value. This \"skip\" can be\nreused for this i-t-a patch as well.\n-- \nDuy\n"}]}