{"thread":{"id":"34240","subject":"fast-import bug?","startedAt":"2013-06-21T09:21:47Z","lastAt":"2013-06-23T14:55:03Z","messageCount":6,"participants":["Dave Abrahams","John Keeping"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"221567","messageId":"m2zjuj2504.fsf@cube.gateway.2wire.net","threadId":"34240","inReplyTo":null,"subject":"fast-import bug?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2013-06-21T09:21:47Z","receivedAt":"2013-06-21T09:21:47Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"\nThe docs for fast-import seem to imply that I can use \"ls\" to get the\nSHA1 of a commit for which I have a mark:\n\n Reading from a named tree\n The can be a mark reference (:) or the full 40-byte\n SHA-1 of a Git tag, commit, or tree object, preexisting or waiting to\n be written. The path is relative to the top level of the tree named by\n .\n\n 'ls' SP SP LF\n\n See filemodify above for a detailed description of .\n\n Output uses the same format as git ls-tree -- :\n\n SP ('blob' | 'tree' | 'commit') SP HT LF\n\n The represents the blob, tree, or commit object at and\n ^^^^^^\n can be used in later cat-blob, filemodify, or ls commands.\n\nbut I can't get it to work. It's not entirely clear it's supposed to\nwork. What path would I pass? Passing an empty path simply causes git\nto report \"missing \".\n\nTIA,\nDave\n\n-- \nDave Abrahams\n"},{"id":"221676","messageId":"20130622102157.GE4676@serenity.lan","threadId":"34240","inReplyTo":"m2zjuj2504.fsf@cube.gateway.2wire.net","subject":"Re: fast-import bug?","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-22T10:21:58Z","receivedAt":"2013-06-22T10:21:58Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jun 21, 2013 at 02:21:47AM -0700, Dave Abrahams wrote:\n> The docs for fast-import seem to imply that I can use \"ls\" to get the\n> SHA1 of a commit for which I have a mark:\n> \n> Reading from a named tree\n> The can be a mark reference (:) or the full 40-byte\n> SHA-1 of a Git tag, commit, or tree object, preexisting or waiting to\n> be written. The path is relative to the top level of the tree named by\n> .\n> \n> 'ls' SP SP LF\n> \n> See filemodify above for a detailed description of .\n> \n> Output uses the same format as git ls-tree -- :\n> \n> SP ('blob' | 'tree' | 'commit') SP HT LF\n> \n> The represents the blob, tree, or commit object at and\n> ^^^^^^\n> can be used in later cat-blob, filemodify, or ls commands.\n> \n> but I can't get it to work. It's not entirely clear it's supposed to\n> work. What path would I pass? Passing an empty path simply causes git\n> to report \"missing \".\n\nWhich version of Git are you using? I just tried this and get the error\n\"fatal: Empty path component found in input\", which seems to be from\ncommit 178e1de (fast-import: don't allow 'ls' of path with empty\ncomponents, 2012-03-09), which is included in Git 1.7.9.5.\n\nIt seems to be slightly more complicated than that though, because after\nallowing empty trees I get the \"missing\" message for the root tree.\nThis seems to be because its mode is 0 and not S_IFDIR.\n\nWith the patch below, things are working as I expect but I don't\nunderstand why the mode of the root is not set correctly at this point.\nPerhaps someone more familiar with fast-import will have some insight...\n\n-- >8 --\ndiff --git a/fast-import.c b/fast-import.c\nindex 23f625f..bcce651 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1626,6 +1626,15 @@ del_entry:\n \treturn 1;\n }\n \n+static void copy_tree_entry(struct tree_entry *dst, struct tree_entry *src)\n+{\n+\tmemcpy(dst, src, sizeof(*dst));\n+\tif (src->tree && is_null_sha1(src->versions[1].sha1))\n+\t\tdst->tree = dup_tree_content(src->tree);\n+\telse\n+\t\tdst->tree = NULL;\n+}\n+\n static int tree_content_get(\n \tstruct tree_entry *root,\n \tconst char *p,\n@@ -1651,11 +1660,7 @@ static int tree_content_get(\n \t\te = t->entries[i];\n \t\tif (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {\n \t\t\tif (!slash1) {\n-\t\t\t\tmemcpy(leaf, e, sizeof(*leaf));\n-\t\t\t\tif (e->tree && is_null_sha1(e->versions[1].sha1))\n-\t\t\t\t\tleaf->tree = dup_tree_content(e->tree);\n-\t\t\t\telse\n-\t\t\t\t\tleaf->tree = NULL;\n+\t\t\t\tcopy_tree_entry(leaf, e);\n \t\t\t\treturn 1;\n \t\t\t}\n \t\t\tif (!S_ISDIR(e->versions[1].mode))\n@@ -3065,7 +3070,11 @@ static void parse_ls(struct branch *b)\n \t\t\tdie(\"Garbage after path in: %s\", command_buf.buf);\n \t\tp = uq.buf;\n \t}\n-\ttree_content_get(root, p, &leaf);\n+\tif (!*p) {\n+\t\tcopy_tree_entry(&leaf, root);\n+\t\tleaf.versions[1].mode = S_IFDIR;\n+\t} else\n+\t\ttree_content_get(root, p, &leaf);\n \t/*\n \t * A directory in preparation would have a sha1 of zero\n \t * until it is saved. Save, for simplicity.\n"},{"id":"221701","messageId":"m2txkp1shb.fsf@cube.gateway.2wire.net","threadId":"34240","inReplyTo":"20130622102157.GE4676@serenity.lan","subject":"Re: fast-import bug?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2013-06-23T02:16:48Z","receivedAt":"2013-06-23T02:16:48Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"\non Sat Jun 22 2013, John Keeping wrote:\n\n> On Fri, Jun 21, 2013 at 02:21:47AM -0700, Dave Abrahams wrote:\n>> The docs for fast-import seem to imply that I can use \"ls\" to get the\n>> SHA1 of a commit for which I have a mark:\n>> \n>> Reading from a named tree\n>> The can be a mark reference (:) or the full 40-byte\n>\n>> SHA-1 of a Git tag, commit, or tree object, preexisting or waiting to\n>> be written. The path is relative to the top level of the tree named by\n>> .\n>> \n>> 'ls' SP SP LF\n>> \n>> See filemodify above for a detailed description of .\n>> \n>> Output uses the same format as git ls-tree -- :\n>> \n>> SP ('blob' | 'tree' | 'commit') SP HT LF\n>> \n>> The represents the blob, tree, or commit object at and\n>> ^^^^^^\n>> can be used in later cat-blob, filemodify, or ls commands.\n>> \n>> but I can't get it to work. It's not entirely clear it's supposed to\n>> work. What path would I pass? Passing an empty path simply causes git\n>> to report \"missing \".\n>\n> Which version of Git are you using? \n\n,----[ git --version ]\n| git version 1.8.3.1\n`----\n\n> I just tried this and get the error\n> \"fatal: Empty path component found in input\", \n\nI get that too.\n\n> which seems to be from commit 178e1de (fast-import: don't allow 'ls'\n> of path with empty components, 2012-03-09), which is included in Git\n> 1.7.9.5.\n\nYes, that's at least part of the issue. I notice git-fast-import\nrejects the root path \"\" for other commands, e.g. when used as the\nsource of a filecopy we get the same issue. I also note that the docs\ndon't make it clear that quoting the path is mandatory if it might turn\nout to be empty.\n\n> It seems to be slightly more complicated than that though, because after\n> allowing empty trees I get the \"missing\" message for the root tree.\n\nYeah, I've tried to patch Git to solve this but ran into that problem\nand gave up.\n\n> This seems to be because its mode is 0 and not S_IFDIR.\n\nAha.\n\n> With the patch below, things are working as I expect \n\nAwesome; works for me, too!\n\n> but I don't understand why the mode of the root is not set correctly\n> at this point. Perhaps someone more familiar with fast-import will\n> have some insight...\n\nYeah... there's no bug tracker for Git, right? So if nobody pays\nattention to this thread, the problem will persist?\n\n> -- >8 --\n> diff --git a/fast-import.c b/fast-import.c\n> index 23f625f..bcce651 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1626,6 +1626,15 @@ del_entry:\n> \treturn 1;\n> }\n>\n> +static void copy_tree_entry(struct tree_entry *dst, struct tree_entry *src)\n> +{\n> +\tmemcpy(dst, src, sizeof(*dst));\n> +\tif (src->tree && is_null_sha1(src->versions[1].sha1))\n> +\t\tdst->tree = dup_tree_content(src->tree);\n> +\telse\n> +\t\tdst->tree = NULL;\n> +}\n> +\n> static int tree_content_get(\n> \tstruct tree_entry *root,\n> \tconst char *p,\n> @@ -1651,11 +1660,7 @@ static int tree_content_get(\n> \t\te = t->entries[i];\n> \t\tif (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {\n> \t\t\tif (!slash1) {\n> -\t\t\t\tmemcpy(leaf, e, sizeof(*leaf));\n> -\t\t\t\tif (e->tree && is_null_sha1(e->versions[1].sha1))\n> -\t\t\t\t\tleaf->tree = dup_tree_content(e->tree);\n> -\t\t\t\telse\n> -\t\t\t\t\tleaf->tree = NULL;\n> +\t\t\t\tcopy_tree_entry(leaf, e);\n> \t\t\t\treturn 1;\n> \t\t\t}\n> \t\t\tif (!S_ISDIR(e->versions[1].mode))\n> @@ -3065,7 +3070,11 @@ static void parse_ls(struct branch *b)\n> \t\t\tdie(\"Garbage after path in: %s\", command_buf.buf);\n> \t\tp = uq.buf;\n> \t}\n> -\ttree_content_get(root, p, &leaf);\n> +\tif (!*p) {\n> +\t\tcopy_tree_entry(&leaf, root);\n> +\t\tleaf.versions[1].mode = S_IFDIR;\n> +\t} else\n> +\t\ttree_content_get(root, p, &leaf);\n> \t/*\n> \t * A directory in preparation would have a sha1 of zero\n> \t * until it is saved. Save, for simplicity.\n\n-- \nDave Abrahams\n"},{"id":"221717","messageId":"20130623110933.GG4676@serenity.lan","threadId":"34240","inReplyTo":"m2txkp1shb.fsf@cube.gateway.2wire.net","subject":"Re: fast-import bug?","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T11:09:33Z","receivedAt":"2013-06-23T11:09:33Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Jun 22, 2013 at 07:16:48PM -0700, Dave Abrahams wrote:\n> \n> on Sat Jun 22 2013, John Keeping wrote:\n> \n> > On Fri, Jun 21, 2013 at 02:21:47AM -0700, Dave Abrahams wrote:\n> >> The docs for fast-import seem to imply that I can use \"ls\" to get the\n> >> SHA1 of a commit for which I have a mark:\n> >> \n> >> Reading from a named tree\n> >> The can be a mark reference (:) or the full 40-byte\n> >\n> >> SHA-1 of a Git tag, commit, or tree object, preexisting or waiting to\n> >> be written. The path is relative to the top level of the tree named by\n> >> .\n> >> \n> >> 'ls' SP SP LF\n> >> \n> >> See filemodify above for a detailed description of .\n> >> \n> >> Output uses the same format as git ls-tree -- :\n> >> \n> >> SP ('blob' | 'tree' | 'commit') SP HT LF\n> >> \n> >> The represents the blob, tree, or commit object at and\n> >> ^^^^^^\n> >> can be used in later cat-blob, filemodify, or ls commands.\n> >> \n> >> but I can't get it to work. It's not entirely clear it's supposed to\n> >> work. What path would I pass? Passing an empty path simply causes git\n> >> to report \"missing \".\n> >\n> > Which version of Git are you using? \n> \n> ,----[ git --version ]\n> | git version 1.8.3.1\n> `----\n> \n> > I just tried this and get the error\n> > \"fatal: Empty path component found in input\", \n> \n> I get that too.\n> \n> > which seems to be from commit 178e1de (fast-import: don't allow 'ls'\n> > of path with empty components, 2012-03-09), which is included in Git\n> > 1.7.9.5.\n> \n> Yes, that's at least part of the issue. I notice git-fast-import\n> rejects the root path \"\" for other commands, e.g. when used as the\n> source of a filecopy we get the same issue. I also note that the docs\n> don't make it clear that quoting the path is mandatory if it might turn\n> out to be empty.\n\nInteresting. There are two places that can produce this error message,\ntree_content_get and tree_content_set, but I wonder if this means that\ntree_content_get should not be doing this check. The two places that\ncall it are:\n\n1) \"parse_ls\" as discussed here\n2) \"file_change_cr\" which deals with file copy and rename.\n\nMy patch in the previous message only changes the behaviour for the\nparse_ls case, but it seems that you have a valid use case for removing\nthis check in the file_change_cr case as well.\n\n> I also note that the docs\n> don't make it clear that quoting the path is mandatory if it might turn\n> out to be empty.\n\nThat's not quite the case. It looks to me like quoting the path is\nmandatory if no \"\" is given, and indeed the documentation says:\n\n Reading from the active commit\n This form can only be used in the middle of a commit. The path\n names a directory entry within fast-import’s active commit. The\n path must be quoted in this case.\n\n 'ls' SP LF\n\n> > It seems to be slightly more complicated than that though, because after\n> > allowing empty trees I get the \"missing\" message for the root tree.\n> \n> Yeah, I've tried to patch Git to solve this but ran into that problem\n> and gave up.\n> \n> > This seems to be because its mode is 0 and not S_IFDIR.\n> \n> Aha.\n> \n> > With the patch below, things are working as I expect \n> \n> Awesome; works for me, too!\n> \n> > but I don't understand why the mode of the root is not set correctly\n> > at this point. Perhaps someone more familiar with fast-import will\n> > have some insight...\n> \n> Yeah... there's no bug tracker for Git, right? So if nobody pays\n> attention to this thread, the problem will persist?\n\nYes, but I don't see that happening particularly often. In the worst\ncase issues are normally documented by a failing test case.\n\nIn this case, I think I do now understand why the mode is 0: in parse_ls\na new tree object is created and the SHA1 of the original is copied in\nbut the mode is left blank; clearly this should be set to S_IFDIR when\nthe SHA1 is non-null.\n\nI think the patch I now have is correct (and addresses the \"copy from\nroot\" scenario), but I need to spend some time understanding t9300 so\nthat I can add suitable test cases.\n\n-- >8 --\ndiff --git a/fast-import.c b/fast-import.c\nindex 23f625f..e2c9d50 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1629,7 +1629,8 @@ del_entry:\n static int tree_content_get(\n \tstruct tree_entry *root,\n \tconst char *p,\n-\tstruct tree_entry *leaf)\n+\tstruct tree_entry *leaf,\n+\tint allow_root)\n {\n \tstruct tree_content *t;\n \tconst char *slash1;\n@@ -1641,31 +1642,39 @@ static int tree_content_get(\n \t\tn = slash1 - p;\n \telse\n \t\tn = strlen(p);\n-\tif (!n)\n+\tif (!n && !allow_root)\n \t\tdie(\"Empty path component found in input\");\n \n \tif (!root->tree)\n \t\tload_tree(root);\n+\n+\tif (!n) {\n+\t\te = root;\n+\t\tgoto found_entry;\n+\t}\n+\n \tt = root->tree;\n \tfor (i = 0; i < t->entry_count; i++) {\n \t\te = t->entries[i];\n \t\tif (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {\n-\t\t\tif (!slash1) {\n-\t\t\t\tmemcpy(leaf, e, sizeof(*leaf));\n-\t\t\t\tif (e->tree && is_null_sha1(e->versions[1].sha1))\n-\t\t\t\t\tleaf->tree = dup_tree_content(e->tree);\n-\t\t\t\telse\n-\t\t\t\t\tleaf->tree = NULL;\n-\t\t\t\treturn 1;\n-\t\t\t}\n+\t\t\tif (!slash1)\n+\t\t\t\tgoto found_entry;\n \t\t\tif (!S_ISDIR(e->versions[1].mode))\n \t\t\t\treturn 0;\n \t\t\tif (!e->tree)\n \t\t\t\tload_tree(e);\n-\t\t\treturn tree_content_get(e, slash1 + 1, leaf);\n+\t\t\treturn tree_content_get(e, slash1 + 1, leaf, 0);\n \t\t}\n \t}\n \treturn 0;\n+\n+found_entry:\n+\tmemcpy(leaf, e, sizeof(*leaf));\n+\tif (e->tree && is_null_sha1(e->versions[1].sha1))\n+\t\tleaf->tree = dup_tree_content(e->tree);\n+\telse\n+\t\tleaf->tree = NULL;\n+\treturn 1;\n }\n \n static int update_branch(struct branch *b)\n@@ -2415,7 +2424,7 @@ static void file_change_cr(struct branch *b, int rename)\n \tif (rename)\n \t\ttree_content_remove(&b->branch_tree, s, &leaf);\n \telse\n-\t\ttree_content_get(&b->branch_tree, s, &leaf);\n+\t\ttree_content_get(&b->branch_tree, s, &leaf, 1);\n \tif (!leaf.versions[1].mode)\n \t\tdie(\"Path %s not in branch\", s);\n \tif (!*d) {\t/* C \"path/to/subdir\" \"\" */\n@@ -3051,6 +3060,8 @@ static void parse_ls(struct branch *b)\n \t\tstruct object_entry *e = parse_treeish_dataref(&p);\n \t\troot = new_tree_entry();\n \t\thashcpy(root->versions[1].sha1, e->idx.sha1);\n+\t\tif (!is_null_sha1(root->versions[1].sha1))\n+\t\t\troot->versions[1].mode = S_IFDIR;\n \t\tload_tree(root);\n \t\tif (*p++ != ' ')\n \t\t\tdie(\"Missing space after tree-ish: %s\", command_buf.buf);\n@@ -3065,7 +3076,7 @@ static void parse_ls(struct branch *b)\n \t\t\tdie(\"Garbage after path in: %s\", command_buf.buf);\n \t\tp = uq.buf;\n \t}\n-\ttree_content_get(root, p, &leaf);\n+\ttree_content_get(root, p, &leaf, 1);\n \t/*\n \t * A directory in preparation would have a sha1 of zero\n \t * until it is saved. Save, for simplicity.\n"},{"id":"221719","messageId":"m2ppvc29le.fsf@cube.gateway.2wire.net","threadId":"34240","inReplyTo":"20130623110933.GG4676@serenity.lan","subject":"Re: fast-import bug?","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2013-06-23T14:19:25Z","receivedAt":"2013-06-23T14:19:25Z","isPatch":false,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"\non Sun Jun 23 2013, John Keeping wrote:\n\n> On Sat, Jun 22, 2013 at 07:16:48PM -0700, Dave Abrahams wrote:\n>> I also note that the docs\n>> don't make it clear that quoting the path is mandatory if it might turn\n>> out to be empty.\n>\n> That's not quite the case. It looks to me like quoting the path is\n> mandatory if no \"\" is given, and indeed the documentation says:\n>\n> Reading from the active commit\n> This form can only be used in the middle of a commit. The path\n> names a directory entry within fast-import’s active commit. The\n> path must be quoted in this case.\n>\n> 'ls' SP LF\n\nOops; good eye.\n\n>> > It seems to be slightly more complicated than that though, because after\n>> > allowing empty trees I get the \"missing\" message for the root tree.\n>> \n>> Yeah, I've tried to patch Git to solve this but ran into that problem\n>> and gave up.\n>> \n>> > This seems to be because its mode is 0 and not S_IFDIR.\n>> \n>> Aha.\n>> \n>> > With the patch below, things are working as I expect \n>> \n>> Awesome; works for me, too!\n>> \n>> > but I don't understand why the mode of the root is not set correctly\n>> > at this point. Perhaps someone more familiar with fast-import will\n>> > have some insight...\n>> \n>> Yeah... there's no bug tracker for Git, right? So if nobody pays\n>> attention to this thread, the problem will persist?\n>\n> Yes, but I don't see that happening particularly often. In the worst\n> case issues are normally documented by a failing test case.\n\nThe reason I ask is because from scouring this list it looks like\nthere's a history of people having issues with this, and someone\nintended to get to a fix in sometime around 1.17.10, but nothing ever\nhappened.\n\n> In this case, I think I do now understand why the mode is 0: in\n> parse_ls a new tree object is created and the SHA1 of the original is\n> copied in but the mode is left blank; clearly this should be set to\n> S_IFDIR when the SHA1 is non-null.\n>\n> I think the patch I now have is correct (and addresses the \"copy from\n> root\" scenario), but I need to spend some time understanding t9300 so\n> that I can add suitable test cases.\n\nt9300? \n\nThanks; I'll try this one too.\n\n> -- >8 --\n> diff --git a/fast-import.c b/fast-import.c\n> index 23f625f..e2c9d50 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1629,7 +1629,8 @@ del_entry:\n> static int tree_content_get(\n> \tstruct tree_entry *root,\n> \tconst char *p,\n> -\tstruct tree_entry *leaf)\n> +\tstruct tree_entry *leaf,\n> +\tint allow_root)\n> {\n> \tstruct tree_content *t;\n> \tconst char *slash1;\n> @@ -1641,31 +1642,39 @@ static int tree_content_get(\n> \t\tn = slash1 - p;\n> \telse\n> \t\tn = strlen(p);\n> -\tif (!n)\n> +\tif (!n && !allow_root)\n> \t\tdie(\"Empty path component found in input\");\n>\n> \tif (!root->tree)\n> \t\tload_tree(root);\n> +\n> +\tif (!n) {\n> +\t\te = root;\n> +\t\tgoto found_entry;\n> +\t}\n> +\n> \tt = root->tree;\n> \tfor (i = 0; i < t->entry_count; i++) {\n> \t\te = t->entries[i];\n> \t\tif (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {\n> -\t\t\tif (!slash1) {\n> -\t\t\t\tmemcpy(leaf, e, sizeof(*leaf));\n> -\t\t\t\tif (e->tree && is_null_sha1(e->versions[1].sha1))\n> -\t\t\t\t\tleaf->tree = dup_tree_content(e->tree);\n> -\t\t\t\telse\n> -\t\t\t\t\tleaf->tree = NULL;\n> -\t\t\t\treturn 1;\n> -\t\t\t}\n> +\t\t\tif (!slash1)\n> +\t\t\t\tgoto found_entry;\n> \t\t\tif (!S_ISDIR(e->versions[1].mode))\n> \t\t\t\treturn 0;\n> \t\t\tif (!e->tree)\n> \t\t\t\tload_tree(e);\n> -\t\t\treturn tree_content_get(e, slash1 + 1, leaf);\n> +\t\t\treturn tree_content_get(e, slash1 + 1, leaf, 0);\n> \t\t}\n> \t}\n> \treturn 0;\n> +\n> +found_entry:\n> +\tmemcpy(leaf, e, sizeof(*leaf));\n> +\tif (e->tree && is_null_sha1(e->versions[1].sha1))\n> +\t\tleaf->tree = dup_tree_content(e->tree);\n> +\telse\n> +\t\tleaf->tree = NULL;\n> +\treturn 1;\n> }\n>\n> static int update_branch(struct branch *b)\n> @@ -2415,7 +2424,7 @@ static void file_change_cr(struct branch *b, int rename)\n> \tif (rename)\n> \t\ttree_content_remove(&b->branch_tree, s, &leaf);\n> \telse\n> -\t\ttree_content_get(&b->branch_tree, s, &leaf);\n> +\t\ttree_content_get(&b->branch_tree, s, &leaf, 1);\n> \tif (!leaf.versions[1].mode)\n> \t\tdie(\"Path %s not in branch\", s);\n> \tif (!*d) {\t/* C \"path/to/subdir\" \"\" */\n> @@ -3051,6 +3060,8 @@ static void parse_ls(struct branch *b)\n> \t\tstruct object_entry *e = parse_treeish_dataref(&p);\n> \t\troot = new_tree_entry();\n> \t\thashcpy(root->versions[1].sha1, e->idx.sha1);\n> +\t\tif (!is_null_sha1(root->versions[1].sha1))\n> +\t\t\troot->versions[1].mode = S_IFDIR;\n> \t\tload_tree(root);\n> \t\tif (*p++ != ' ')\n> \t\t\tdie(\"Missing space after tree-ish: %s\", command_buf.buf);\n> @@ -3065,7 +3076,7 @@ static void parse_ls(struct branch *b)\n> \t\t\tdie(\"Garbage after path in: %s\", command_buf.buf);\n> \t\tp = uq.buf;\n> \t}\n> -\ttree_content_get(root, p, &leaf);\n> +\ttree_content_get(root, p, &leaf, 1);\n> \t/*\n> \t * A directory in preparation would have a sha1 of zero\n> \t * until it is saved. Save, for simplicity.\n\n-- \nDave Abrahams\n"},{"id":"221721","messageId":"20130623145503.GH4676@serenity.lan","threadId":"34240","inReplyTo":"m2ppvc29le.fsf@cube.gateway.2wire.net","subject":"Re: fast-import bug?","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T14:55:03Z","receivedAt":"2013-06-23T14:55:03Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Jun 23, 2013 at 07:19:25AM -0700, Dave Abrahams wrote:\n> on Sun Jun 23 2013, John Keeping wrote:\n> > In this case, I think I do now understand why the mode is 0: in\n> > parse_ls a new tree object is created and the SHA1 of the original is\n> > copied in but the mode is left blank; clearly this should be set to\n> > S_IFDIR when the SHA1 is non-null.\n> >\n> > I think the patch I now have is correct (and addresses the \"copy from\n> > root\" scenario), but I need to spend some time understanding t9300 so\n> > that I can add suitable test cases.\n> \n> t9300? \n\nt/t9300-fast-import.sh in Git's source tree - it's where the tests for\nfast-import live.\n\n> Thanks; I'll try this one too.\n\nThanks. I now have a patch series incorporating this which also adds a\nfew tests for handling of empty paths. I'm sending it out in the next\nfew minutes.\n"}]}