{"thread":{"id":"32355","subject":"[PATCH v4 1/4] cache-tree: remove dead i-t-a code in verify_cache()","startedAt":"2012-12-16T04:15:24Z","lastAt":"2012-12-16T10:38:40Z","messageCount":8,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"204952","messageId":"1355631328-26678-1-git-send-email-pclouds@gmail.com","threadId":"32355","inReplyTo":null,"subject":"[PATCH v4 0/4] nd/invalidate-i-t-a-cache-tree","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-16T04:15:24Z","receivedAt":"2012-12-16T04:15:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This version also fixes the CE_REMOVE bug I mentioned. As a side\neffect, the bug fix makes the i-t-a fix cleaner.\n\nNguyễn Thái Ngọc Duy (4):\n cache-tree: remove dead i-t-a code in verify_cache()\n cache-tree: replace \"for\" loops in update_one with \"while\" loops\n cache-tree: fix writing cache-tree when CE_REMOVE is present\n cache-tree: invalidate i-t-a paths after generating trees\n\n cache-tree.c | 61 +++++++++++++++++++++++++++++++++++++--------------\n cache-tree.h | 1 +\n t/t2203-add-intent.sh | 20 +++++++++++++++++\n 3 files changed, 65 insertions(+), 17 deletions(-)\n\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"204950","messageId":"1355631328-26678-2-git-send-email-pclouds@gmail.com","threadId":"32355","inReplyTo":"1355631328-26678-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 1/4] 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-16T04:15:25Z","receivedAt":"2012-12-16T04:15:25Z","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.\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 28ed657..e2beab5 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":"204951","messageId":"1355631328-26678-3-git-send-email-pclouds@gmail.com","threadId":"32355","inReplyTo":"1355631328-26678-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 2/4] cache-tree: replace \"for\" loops in update_one with \"while\" loops","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-16T04:15:26Z","receivedAt":"2012-12-16T04:15:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The loops in update_one can be increased in two different ways: step\nby one for files and by for directories. \"for\" loop is not\nsuitable for this as it always steps by one and special handling is\nrequired for directories. Replace them with \"while\" loops for clarity.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache-tree.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex e2beab5..44eed28 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -259,7 +259,8 @@ static int update_one(struct cache_tree *it,\n \t/*\n \t * Find the subtrees and update them.\n \t */\n-\tfor (i = 0; i < entries; i++) {\n+\ti = 0;\n+\twhile (i < entries) {\n \t\tstruct cache_entry *ce = cache[i];\n \t\tstruct cache_tree_sub *sub;\n \t\tconst char *path, *slash;\n@@ -271,8 +272,10 @@ static int update_one(struct cache_tree *it,\n \t\t\tbreak; /* at the end of this level */\n \n \t\tslash = strchr(path + baselen, '/');\n-\t\tif (!slash)\n+\t\tif (!slash) {\n+\t\t\ti++;\n \t\t\tcontinue;\n+\t\t}\n \t\t/*\n \t\t * a/bbb/c (base = a/, slash = /c)\n \t\t * ==>\n@@ -289,7 +292,7 @@ static int update_one(struct cache_tree *it,\n \t\t\t\t flags);\n \t\tif (subcnt < 0)\n \t\t\treturn subcnt;\n-\t\ti += subcnt - 1;\n+\t\ti += subcnt;\n \t\tsub->used = 1;\n \t}\n \n@@ -300,7 +303,8 @@ static int update_one(struct cache_tree *it,\n \t */\n \tstrbuf_init(&buffer, 8192);\n \n-\tfor (i = 0; i < entries; i++) {\n+\ti = 0;\n+\twhile (i < entries) {\n \t\tstruct cache_entry *ce = cache[i];\n \t\tstruct cache_tree_sub *sub;\n \t\tconst char *path, *slash;\n@@ -320,7 +324,7 @@ 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 += sub->cache_tree->entry_count;\n \t\t\tsha1 = sub->cache_tree->sha1;\n \t\t\tmode = S_IFDIR;\n \t\t}\n@@ -328,6 +332,7 @@ static int update_one(struct cache_tree *it,\n \t\t\tsha1 = ce->sha1;\n \t\t\tmode = ce->ce_mode;\n \t\t\tentlen = pathlen - baselen;\n+\t\t\ti++;\n \t\t}\n \t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {\n \t\t\tstrbuf_release(&buffer);\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"204953","messageId":"1355631328-26678-4-git-send-email-pclouds@gmail.com","threadId":"32355","inReplyTo":"1355631328-26678-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-16T04:15:27Z","receivedAt":"2012-12-16T04:15:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"entry_count is used in update_one() for two purposes:\n\n1. to skip through the number of processed entries in in-memory index\n2. to record the number of entries this cache-tree covers on disk\n\nUnfortunately when CE_REMOVE is present these numbers are not the same\nbecause CE_REMOVE entries are automatically removed before writing to\ndisk but entry_count is not adjusted and still counts CE_REMOVE\nentries.\n\nSeparate the two use cases into two different variables. #1 is taken\ncare by the new field count in struct cache_tree_sub and entry_count\nis prepared for #2.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache-tree.c | 30 +++++++++++++++++++++++-------\n cache-tree.h | 1 +\n 2 files changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 44eed28..2c10b2e 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -238,6 +238,7 @@ static int update_one(struct cache_tree *it,\n \t\t int entries,\n \t\t const char *base,\n \t\t int baselen,\n+\t\t int *skip_count,\n \t\t int flags)\n {\n \tstruct strbuf buffer;\n@@ -245,6 +246,8 @@ static int update_one(struct cache_tree *it,\n \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n \tint i;\n \n+\t*skip_count = 0;\n+\n \tif (0 <= it->entry_count && has_sha1_file(it->sha1))\n \t\treturn it->entry_count;\n \n@@ -264,7 +267,7 @@ static int update_one(struct cache_tree *it,\n \t\tstruct cache_entry *ce = cache[i];\n \t\tstruct cache_tree_sub *sub;\n \t\tconst char *path, *slash;\n-\t\tint pathlen, sublen, subcnt;\n+\t\tint pathlen, sublen, subcnt, subskip;\n \n \t\tpath = ce->name;\n \t\tpathlen = ce_namelen(ce);\n@@ -289,10 +292,13 @@ static int update_one(struct cache_tree *it,\n \t\t\t\t cache + i, entries - i,\n \t\t\t\t path,\n \t\t\t\t baselen + sublen + 1,\n+\t\t\t\t &subskip,\n \t\t\t\t flags);\n \t\tif (subcnt < 0)\n \t\t\treturn subcnt;\n \t\ti += subcnt;\n+\t\tsub->count = subcnt; /* to be used in the next loop */\n+\t\t*skip_count += subskip;\n \t\tsub->used = 1;\n \t}\n \n@@ -324,7 +330,7 @@ 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;\n+\t\t\ti += sub->count;\n \t\t\tsha1 = sub->cache_tree->sha1;\n \t\t\tmode = S_IFDIR;\n \t\t}\n@@ -340,8 +346,18 @@ 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\t*skip_count = *skip_count + 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (ce->ce_flags & CE_INTENT_TO_ADD)\n+\t\t\tcontinue;\n \n \t\tstrbuf_grow(&buffer, entlen + 100);\n \t\tstrbuf_addf(&buffer, \"%o %.*s%c\", mode, entlen, path + baselen, '\\0');\n@@ -361,7 +377,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 = i - *skip_count;\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@@ -375,11 +391,11 @@ int cache_tree_update(struct cache_tree *it,\n \t\t int entries,\n \t\t int flags)\n {\n-\tint i;\n+\tint i, skip;\n \ti = verify_cache(cache, entries, flags);\n \tif (i)\n \t\treturn i;\n-\ti = update_one(it, cache, entries, \"\", 0, flags);\n+\ti = update_one(it, cache, entries, \"\", 0, &skip, flags);\n \tif (i < 0)\n \t\treturn i;\n \treturn 0;\ndiff --git a/cache-tree.h b/cache-tree.h\nindex d8cb2e9..55d0f59 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -7,6 +7,7 @@\n struct cache_tree;\n struct cache_tree_sub {\n \tstruct cache_tree *cache_tree;\n+\tint count;\t\t/* internally used by update_one() */\n \tint namelen;\n \tint used;\n \tchar name[FLEX_ARRAY];\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"204954","messageId":"1355631328-26678-5-git-send-email-pclouds@gmail.com","threadId":"32355","inReplyTo":"1355631328-26678-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v4 4/4] 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-16T04:15:28Z","receivedAt":"2012-12-16T04:15:28Z","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 cache-tree.c | 14 ++++++++++++--\n t/t2203-add-intent.sh | 20 ++++++++++++++++++++\n 2 files changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 2c10b2e..37e4d00 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -244,6 +244,7 @@ static int update_one(struct cache_tree *it,\n \tstruct strbuf buffer;\n \tint missing_ok = flags & WRITE_TREE_MISSING_OK;\n \tint dryrun = flags & WRITE_TREE_DRY_RUN;\n+\tint to_invalidate = 0;\n \tint i;\n \n \t*skip_count = 0;\n@@ -333,6 +334,8 @@ static int update_one(struct cache_tree *it,\n \t\t\ti += sub->count;\n \t\t\tsha1 = sub->cache_tree->sha1;\n \t\t\tmode = S_IFDIR;\n+\t\t\tif (sub->cache_tree->entry_count < 0)\n+\t\t\t\tto_invalidate = 1;\n \t\t}\n \t\telse {\n \t\t\tsha1 = ce->sha1;\n@@ -356,8 +359,15 @@ static int update_one(struct cache_tree *it,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (ce->ce_flags & CE_INTENT_TO_ADD)\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@@ -377,7 +387,7 @@ static int update_one(struct cache_tree *it,\n \t}\n \n \tstrbuf_release(&buffer);\n-\tit->entry_count = i - *skip_count;\n+\tit->entry_count = to_invalidate ? -1 : i - *skip_count;\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":"204958","messageId":"7vk3siqx8b.fsf@alter.siamese.dyndns.org","threadId":"32355","inReplyTo":"1355631328-26678-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-16T07:20:36Z","receivedAt":"2012-12-16T07:20:36Z","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> entry_count is used in update_one() for two purposes:\n>\n> 1. to skip through the number of processed entries in in-memory index\n> 2. to record the number of entries this cache-tree covers on disk\n>\n> Unfortunately when CE_REMOVE is present these numbers are not the same\n> because CE_REMOVE entries are automatically removed before writing to\n> disk but entry_count is not adjusted and still counts CE_REMOVE\n> entries.\n\nNicely explained. I wonder if we can also add a piece of test to\nthe patch 4/4 to demonstrate the issue with CE_REMOVE entries,\nthough.\n\nThanks.\n"},{"id":"204963","messageId":"CACsJy8DgwQ2jn=tPpE8f22JNE1Vih86d=Xf1iGBznLcwMukd+w@mail.gmail.com","threadId":"32355","inReplyTo":"7vk3siqx8b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-12-16T10:15:48Z","receivedAt":"2012-12-16T10:15:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Dec 16, 2012 at 2:20 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> entry_count is used in update_one() for two purposes:\n>>\n>> 1. to skip through the number of processed entries in in-memory index\n>> 2. to record the number of entries this cache-tree covers on disk\n>>\n>> Unfortunately when CE_REMOVE is present these numbers are not the same\n>> because CE_REMOVE entries are automatically removed before writing to\n>> disk but entry_count is not adjusted and still counts CE_REMOVE\n>> entries.\n>\n> Nicely explained. I wonder if we can also add a piece of test to\n> the patch 4/4 to demonstrate the issue with CE_REMOVE entries,\n> though.\n\nA hand crafted one, maybe. I did not attempt to recreate it with git\ncommands (and I don't think we update cache-tree after unpack_trees).\nSo I wrote something like this instead:\n\nint main(int ac, char **av)\n{\n\tunsigned char sha1[20];\n\tsetup_git_directory();\n\tread_cache();\n\tactive_cache[1]->ce_flags |= CE_REMOVE;\n\twrite_cache_as_tree(sha1, 0, NULL);\n\treturn 0;\n}\n\nI can polish it a bit and write new tests based on it and\ntest-dump-cache-tree if you want.\n-- \nDuy\n"},{"id":"204964","messageId":"80fec1df-83ec-4634-b06e-27369949af94@email.android.com","threadId":"32355","inReplyTo":"CACsJy8DgwQ2jn=tPpE8f22JNE1Vih86d=Xf1iGBznLcwMukd+w@mail.gmail.com","subject":"Re: [PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-16T10:38:40Z","receivedAt":"2012-12-16T10:38:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\n\nNguyen Thai Ngoc Duy wrote:\n\n>On Sun, Dec 16, 2012 at 2:20 PM, Junio C Hamano \n>wrote:\n>> Nicely explained. I wonder if we can also add a piece of test to\n>> the patch 4/4 to demonstrate the issue with CE_REMOVE entries,\n>> though.\n>\n>A hand crafted one, maybe. I did not attempt to recreate it with git\n>commands (and I don't think we update cache-tree after unpack_trees).\n\nYeah, that's what I thought. No need to bother creating a bug that won't appear in the real life :-) \n\nTusks for sanity checking.\n-- \nPardon terseness, typo and HTML from a tablet.\n"}]}