{"thread":{"id":"34257","subject":"[PATCH 0/4] fast-import: handle empty paths better","startedAt":"2013-06-23T14:58:18Z","lastAt":"2013-07-01T21:57:32Z","messageCount":6,"participants":["John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"221722","messageId":"cover.1371999369.git.john@keeping.me.uk","threadId":"34257","inReplyTo":null,"subject":"[PATCH 0/4] fast-import: handle empty paths better","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T14:58:18Z","receivedAt":"2013-06-23T14:58:18Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This series addressed Dave Abraham's recent bug report [1] about using\nfast-import's \"ls\" command with an empty path. I also found a couple of\nother places that do not handle the empty path when it can reasonably be\ninterpreted as meaning the root tree object, which are also fixed here.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/228586\n\nJohn Keeping (4):\n t9300: document fast-import empty path issues\n fast-import: set valid mode on root tree in \"ls\"\n fast-import: allow ls or filecopy of the root tree\n fast-import: allow moving the root tree\n\n fast-import.c | 58 ++++++++++++++++++++++++++++----------------\n t/t9300-fast-import.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 103 insertions(+), 20 deletions(-)\n\n-- \n1.8.3.1.676.gaae6535\n"},{"id":"221723","messageId":"79532d59f0480a07ceb7c11e31bcd793c12122d6.1371999369.git.john@keeping.me.uk","threadId":"34257","inReplyTo":"cover.1371999369.git.john@keeping.me.uk","subject":"[PATCH 1/4] t9300: document fast-import empty path issues","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T14:58:19Z","receivedAt":"2013-06-23T14:58:19Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When given an empty path, fast-import sometimes reports \"missing\"\ninstead of using the root tree object. On top of this, for \"ls\" and\nfile copy (but not move) it dies with \"Empty path component found in\ninput\".\n\nDocument this behaviour with failing test cases.\n\nReported-by: Dave Abrahams \nSigned-off-by: John Keeping \n---\n t/t9300-fast-import.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 65 insertions(+)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex ac6f3b6..f4b9355 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1031,6 +1031,32 @@ test_expect_success \\\n \t git diff-tree -M -r M3^ M3 >actual &&\n \t compare_diff_raw expect actual'\n \n+cat >input < $GIT_COMMITTER_DATE\n+data <expect <actual &&\n+\t cat actual &&\n+\t compare_diff_raw expect actual'\n+\n ###\n ### series N\n ###\n@@ -1227,6 +1253,29 @@ test_expect_success \\\n \t git diff-tree -C --find-copies-harder -r N4 N6 >actual &&\n \t compare_diff_raw expect actual'\n \n+test_expect_failure \\\n+\t'N: copy root by path' \\\n+\t'cat >expect <<-\\EOF &&\n+\t:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100\tfile2/newf\toldroot/file2/newf\n+\t:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100\tfile2/oldf\toldroot/file2/oldf\n+\t:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100\tfile4\toldroot/file4\n+\t:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100\tnewdir/exec.sh\toldroot/newdir/exec.sh\n+\t:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100\tnewdir/interesting\toldroot/newdir/interesting\n+\tEOF\n+\t cat >input <<-INPUT_END &&\n+\tcommit refs/heads/N-copy-root-path\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <actual &&\n+\t compare_diff_raw expect actual'\n+\n test_expect_success \\\n \t'N: delete directory by copying' \\\n \t'cat >expect <<-\\EOF &&\n@@ -2934,4 +2983,20 @@ test_expect_success 'S: ls with garbage after sha1 must fail' '\n \ttest_i18ngrep \"space after tree-ish\" err\n '\n \n+###\n+### series T (ls)\n+###\n+# Setup is carried over from series S.\n+\n+test_expect_failure 'T: ls root tree' '\n+\tsed -e \"s/Z\\$//\" >expect <<-EOF &&\n+\t040000 tree $(git rev-parse S^{tree})\tZ\n+\tEOF\n+\tsha1=$(git rev-parse --verify S) &&\n+\tgit fast-import --import-marks=marks <<-EOF >actual &&\n+\tls $sha1 \"\"\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.3.1.676.gaae6535\n"},{"id":"221724","messageId":"093e9ce49bf92d81c3a7a11d91292be675eaf6fa.1371999369.git.john@keeping.me.uk","threadId":"34257","inReplyTo":"cover.1371999369.git.john@keeping.me.uk","subject":"[PATCH 2/4] fast-import: set valid mode on root tree in \"ls\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T14:58:20Z","receivedAt":"2013-06-23T14:58:20Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This prevents a failure later when we lift the restriction on ls with\nthe empty path.\n\nSigned-off-by: John Keeping \n---\n fast-import.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 23f625f..bdbadea 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3051,6 +3051,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-- \n1.8.3.1.676.gaae6535\n"},{"id":"221725","messageId":"acdb5e89bd99ad1374e857624181d8323f72cef8.1371999369.git.john@keeping.me.uk","threadId":"34257","inReplyTo":"cover.1371999369.git.john@keeping.me.uk","subject":"[PATCH 3/4] fast-import: allow ls or filecopy of the root tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T14:58:21Z","receivedAt":"2013-06-23T14:58:21Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Commit 178e1de (fast-import: don't allow 'ls' of path with empty\ncomponents, 2012-03-09) restricted paths which:\n\n . contain an empty directory component (e.g. foo//bar is invalid),\n . end with a directory separator (e.g. foo/ is invalid),\n . start with a directory separator (e.g. /foo is invalid).\n\nHowever, the implementation also caught the empty path, which should\nrepresent the root tree. Relax this restriction so that the empty path\nis explicitly allowed and refers to the root tree.\n\nReported-by: Dave Abrahams \nSigned-off-by: John Keeping \n---\n fast-import.c | 35 ++++++++++++++++++++++-------------\n t/t9300-fast-import.sh | 4 ++--\n 2 files changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex bdbadea..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@@ -3067,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.\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex f4b9355..04385a7 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1253,7 +1253,7 @@ test_expect_success \\\n \t git diff-tree -C --find-copies-harder -r N4 N6 >actual &&\n \t compare_diff_raw expect actual'\n \n-test_expect_failure \\\n+test_expect_success \\\n \t'N: copy root by path' \\\n \t'cat >expect <<-\\EOF &&\n \t:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100\tfile2/newf\toldroot/file2/newf\n@@ -2988,7 +2988,7 @@ test_expect_success 'S: ls with garbage after sha1 must fail' '\n ###\n # Setup is carried over from series S.\n \n-test_expect_failure 'T: ls root tree' '\n+test_expect_success 'T: ls root tree' '\n \tsed -e \"s/Z\\$//\" >expect <<-EOF &&\n \t040000 tree $(git rev-parse S^{tree})\tZ\n \tEOF\n-- \n1.8.3.1.676.gaae6535\n"},{"id":"221726","messageId":"08c4486c228eff5754b95708d8f5d56e95620278.1371999369.git.john@keeping.me.uk","threadId":"34257","inReplyTo":"cover.1371999369.git.john@keeping.me.uk","subject":"[PATCH 4/4] fast-import: allow moving the root tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-23T14:58:22Z","receivedAt":"2013-06-23T14:58:22Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Because fast-import.c::tree_content_remove does not check for the empty\npath, it is not possible to move the root tree to a subdirectory.\nInstead the error \"Path not in branch\" is produced (note the double\nspace where the empty path has been inserted).\n\nFix this by explicitly checking for the empty path and handling it.\n\nSigned-off-by: John Keeping \n---\n fast-import.c | 21 ++++++++++++++-------\n t/t9300-fast-import.sh | 2 +-\n 2 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex e2c9d50..21db3fc 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1568,7 +1568,8 @@ static int tree_content_set(\n static int tree_content_remove(\n \tstruct tree_entry *root,\n \tconst char *p,\n-\tstruct tree_entry *backup_leaf)\n+\tstruct tree_entry *backup_leaf,\n+\tint allow_root)\n {\n \tstruct tree_content *t;\n \tconst char *slash1;\n@@ -1583,6 +1584,12 @@ static int tree_content_remove(\n \n \tif (!root->tree)\n \t\tload_tree(root);\n+\n+\tif (!*p && allow_root) {\n+\t\te = root;\n+\t\tgoto del_entry;\n+\t}\n+\n \tt = root->tree;\n \tfor (i = 0; i < t->entry_count; i++) {\n \t\te = t->entries[i];\n@@ -1599,7 +1606,7 @@ static int tree_content_remove(\n \t\t\t\tgoto del_entry;\n \t\t\tif (!e->tree)\n \t\t\t\tload_tree(e);\n-\t\t\tif (tree_content_remove(e, slash1 + 1, backup_leaf)) {\n+\t\t\tif (tree_content_remove(e, slash1 + 1, backup_leaf, 0)) {\n \t\t\t\tfor (n = 0; n < e->tree->entry_count; n++) {\n \t\t\t\t\tif (e->tree->entries[n]->versions[1].mode) {\n \t\t\t\t\t\thashclr(root->versions[1].sha1);\n@@ -2188,7 +2195,7 @@ static uintmax_t do_change_note_fanout(\n \t\t\t}\n \n \t\t\t/* Rename fullpath to realpath */\n-\t\t\tif (!tree_content_remove(orig_root, fullpath, &leaf))\n+\t\t\tif (!tree_content_remove(orig_root, fullpath, &leaf, 0))\n \t\t\t\tdie(\"Failed to remove path %s\", fullpath);\n \t\t\ttree_content_set(orig_root, realpath,\n \t\t\t\tleaf.versions[1].sha1,\n@@ -2323,7 +2330,7 @@ static void file_change_m(struct branch *b)\n \n \t/* Git does not track empty, non-toplevel directories. */\n \tif (S_ISDIR(mode) && !memcmp(sha1, EMPTY_TREE_SHA1_BIN, 20) && *p) {\n-\t\ttree_content_remove(&b->branch_tree, p, NULL);\n+\t\ttree_content_remove(&b->branch_tree, p, NULL, 0);\n \t\treturn;\n \t}\n \n@@ -2384,7 +2391,7 @@ static void file_change_d(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_remove(&b->branch_tree, p, NULL);\n+\ttree_content_remove(&b->branch_tree, p, NULL, 1);\n }\n \n static void file_change_cr(struct branch *b, int rename)\n@@ -2422,7 +2429,7 @@ static void file_change_cr(struct branch *b, int rename)\n \n \tmemset(&leaf, 0, sizeof(leaf));\n \tif (rename)\n-\t\ttree_content_remove(&b->branch_tree, s, &leaf);\n+\t\ttree_content_remove(&b->branch_tree, s, &leaf, 1);\n \telse\n \t\ttree_content_get(&b->branch_tree, s, &leaf, 1);\n \tif (!leaf.versions[1].mode)\n@@ -2530,7 +2537,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n \t}\n \n \tconstruct_path_with_fanout(sha1_to_hex(commit_sha1), *old_fanout, path);\n-\tif (tree_content_remove(&b->branch_tree, path, NULL))\n+\tif (tree_content_remove(&b->branch_tree, path, NULL, 0))\n \t\tb->num_notes--;\n \n \tif (is_null_sha1(sha1))\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 04385a7..31a770d 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1050,7 +1050,7 @@ cat >expect <actual &&\n-- \n1.8.3.1.676.gaae6535\n"},{"id":"222311","messageId":"7v7ghaapzy.fsf@alter.siamese.dyndns.org","threadId":"34257","inReplyTo":"cover.1371999369.git.john@keeping.me.uk","subject":"Re: [PATCH 0/4] fast-import: handle empty paths better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-01T21:57:32Z","receivedAt":"2013-07-01T21:57:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> This series addressed Dave Abraham's recent bug report [1] about using\n> fast-import's \"ls\" command with an empty path. I also found a couple of\n> other places that do not handle the empty path when it can reasonably be\n> interpreted as meaning the root tree object, which are also fixed here.\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/228586\n>\n> John Keeping (4):\n> t9300: document fast-import empty path issues\n> fast-import: set valid mode on root tree in \"ls\"\n> fast-import: allow ls or filecopy of the root tree\n> fast-import: allow moving the root tree\n>\n> fast-import.c | 58 ++++++++++++++++++++++++++++----------------\n> t/t9300-fast-import.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 103 insertions(+), 20 deletions(-)\n\nPing? Reviews, please?\n\nNo need to hurry, but just to make sure this didn't disappear from\neverybody's radar.\n\nThanks.\n"}]}