{"thread":{"id":"26416","subject":"[PATCH] cache-tree: do not cache empty trees","startedAt":"2011-02-05T08:30:11Z","lastAt":"2011-02-16T14:29:58Z","messageCount":20,"participants":["Nguyễn Thái Ngọc Duy","Jonathan Nieder","Nguyen Thai Ngoc Duy","Junio C Hamano","Ilari Liusvaara","Yann Dirson","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"160429","messageId":"1296894611-29398-1-git-send-email-pclouds@gmail.com","threadId":"26416","inReplyTo":null,"subject":"[PATCH] cache-tree: do not cache empty trees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-05T08:30:11Z","receivedAt":"2011-02-05T08:30:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current index does not support empty trees. But users can construct\nempty trees directly using plumbing. When empty trees are checked out,\nthings become inconsistent:\n\n - If cache-tree somehow is invalidated, when a tree is read to index,\n   empty trees disappear. When we write trees back, empty trees will\n   be gone.\n\n - If cache-tree is generated by read-tree and remains valid by the\n   time trees are written back, empty trees remain.\n\nLet's do it in a consistent way, always disregard empty trees in\nindex. If users choose to create empty trees their own way, they\nshould not use index at all.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n On Wed, Feb 2, 2011 at 2:09 AM, Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:\n > Yes, writing to index/working area. IIRC, having such entry in tree causes\n > a \"ghost directory\". I don't exactly recall what such thing broke, but I\n > remember that it broke something (merging?)...\n >\n > Those ghosts also had annoying tendency to persist between commits. Commits\n > didn't kill them. Rm didn't work. You had to create something on top/inside to\n > get rid of them.\n\n That's probably because of cache-tree. Empty trees can't exist in\n index so an operation \"trees -> index -> trees\" will remove empty\n trees.\n\n But read-tree can preserve the exact structure of original trees\n (including empty trees) so if that particular path is untouched,\n empty trees will remain.\n\n Perhaps a patch like this for pre-1.8.0?\n\n cache-tree.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f755590..f717793 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -621,6 +621,8 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n \t\t\tif (!subtree->object.parsed)\n \t\t\t\tparse_tree(subtree);\n+\t\t\tif (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN))\n+\t\t\t\tcontinue;\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n \t\t\tprime_cache_tree_rec(sub->cache_tree, subtree);\n-- \n1.7.3.4.878.g439c7\n"},{"id":"160431","messageId":"1296899427-1394-1-git-send-email-pclouds@gmail.com","threadId":"26416","inReplyTo":"1296894611-29398-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] cache-tree: do not cache empty trees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-05T09:50:27Z","receivedAt":"2011-02-05T09:50:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current index does not support empty trees. But users can construct\nempty trees directly using plumbing. When empty trees are checked out,\nthings become inconsistent:\n\n - If cache-tree somehow is invalidated, when a tree is read to index,\n   empty trees disappear. When we write trees back, empty trees will\n   be gone.\n\n - If cache-tree is generated by read-tree and remains valid by the\n   time trees are written back, empty trees remain.\n\nLet's do it in a consistent way, always disregard empty trees in\nindex. If users choose to create empty trees their own way, they\nshould not use index at all.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Scratch the first version. This one actually works.\n\n cache-tree.c               |    9 +++++++++\n t/t1013-read-tree-empty.sh |   38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 0 deletions(-)\n create mode 100755 t/t1013-read-tree-empty.sh\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f755590..03732ad 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n \t\t\tif (!subtree->object.parsed)\n \t\t\t\tparse_tree(subtree);\n+\t\t\tif (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) {\n+\t\t\t\twarning(\"empty tree detected! Will be removed from new commits\");\n+\t\t\t\tcnt = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n \t\t\tprime_cache_tree_rec(sub->cache_tree, subtree);\n+\t\t\tif (sub->cache_tree->entry_count == -1) {\n+\t\t\t\tcnt = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t\tcnt += sub->cache_tree->entry_count;\n \t\t}\n \t}\ndiff --git a/t/t1013-read-tree-empty.sh b/t/t1013-read-tree-empty.sh\nnew file mode 100755\nindex 0000000..a9279f0\n--- /dev/null\n+++ b/t/t1013-read-tree-empty.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='read-tree with empty trees'\n+\n+. ./test-lib.sh\n+\n+T1=f4ec99e8174c01eab488469b4c2680500bbb18da\n+T2=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\n+test_expect_success 'setup' '\n+\tprintf \"40000 empty\\0\\x4b\\x82\\x5d\\xc6\\x42\\xcb\\x6e\\xb9\\xa0\\x60\\xe5\\x4b\\xf8\\xd6\\x92\\x88\\xfb\\xee\\x49\\x04\" >newtree &&\n+\tgit hash-object -w -t tree newtree >actual &&\n+\techo $T1 >expected\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'ls-tree T1 (with empty tree)' '\n+\tgit ls-tree $T1 >actual &&\n+\tcat <<EOF >expected &&\n+040000 tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\tempty\n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'write-tree removes empty tree' '\n+\tgit read-tree \"$T1\" &&\n+\tgit write-tree >actual\n+\techo $T2 >expected\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'ls-tree T2 (without empty tree)' '\n+\tgit ls-tree $T2 >actual &&\n+\t: >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n1.7.3.4.878.g439c7\n"},{"id":"160433","messageId":"20110205101412.GB1685@elie","threadId":"26416","inReplyTo":"1296899427-1394-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-05T10:14:12Z","receivedAt":"2011-02-05T10:14:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSome quick nits to save myself time.  The basic idea of the patch\nseems sound.\n\nNguyễn Thái Ngọc Duy wrote:\n\n> --- /dev/null\n> +++ b/t/t1013-read-tree-empty.sh\n> @@ -0,0 +1,38 @@\n> +#!/bin/sh\n> +\n> +test_description='read-tree with empty trees'\n> +\n> +. ./test-lib.sh\n> +\n> +T1=f4ec99e8174c01eab488469b4c2680500bbb18da\n> +T2=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\nWhat are these trees?  Do they need to be hardcoded?\n\n> +\n> +test_expect_success 'setup' '\n> +\tprintf \"40000 empty\\0\\x4b\\x82\\x5d\\xc6\\x42\\xcb\\x6e\\xb9\\xa0\\x60\\xe5\\x4b\\xf8\\xd6\\x92\\x88\\xfb\\xee\\x49\\x04\" >newtree &&\n\nprintf '\\xab' is unfortunately unportable.  I suppose\nthis should rather say something like\n\n test_unequal () {\n\tprintf '%s\\n' \"$1\" >bad &&\n\tprintf '%s\\n' \"$2\" >actual &&\n\t! test_cmp bad actual\n }\n\n\tempty_tree=$(git mktree </dev/null) &&\n\ttree_with_empty_subtree=$(\n\t\techo \"040000 tree $empty_tree\tempty\" |\n\t\tgit mktree\n\t) &&\n\ttest_unequal \"$empty_tree\" \"$tree_with_empty_subtree\"\n\n> +test_expect_success 'ls-tree T1 (with empty tree)' '\n> +\tgit ls-tree $T1 >actual &&\n> +\tcat <<EOF >expected &&\n> +040000 tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\tempty\n> +EOF\n> +\ttest_cmp expected actual\n> +'\n\n\techo \"040000 tree $empty_tree\tempty\" >expect &&\n\tgit ls-tree \"$tree_with_empty_subtree\" >actual &&\n\ttest_cmp expect actual\n\n> +\n> +test_expect_success 'write-tree removes empty tree' '\n> +\tgit read-tree \"$T1\" &&\n> +\tgit write-tree >actual\n> +\techo $T2 >expected\n> +\ttest_cmp expected actual\n> +'\n\n\tgit read-tree \"$tree_with_empty_subtree\" &&\n\t...\n\nSane?\nJonathan\n"},{"id":"160434","messageId":"AANLkTingZB_+3W-PBATCVywjR31ft_AH+e0Z+4rsUR45@mail.gmail.com","threadId":"26416","inReplyTo":"20110205101412.GB1685@elie","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-05T10:32:05Z","receivedAt":"2011-02-05T10:32:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/2/5 Jonathan Nieder <jrnieder@gmail.com>:\n> Nguyễn Thái Ngọc Duy wrote:\n>> +\n>> +T1=f4ec99e8174c01eab488469b4c2680500bbb18da\n>> +T2=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n>\n> What are these trees?  Do they need to be hardcoded?\n\nI have uneasy feeling constructing a tree manually. Hardcoding it (or\nless automatic check) makes me feel better. I'm cooking a patch to\nmake hash-object reject malformed trees/commits/tags. Then I'll remove\nthese SHA-1.\n\n>> +\n>> +test_expect_success 'setup' '\n>> +     printf \"40000 empty\\0\\x4b\\x82\\x5d\\xc6\\x42\\xcb\\x6e\\xb9\\xa0\\x60\\xe5\\x4b\\xf8\\xd6\\x92\\x88\\xfb\\xee\\x49\\x04\" >newtree &&\n>\n> printf '\\xab' is unfortunately unportable.  I suppose\n> this should rather say something like\n>\n>  test_unequal () {\n>        printf '%s\\n' \"$1\" >bad &&\n>        printf '%s\\n' \"$2\" >actual &&\n>        ! test_cmp bad actual\n>  }\n>\n>        empty_tree=$(git mktree </dev/null) &&\n>        tree_with_empty_subtree=$(\n>                echo \"040000 tree $empty_tree   empty\" |\n>                git mktree\n>        ) &&\n>        test_unequal \"$empty_tree\" \"$tree_with_empty_subtree\"\n\nHah! Was wondering how the heck they could create trees manually. So\nit's mktree. Thanks!\n\n>        ...\n>\n> Sane?\n\nDon't know. But it's definitely saner than my version.\n-- \nDuy\n"},{"id":"160442","messageId":"1296914835-808-1-git-send-email-pclouds@gmail.com","threadId":"26416","inReplyTo":"1296899427-1394-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] cache-tree: do not cache empty trees","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-05T14:07:15Z","receivedAt":"2011-02-05T14:07:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current index does not support empty trees. But users can construct\nempty trees directly using plumbing. When empty trees are checked out,\nthings become inconsistent:\n\n - If cache-tree somehow is invalidated, when a tree is read to index,\n   empty trees disappear. When we write trees back, empty trees will\n   be gone.\n\n - If cache-tree is generated by read-tree and remains valid by the\n   time trees are written back, empty trees remain.\n\nLet's do it in a consistent way, always disregard empty trees in\nindex. If users choose to create empty trees their own way, they\nshould not use index at all.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Third version. Cleaned up t1013. Also needs [1] for mktree to work\n with empty trees.\n\n [1] http://mid.gmane.org/1296914582-619-1-git-send-email-pclouds@gmail.com\n\n cache-tree.c               |    9 +++++++++\n t/t1013-read-tree-empty.sh |   20 ++++++++++++++++++++\n 2 files changed, 29 insertions(+), 0 deletions(-)\n create mode 100755 t/t1013-read-tree-empty.sh\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f755590..03732ad 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n \t\t\tif (!subtree->object.parsed)\n \t\t\t\tparse_tree(subtree);\n+\t\t\tif (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) {\n+\t\t\t\twarning(\"empty tree detected! Will be removed in new commits\");\n+\t\t\t\tcnt = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t\tsub = cache_tree_sub(it, entry.path);\n \t\t\tsub->cache_tree = cache_tree();\n \t\t\tprime_cache_tree_rec(sub->cache_tree, subtree);\n+\t\t\tif (sub->cache_tree->entry_count == -1) {\n+\t\t\t\tcnt = -1;\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t\tcnt += sub->cache_tree->entry_count;\n \t\t}\n \t}\ndiff --git a/t/t1013-read-tree-empty.sh b/t/t1013-read-tree-empty.sh\nnew file mode 100755\nindex 0000000..8d2ab97\n--- /dev/null\n+++ b/t/t1013-read-tree-empty.sh\n@@ -0,0 +1,20 @@\n+#!/bin/sh\n+\n+test_description='read-tree with empty trees'\n+\n+. ./test-lib.sh\n+\n+EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\n+test_expect_success 'setup' '\n+\techo \"040000 tree $EMPTY_TREE\tempty\" | git mktree >tree\n+'\n+\n+test_expect_success 'write-tree removes empty tree' '\n+\tgit read-tree `cat tree` &&\n+\tgit write-tree >actual\n+\techo $EMPTY_TREE >expected\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n1.7.3.4.878.g439c7\n"},{"id":"160568","messageId":"7v62swwq7s.fsf@alter.siamese.dyndns.org","threadId":"26416","inReplyTo":"1296914835-808-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-07T02:09:11Z","receivedAt":"2011-02-07T02:09:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> diff --git a/cache-tree.c b/cache-tree.c\n> index f755590..03732ad 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n>  \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n>  \t\t\tif (!subtree->object.parsed)\n>  \t\t\t\tparse_tree(subtree);\n> +\t\t\tif (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) {\n> +\t\t\t\twarning(\"empty tree detected! Will be removed in new commits\");\n> +\t\t\t\tcnt = -1;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nYou shouldn't need the cast (if you did, then hashcmp() macro should be\nfixed so that you don't need to).\n\nI don't think warning() is warranted for an operation you introduced to\nkeep the internal data structure consistent.\n\nShould this comparison done after we parsed the subtree, or should we be\ndoing that before it?\n\nIf you are adding this new check to a point where we have already parsed\nthe subtree object, don't you have a better and cheaper way to detect if\nthe subtree is empty than the 20-byte comparision, namely, perhaps by\nlooking at subtree->size?\n"},{"id":"160571","messageId":"AANLkTinCyOq4rmb-tf4B91bK97GWca-4DyC715tUv+zx@mail.gmail.com","threadId":"26416","inReplyTo":"7v62swwq7s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-07T02:36:37Z","receivedAt":"2011-02-07T02:36:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/2/7 Junio C Hamano <gitster@pobox.com>:\n>> diff --git a/cache-tree.c b/cache-tree.c\n>> index f755590..03732ad 100644\n>> --- a/cache-tree.c\n>> +++ b/cache-tree.c\n>> @@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n>>                       struct tree *subtree = lookup_tree(entry.sha1);\n>>                       if (!subtree->object.parsed)\n>>                               parse_tree(subtree);\n>> +                     if (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) {\n>> +                             warning(\"empty tree detected! Will be removed in new commits\");\n>> +                             cnt = -1;\n>> +                             break;\n>> +                     }\n>\n> You shouldn't need the cast (if you did, then hashcmp() macro should be\n> fixed so that you don't need to).\n\nOr perhaps EMPTY_TREE_SHA1_BIN should be casted to const unsigned char\n*. That would eliminate 4 typecastings elsewhere.\n\n> I don't think warning() is warranted for an operation you introduced to\n> keep the internal data structure consistent.\n\nWorse. I don't think users know an empty tree is added or removed.\ndiff-tree does not show it (or should not, I haven't tested).\n\n> Should this comparison done after we parsed the subtree, or should we be\n> doing that before it?\n>\n> If you are adding this new check to a point where we have already parsed\n> the subtree object, don't you have a better and cheaper way to detect if\n> the subtree is empty than the 20-byte comparision, namely, perhaps by\n> looking at subtree->size?\n\nOK before is better.\n-- \nDuy\n"},{"id":"161591","messageId":"20110207081727.GB2736@elie","threadId":"26416","inReplyTo":"7v62swwq7s.fsf@alter.siamese.dyndns.org","subject":"[PATCH] correct type of EMPTY_TREE_SHA1_BIN","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-07T08:17:27Z","receivedAt":"2011-02-07T08:17:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n>> --- a/cache-tree.c\n>> +++ b/cache-tree.c\n>> @@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n>>  \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n>>  \t\t\tif (!subtree->object.parsed)\n>>  \t\t\t\tparse_tree(subtree);\n>> +\t\t\tif (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) {\n>> +\t\t\t\twarning(\"empty tree detected! Will be removed in new commits\");\n>> +\t\t\t\tcnt = -1;\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>\n> You shouldn't need the cast (if you did, then hashcmp() macro should be\n> fixed so that you don't need to).\n\nIsn't this a bug in the definition of EMPTY_TREE_SHA1_BIN rather than\nthe signature of hashcmp?\n\n-- 8< --\nSubject: correct type of EMPTY_TREE_SHA1_BIN\n\nFunctions such as hashcmp that expect a binary SHA-1 value take\nparameters of type \"unsigned char *\" to avoid accepting a textual\nSHA-1 passed by mistake.  Unfortunately, this means passing the string\nliteral EMPTY_TREE_SHA1_BIN requires an ugly cast.  Tweak the\ndefinition of EMPTY_TREE_SHA1_BIN to produce a value of more\nconvenient type.\n\nIn the future the definition might change to\n\n\textern const unsigned char empty_tree_sha1_bin[20];\n\t#define EMPTY_TREE_SHA1_BIN empty_tree_sha1_bin\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n builtin/checkout.c |    2 +-\n cache.h            |    4 +++-\n notes-merge.c      |    2 +-\n sha1_file.c        |    2 +-\n 4 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 757f9a0..cd7f56e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -404,7 +404,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.dir->exclude_per_dir = \".gitignore\";\n \t\ttree = parse_tree_indirect(old->commit ?\n \t\t\t\t\t   old->commit->object.sha1 :\n-\t\t\t\t\t   (unsigned char *)EMPTY_TREE_SHA1_BIN);\n+\t\t\t\t\t   EMPTY_TREE_SHA1_BIN);\n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n \t\ttree = parse_tree_indirect(new->commit->object.sha1);\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\ndiff --git a/cache.h b/cache.h\nindex d83d68c..3abf895 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -676,9 +676,11 @@ static inline void hashclr(unsigned char *hash)\n \n #define EMPTY_TREE_SHA1_HEX \\\n \t\"4b825dc642cb6eb9a060e54bf8d69288fbee4904\"\n-#define EMPTY_TREE_SHA1_BIN \\\n+#define EMPTY_TREE_SHA1_BIN_LITERAL \\\n \t \"\\x4b\\x82\\x5d\\xc6\\x42\\xcb\\x6e\\xb9\\xa0\\x60\" \\\n \t \"\\xe5\\x4b\\xf8\\xd6\\x92\\x88\\xfb\\xee\\x49\\x04\"\n+#define EMPTY_TREE_SHA1_BIN \\\n+\t ((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)\n \n int git_mkstemp(char *path, size_t n, const char *template);\n \ndiff --git a/notes-merge.c b/notes-merge.c\nindex 71c4d45..1467ad3 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -615,7 +615,7 @@ int notes_merge(struct notes_merge_options *o,\n \tbases = get_merge_bases(local, remote, 1);\n \tif (!bases) {\n \t\tbase_sha1 = null_sha1;\n-\t\tbase_tree_sha1 = (unsigned char *)EMPTY_TREE_SHA1_BIN;\n+\t\tbase_tree_sha1 = EMPTY_TREE_SHA1_BIN;\n \t\tOUTPUT(o, 4, \"No merge base found; doing history-less merge\");\n \t} else if (!bases->next) {\n \t\tbase_sha1 = bases->item->object.sha1;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d86a8db..b44fc95 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2048,7 +2048,7 @@ static struct cached_object {\n static int cached_object_nr, cached_object_alloc;\n \n static struct cached_object empty_tree = {\n-\tEMPTY_TREE_SHA1_BIN,\n+\tEMPTY_TREE_SHA1_BIN_LITERAL,\n \tOBJ_TREE,\n \t\"\",\n \t0\n-- \n1.7.4\n"},{"id":"160591","messageId":"20110207091740.GA5391@elie","threadId":"26416","inReplyTo":"1296914835-808-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-07T09:17:40Z","receivedAt":"2011-02-07T09:17:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> Let's do it in a consistent way, always disregard empty trees in\n> index. If users choose to create empty trees their own way, they\n> should not use index at all.\n\nWhile this violates some seeming invariants, like\n\n1.\n\tgit reset --hard\n\tgit commit --allow-empty\n\tgit rev-parse HEAD^^{tree} >expect\n\tgit rev-parse HEAD^{tree} >actual\n\ttest_cmp expect actual\n\n2.\n\tgit reset --hard\n\tgit revert HEAD\n\tif git rev-parse HEAD~2\n\tthen\n\t\tgit rev-parse HEAD~2^{tree} >expect\n\t\tgit rev-parse HEAD^{tree} >actual\n\t\ttest_cmp expect actual\n\tfi\n\n, I think it's a good change.  Malformed modes in trees already break\nthose false invariants iiuc.\n\nThanks.\n\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n>  \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n>  \t\t\tif (!subtree->object.parsed)\n>  \t\t\t\tparse_tree(subtree);\n> +\t\t\tif (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) {\n> +\t\t\t\twarning(\"empty tree detected! Will be removed in new commits\");\n> +\t\t\t\tcnt = -1;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nAside from the warning, this part is an optimization, right?\n\n>  \t\t\tsub = cache_tree_sub(it, entry.path);\n>  \t\t\tsub->cache_tree = cache_tree();\n>  \t\t\tprime_cache_tree_rec(sub->cache_tree, subtree);\n> +\t\t\tif (sub->cache_tree->entry_count == -1) {\n> +\t\t\t\tcnt = -1;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nWould be nice to include a test for this, like so:\n\n\tsubdir/\n\t\tempty1/\n\t\tsubsubdir/\n\t\t\tempty2/\n\t\t\tempty3/\n\n> --- /dev/null\n> +++ b/t/t1013-read-tree-empty.sh\n> @@ -0,0 +1,20 @@\n> +#!/bin/sh\n> +\n> +test_description='read-tree with empty trees'\n> +\n> +. ./test-lib.sh\n> +\n> +EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\nIf we _have_ to hard-code this (why?) then I'd prefer to do so\nin test-lib.sh.\n\n[...]\n> +test_expect_success 'write-tree removes empty tree' '\n> +\tgit read-tree `cat tree` &&\n> +\tgit write-tree >actual\n> +\techo $EMPTY_TREE >expected\n> +\ttest_cmp expected actual\n\nBroken &&-chain.  Not sure why this test relies on virtual objects\nand another (independently nice) patch instead of adding the empty\ntree to the object db --- is there something subtle I am missing?\n\nThe test does not distinguish between success due to git read-tree\nomitting empty trees and success due to git mktree omitting empty\ntrees.\n\nHope that helps,\nJonathan\n"},{"id":"160594","messageId":"20110207095713.GA19653@do","threadId":"26416","inReplyTo":"20110207091740.GA5391@elie","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-07T09:57:13Z","receivedAt":"2011-02-07T09:57:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 07, 2011 at 03:17:40AM -0600, Jonathan Nieder wrote:\n> While this violates some seeming invariants, like\n> \n> 1.\n> \tgit reset --hard\n> \tgit commit --allow-empty\n> \tgit rev-parse HEAD^^{tree} >expect\n> \tgit rev-parse HEAD^{tree} >actual\n> \ttest_cmp expect actual\n> \n> 2.\n> \tgit reset --hard\n> \tgit revert HEAD\n> \tif git rev-parse HEAD~2\n> \tthen\n> \t\tgit rev-parse HEAD~2^{tree} >expect\n> \t\tgit rev-parse HEAD^{tree} >actual\n> \t\ttest_cmp expect actual\n> \tfi\n> \n> , I think it's a good change.  Malformed modes in trees already break\n> those false invariants iiuc.\n\nPerhaps it's not a good approach after all. What I wanted was to make\npre-1.8.0 tolerate empty trees created by 1.8.0. Perhaps it's better\nto just let pre-1.8.0 refuse to work with empty trees, forcing users\nto upgrade to 1.8.0?\n\nThe (untested) patch below would make git refuse to create an index\nfrom a tree that contains empty trees. Hmm?\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f755590..e33998a 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -619,6 +619,8 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n \t\telse {\n \t\t\tstruct cache_tree_sub *sub;\n \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n+\t\t\tif (!hashcmp(entry.sha1, EMPTY_TREE_SHA1_BIN))\n+\t\t\t\tdie(\"empty tree .../%s detected!\", entry.path);\n \t\t\tif (!subtree->object.parsed)\n \t\t\t\tparse_tree(subtree);\n \t\t\tsub = cache_tree_sub(it, entry.path);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 1ca41b1..0e6738e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -434,6 +434,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long\n \tvoid *buf[MAX_UNPACK_TREES];\n \tstruct traverse_info newinfo;\n \tstruct name_entry *p;\n+\tstruct unpack_trees_options *o = info->data;\n \n \tp = names;\n \twhile (!p->mode)\n@@ -447,8 +448,11 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long\n \n \tfor (i = 0; i < n; i++, dirmask >>= 1) {\n \t\tconst unsigned char *sha1 = NULL;\n-\t\tif (dirmask & 1)\n+\t\tif (dirmask & 1) {\n \t\t\tsha1 = names[i].sha1;\n+\t\t\tif (o->merge && !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))\n+\t\t\t\treturn error(\"empty tree .../%s detected!\", p->path);\n+\t\t}\n \t\tbuf[i] = fill_tree_descriptor(t+i, sha1);\n \t}\n \n\n-- \nDuy\n"},{"id":"160604","messageId":"20110207121855.GA7298@LK-Perkele-VI.localdomain","threadId":"26416","inReplyTo":"20110207095713.GA19653@do","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2011-02-07T12:18:55Z","receivedAt":"2011-02-07T12:18:55Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Mon, Feb 07, 2011 at 04:57:13PM +0700, Nguyen Thai Ngoc Duy wrote:\n> \n> Perhaps it's not a good approach after all. What I wanted was to make\n> pre-1.8.0 tolerate empty trees created by 1.8.0. Perhaps it's better\n> to just let pre-1.8.0 refuse to work with empty trees, forcing users\n> to upgrade to 1.8.0?\n> \n> The (untested) patch below would make git refuse to create an index\n> from a tree that contains empty trees. Hmm?\n \nRemember, many distros ship with old versions of Git. Debian stable\nis now at 1.7.2.3 (Squeeze became Debian 6.0) and it'll take years\nbefore next release. What about these?\n\nMaking previous versions refuse to work with empty trees isn't exactly\ntrivial, as the tree parser seems to be written to be extremely\nliberal on what it accepts.\n\n-Ilari\n"},{"id":"160605","messageId":"AANLkTim-nbVsp8z0Cw+p67JU0B9yAE3hkaZ1ay9cmsjm@mail.gmail.com","threadId":"26416","inReplyTo":"20110207121855.GA7298@LK-Perkele-VI.localdomain","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-07T12:29:33Z","receivedAt":"2011-02-07T12:29:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 7, 2011 at 7:18 PM, Ilari Liusvaara\n<ilari.liusvaara@elisanet.fi> wrote:\n> On Mon, Feb 07, 2011 at 04:57:13PM +0700, Nguyen Thai Ngoc Duy wrote:\n>>\n>> Perhaps it's not a good approach after all. What I wanted was to make\n>> pre-1.8.0 tolerate empty trees created by 1.8.0. Perhaps it's better\n>> to just let pre-1.8.0 refuse to work with empty trees, forcing users\n>> to upgrade to 1.8.0?\n>>\n>> The (untested) patch below would make git refuse to create an index\n>> from a tree that contains empty trees. Hmm?\n>\n> Remember, many distros ship with old versions of Git. Debian stable\n> is now at 1.7.2.3 (Squeeze became Debian 6.0) and it'll take years\n> before next release. What about these?\n\nWaiting a few years is my best bet :P Really I don't figure out any\nother way for migration. New empty trees would end up in repository\nand affect all connected clients regardless version.\n\n> Making previous versions refuse to work with empty trees isn't exactly\n> trivial, as the tree parser seems to be written to be extremely\n> liberal on what it accepts.\n\nThe repository _can_ contain empty trees. The main problem is index\nnot supporting empty trees. I only prevent index being used. If users\ncreate new commits (including empty trees) with mktree and\ncommit-tree, they can also checkout without index using ls-tree and\ncat-file.\n-- \nDuy\n"},{"id":"160606","messageId":"20110207123250.GA11854@elie","threadId":"26416","inReplyTo":"20110207121855.GA7298@LK-Perkele-VI.localdomain","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-07T12:32:50Z","receivedAt":"2011-02-07T12:32:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ilari Liusvaara wrote:\n\n> Remember, many distros ship with old versions of Git. Debian stable\n> is now at 1.7.2.3 (Squeeze became Debian 6.0) and it'll take years\n> before next release. What about these?\n\nThe next Debian point release is scheduled for a month from now. :)\nThere will probably be more point releases after that.\n"},{"id":"160628","messageId":"7v1v3jvaef.fsf@alter.siamese.dyndns.org","threadId":"26416","inReplyTo":"20110207095713.GA19653@do","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-07T20:48:24Z","receivedAt":"2011-02-07T20:48:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> Perhaps it's not a good approach after all. What I wanted was to make\n> pre-1.8.0 tolerate empty trees created by 1.8.0. Perhaps it's better\n> to just let pre-1.8.0 refuse to work with empty trees, forcing users\n> to upgrade to 1.8.0?\n\nI don't think we saw even something remotely resembles an agreement that\nempty tree is a good thing to have yet.  Why are you rushing things?\n"},{"id":"160676","messageId":"AANLkTim_G9cPs=+1GQ2qBEgriOyKYgXk17iHE4oq9h2C@mail.gmail.com","threadId":"26416","inReplyTo":"7v1v3jvaef.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-08T04:11:19Z","receivedAt":"2011-02-08T04:11:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 8, 2011 at 3:48 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n>\n>> Perhaps it's not a good approach after all. What I wanted was to make\n>> pre-1.8.0 tolerate empty trees created by 1.8.0. Perhaps it's better\n>> to just let pre-1.8.0 refuse to work with empty trees, forcing users\n>> to upgrade to 1.8.0?\n>\n> I don't think we saw even something remotely resembles an agreement that\n> empty tree is a good thing to have yet.  Why are you rushing things?\n\nBut empty trees are allowed in repo since 79b1138 (fsck.c: fix bogus\n\"empty tree\" check). Index can't handle empty trees, so it's a bug to\nme that index still accepts them as input and silently discard them. A\nbug regardless directory tracking support in 1.8.0. A corner case that\nnobody would likely encounter (except Ilari and his \"ghost directory\"\nproblem).\n-- \nDuy\n"},{"id":"160677","messageId":"20110208043000.GA6375@elie","threadId":"26416","inReplyTo":"AANLkTim_G9cPs=+1GQ2qBEgriOyKYgXk17iHE4oq9h2C@mail.gmail.com","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-08T04:30:00Z","receivedAt":"2011-02-08T04:30:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> But empty trees are allowed in repo since 79b1138 (fsck.c: fix bogus\n> \"empty tree\" check). Index can't handle empty trees, so it's a bug to\n> me that index still accepts them as input and silently discard them.\n\nFWIW my instinct points in the opposite direction.  I wouldn't mind\nseeing fsck warn about trees containing empty subtrees[1].  As for\ncache-tree, while it is not obvious what the right thing to do is,\ndiscarding empty subtrees sounds accepatable.\n\nFor storage of empty subtrees in repos imported from svn, Yann's idea\nof using .gitattributes somehow (maybe in the parent directory or\nmaybe in the subdir itself) seems oddly appealing.\n\nJust my unproductive two cents,\nJonathan\n\n[1] I suspect 79b1138 was only meant to deal with the \"git commit\n--allow-empty from newly initialized repo\" case.\n"},{"id":"160693","messageId":"20110208104055.GA10629@LK-Perkele-VI.localdomain","threadId":"26416","inReplyTo":"AANLkTim_G9cPs=+1GQ2qBEgriOyKYgXk17iHE4oq9h2C@mail.gmail.com","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2011-02-08T10:40:55Z","receivedAt":"2011-02-08T10:40:55Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Tue, Feb 08, 2011 at 11:11:19AM +0700, Nguyen Thai Ngoc Duy wrote:\n> On Tue, Feb 8, 2011 at 3:48 AM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> bug regardless directory tracking support in 1.8.0. A corner case that\n> nobody would likely encounter (except Ilari and his \"ghost directory\"\n> problem).\n\nExcept it wasn't me who directly saw those problems. It was somebody else\non #git (back when I was there). That was a while ago. That's the reason\nI don't exactly remember what exactly it did (possibly false modifications\ncausing some commands (merge/rebase) to abort?).\n\nHmm. I should test what exactly happens (including with older git versions\nto see if those problems have (possibly unintentionally) got fixed).\n\n-Ilari\n"},{"id":"160797","messageId":"7vwrl8n5pe.fsf@alter.siamese.dyndns.org","threadId":"26416","inReplyTo":"20110207081727.GB2736@elie","subject":"Re: [PATCH] correct type of EMPTY_TREE_SHA1_BIN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-09T23:33:49Z","receivedAt":"2011-02-09T23:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>\n>>> --- a/cache-tree.c\n>>> +++ b/cache-tree.c\n>>> @@ -621,9 +621,18 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n>>>  \t\t\tstruct tree *subtree = lookup_tree(entry.sha1);\n>>>  \t\t\tif (!subtree->object.parsed)\n>>>  \t\t\t\tparse_tree(subtree);\n>>> +\t\t\tif (!hashcmp(entry.sha1, (unsigned char *)EMPTY_TREE_SHA1_BIN)) {\n>>> +\t\t\t\twarning(\"empty tree detected! Will be removed in new commits\");\n>>> +\t\t\t\tcnt = -1;\n>>> +\t\t\t\tbreak;\n>>> +\t\t\t}\n>>\n>> You shouldn't need the cast (if you did, then hashcmp() macro should be\n>> fixed so that you don't need to).\n>\n> Isn't this a bug in the definition of EMPTY_TREE_SHA1_BIN rather than\n> the signature of hashcmp?\n\nYeah, you are right.\n"},{"id":"161171","messageId":"20110215111926.5fdafa1d@chalon.bertin.fr","threadId":"26416","inReplyTo":"20110208043000.GA6375@elie","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Yann Dirson","fromEmail":"dirson@bertin.fr","sentAt":"2011-02-15T10:19:26Z","receivedAt":"2011-02-15T10:19:26Z","isPatch":true,"sender":{"key":"dirson@bertin.fr","avatar":null},"body":"On Mon, 07 Feb 2011 22:30:00 -0600\nJonathan Nieder <jrnieder@gmail.com> wrote:\n> For storage of empty subtrees in repos imported from svn, Yann's idea\n> of using .gitattributes somehow (maybe in the parent directory or\n> maybe in the subdir itself) seems oddly appealing.\n\nIn fact, I was mostly thinking about only using the\ntoplevel .gitattributes, if only to avoid the same sort\nof clutter caused by the .gitignore-trick.\n\n-- \nYann Dirson - Bertin Technologies\n"},{"id":"161302","messageId":"201102161530.01017.jnareb@gmail.com","threadId":"26416","inReplyTo":"20110215111926.5fdafa1d@chalon.bertin.fr","subject":"Re: [PATCH] cache-tree: do not cache empty trees","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-02-16T14:29:58Z","receivedAt":"2011-02-16T14:29:58Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 15 Feb 2011, Yann Dirson wrote:\n> On Mon, 07 Feb 2011 22:30:00 -0600\n> Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> > For storage of empty subtrees in repos imported from svn, Yann's idea\n> > of using .gitattributes somehow (maybe in the parent directory or\n> > maybe in the subdir itself) seems oddly appealing.\n> \n> In fact, I was mostly thinking about only using the\n> toplevel .gitattributes, if only to avoid the same sort\n> of clutter caused by the .gitignore-trick.\n\nI was thinking about configuration variable (to keep or automatically\ndelete empty directories), overridable per-pathname basis with \n.gitattributes, similarly to 'eof' situation.\n\n-- \nJakub Narebski\nPoland\n"}]}