{"thread":{"id":"28116","subject":"[PATCH 1/2] fast-import: be saner with temporary trees","startedAt":"2011-08-16T10:08:55Z","lastAt":"2012-03-08T19:12:00Z","messageCount":4,"participants":["Dmitry Ivankov","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"173590","messageId":"1313489337-2523-1-git-send-email-divanorama@gmail.com","threadId":"28116","inReplyTo":null,"subject":"[PATCH 0/2] fast-import: allow more operations on root directory","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-16T10:08:55Z","receivedAt":"2011-08-16T10:08:55Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"[1/2] has been already reviewed in [1] where it didn't seem to have any effect,\nbut for this series it does, for 'ls \"\"' command.\nSo the change is about allowing to \"ls\" root directory and to copy or rename \nit to some subdirectory. The documentation doesn't deny these, but they don't\nwork. Make them work.\n\nMore details are in [2/2].\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/178007/focus=178009\n\nDmitry Ivankov (2):\n  fast-import: be saner with temporary trees\n  fast-import: allow top directory as an argument for some commands\n\n fast-import.c          |   78 +++++++++++++++++++++++++++++++++++++++++------\n t/t9300-fast-import.sh |   58 +++++++++++++++++++++++++++++++++++\n 2 files changed, 126 insertions(+), 10 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"173589","messageId":"1313489337-2523-2-git-send-email-divanorama@gmail.com","threadId":"28116","inReplyTo":"1313489337-2523-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 1/2] fast-import: be saner with temporary trees","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-16T10:08:56Z","receivedAt":"2011-08-16T10:08:56Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"new_tree_entry/release_tree_entry manage a stack of tree_entry structs\nto use as temporaries. Initializing them is the responsibility of the\ncaller, both after allocation with xmalloc() when existing temporaries\nare exhausted and after used entries are pushed with release_tree_entry.\n\nparse_ls doesn't set root->versions[0] fields, making root an invalid\nentry. The problem could arise if store_tree(root) is called. parse_ls\ncalls store_tree on the node corresponding to the path it is called on.\n\nDo initialize entry->versions[0]. As of now, ls command can not list\nthe topmost tree so this change is just to avoid surprises in case\nthings will change around ls or tree_content_get.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n fast-import.c |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 7cc2262..3a0aaad 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2951,6 +2951,9 @@ static void parse_ls(struct branch *b)\n \t} else {\n \t\tstruct object_entry *e = parse_treeish_dataref(&p);\n \t\troot = new_tree_entry();\n+\t\thashclr(root->versions[0].sha1);\n+\t\troot->versions[0].mode = 0;\n+\t\troot->versions[1].mode = S_IFDIR;\n \t\thashcpy(root->versions[1].sha1, e->idx.sha1);\n \t\tload_tree(root);\n \t\tif (*p++ != ' ')\n-- \n1.7.3.4\n"},{"id":"173591","messageId":"1313489337-2523-3-git-send-email-divanorama@gmail.com","threadId":"28116","inReplyTo":"1313489337-2523-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 2/2] fast-import: allow top directory as an argument for some commands","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-16T10:08:57Z","receivedAt":"2011-08-16T10:08:57Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"fast-import allows to operate on the toplevel directory. This includes\nsetting the value by sha1/mark, copying/renaming some subdirectory to\na toplevel one.\n\nBut it's not possible to 'ls' it or copy/rename it so somewhere, or\ndelete it with 'D' command.\n\nFor ls it's quite reasonable input so just handle it. The special case\nis empty directory, for other empty directories we report 'missing' so\nstick with this behaviour with toplevel too.\n\nFor copy/rename it looks like a normal operation too so handle it too.\nBut if it's empty, do nothing - there is nothing to copy/rename and the\ndestination is nonexistent because toplevel is empty.\n\nFor 'D \"\"' we already have deleteall command so probably it's not worth\nadding one more way to do the same thing.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\n fast-import.c          |   75 +++++++++++++++++++++++++++++++++++++++++------\n t/t9300-fast-import.sh |   58 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 123 insertions(+), 10 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 3a0aaad..8643e23 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1552,6 +1552,31 @@ static int tree_content_set(\n \treturn 1;\n }\n \n+static int tree_content_clear(\n+\tstruct tree_entry *root,\n+\tstruct tree_entry *backup_leaf)\n+{\n+\tif (backup_leaf)\n+\t\tmemcpy(backup_leaf, root, sizeof(*backup_leaf));\n+\telse if (root->tree)\n+\t\trelease_tree_content_recursive(root->tree);\n+\troot->tree = NULL;\n+\thashclr(root->versions[1].sha1);\n+\treturn 1;\n+}\n+\n+static int is_empty_tree(struct tree_entry *root)\n+{\n+\tint n;\n+\tif (!root->tree)\n+\t\tload_tree(root);\n+\tfor (n = 0; n < root->tree->entry_count; n++) {\n+\t\tif (root->tree->entries[n]->versions[1].mode)\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n static int tree_content_remove(\n \tstruct tree_entry *root,\n \tconst char *p,\n@@ -1587,11 +1612,9 @@ static int tree_content_remove(\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\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-\t\t\t\t\t\treturn 1;\n-\t\t\t\t\t}\n+\t\t\t\tif (!is_empty_tree(e)) {\n+\t\t\t\t\thashclr(root->versions[1].sha1);\n+\t\t\t\t\treturn 1;\n \t\t\t\t}\n \t\t\t\tbackup_leaf = NULL;\n \t\t\t\tgoto del_entry;\n@@ -1613,6 +1636,19 @@ del_entry:\n \treturn 1;\n }\n \n+static int tree_content_copy_root(\n+\tstruct tree_entry *root,\n+\tstruct tree_entry *leaf\n+)\n+{\n+\tmemcpy(leaf, root, sizeof(*leaf));\n+\tif (root->tree && is_null_sha1(root->versions[1].sha1))\n+\t\tleaf->tree = dup_tree_content(root->tree);\n+\telse\n+\t\tleaf->tree = NULL;\n+\treturn 1;\n+}\n+\n static int tree_content_get(\n \tstruct tree_entry *root,\n \tconst char *p,\n@@ -2329,10 +2365,17 @@ static void file_change_cr(struct branch *b, int rename)\n \t}\n \n \tmemset(&leaf, 0, sizeof(leaf));\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+\tif (rename) {\n+\t\tif (!*s)\n+\t\t\ttree_content_clear(&b->branch_tree, &leaf);\n+\t\telse\n+\t\t\ttree_content_remove(&b->branch_tree, s, &leaf);\n+\t} else {\n+\t\tif (!*s)\n+\t\t\ttree_content_copy_root(&b->branch_tree, &leaf);\n+\t\telse\n+\t\t\ttree_content_get(&b->branch_tree, s, &leaf);\n+\t}\n \tif (!leaf.versions[1].mode)\n \t\tdie(\"Path %s not in branch\", s);\n \tif (!*d) {\t/* C \"path/to/subdir\" \"\" */\n@@ -2342,6 +2385,14 @@ static void file_change_cr(struct branch *b, int rename)\n \t\t\tleaf.tree);\n \t\treturn;\n \t}\n+\t/*\n+\t * Git does not track empty, non-toplevel directories.\n+\t * At this point destination is non-toplevel, so if source is\n+\t * toplevel and empty, leave it as is all empty.\n+\t */\n+\tif (!*s && is_empty_tree(&leaf)) {\n+\t\treturn;\n+\t}\n \ttree_content_set(&b->branch_tree, d,\n \t\tleaf.versions[1].sha1,\n \t\tleaf.versions[1].mode,\n@@ -2969,7 +3020,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\tif (!is_empty_tree(root))\n+\t\t\ttree_content_copy_root(root, &leaf);\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.\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex f256475..0e46cf0 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -978,6 +978,64 @@ test_expect_success PIPE 'N: empty directory reads as missing' '\n '\n \n test_expect_success \\\n+\t'N: ls root directory' \\\n+\t'cat >expect <<-\\EOF &&\n+\tmissing\n+\t040000\n+\tEOF\n+\tcat >input <<-EOF &&\n+\tcommit refs/heads/ls-root\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tOn empty tree: ls add file ls.\n+\tCOMMIT\n+\tls \"\"\n+\tM 644 inline file1\n+\tdata 0\n+\tls \"\"\n+\tEOF\n+\tgit fast-import --quiet <input >tmp &&\n+\tcat tmp | cut -d \" \" -f 1 >actual &&\n+\ttest_cmp expect actual'\n+\n+test_expect_success \\\n+\t'N: move root directory' \\\n+\t'echo \"root/a/b\" >expect &&\n+\tcat >input <<-EOF &&\n+\tcommit refs/heads/move-root\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tAdd stuff and move it.\n+\tCOMMIT\n+\tM 644 inline a/b\n+\tdata 0\n+\tR \"\" root\n+\tEOF\n+\tgit fast-import <input &&\n+\tgit ls-tree -r --name-only refs/heads/move-root >actual &&\n+\ttest_cmp expect actual'\n+\n+test_expect_success \\\n+\t'N: copy root directory' \\\n+\t'cat <<-\\EOF >expect &&\n+\ta/b\n+\ta/root/a/b\n+\tEOF\n+\tcat >input <<-EOF &&\n+\tcommit refs/heads/copy-root\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tAdd stuff and move it.\n+\tCOMMIT\n+\tM 644 inline a/b\n+\tdata 0\n+\tC \"\" a/root\n+\tEOF\n+\tgit fast-import <input &&\n+\tgit ls-tree -r --name-only refs/heads/copy-root >actual &&\n+\ttest_cmp expect actual'\n+\n+test_expect_success \\\n \t'N: copy root directory by tree hash' \\\n \t'cat >expect <<-\\EOF &&\n \t:100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D\tfile3/newf\n-- \n1.7.3.4\n"},{"id":"186479","messageId":"20120308191200.GA17903@burratino","threadId":"28116","inReplyTo":"1313489337-2523-2-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH 1/2] fast-import: be saner with temporary trees","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T19:12:00Z","receivedAt":"2012-03-08T19:12:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDmitry Ivankov wrote:\n\n> new_tree_entry/release_tree_entry manage a stack of tree_entry structs\n> to use as temporaries. Initializing them is the responsibility of the\n> caller,\n[...]\n> Do initialize entry->versions[0]. As of now, ls command can not list\n> the topmost tree so this change is just to avoid surprises in case\n> things will change around ls or tree_content_get.\n>\n> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>\n> Acked-by: Jonathan Nieder <jrnieder@gmail.com>\n[...]\n> +++ b/fast-import.c\n> @@ -2951,6 +2951,9 @@ static void parse_ls(struct branch *b)\n>  \t} else {\n>  \t\tstruct object_entry *e = parse_treeish_dataref(&p);\n>  \t\troot = new_tree_entry();\n> +\t\thashclr(root->versions[0].sha1);\n> +\t\troot->versions[0].mode = 0;\n> +\t\troot->versions[1].mode = S_IFDIR;\n>  \t\thashcpy(root->versions[1].sha1, e->idx.sha1);\n\nI don't remember what this patch was about or why I acked it, which is\na bad sign.  (If the above explanation doesn't make me want to ack it\nagain, how is it going to help someone in the future understand the\nharm in reverting this patch if some other bug bisects to it?)\n\nI think part of the problem is that the explanation seems to be\nphrased in terms of implementation details (root->versions[0] fields,\ncalling store_tree, initializing entry->versions[0]) instead of the\nhigher-level logic that these implement.\n\nIt sounds (\"avoid surprises\") like this is meant to avoid scary\nbehavior from the programmer's perspective, so: what are the symptoms\nwithout this patch, and why is this a good way to eliminate them?\n\nOn the other hand, if we squash this in with David's [1], it starts\nto make perfect sense.  The explanation can say:\n\n\tThere is a pathological Subversion operation that svn-fe handles\n\tincorrectly due to an unexpected response from fast-import:\n\n\t  svn cp $SVN_ROOT $SVN_ROOT/subdirectory\n\n\tWhen the following command is sent to fast-import:\n\n\t 'ls' SP ':1' SP LF\n\n\tThe expected output is:\n\n\t '040000' SP 'tree' SP <dataref> HT LF\n\n\tThe actual output is:\n\n\t 'missing' SP LF\n\n\tThis is because tree_content_get() is called but expects a non-empty\n\tpath. Instead, copy the root entry and force the mode to S_IFDIR.\n\n\tIn the 'ls <dataref> \"\"' case, this requires some care, because\n\tparse_ls currently uses an uninitialized temporary tree entry\n\tfrom the new_tree_entry/release_tree_entry stack in a quick and\n\tdirty way without initializing its fields well enough for\n\tfunctions like store_tree that might calculate its tree name.\n\nSensible?  Alternatively, we could rush in David's [1] with the\ntweak below to dodge the 'ls <dataref> \"\"' case, and include your fix\nas part of a patch that just addresses that case.\n\nThanks very much for the patch and the reminder.\n\nSincerely,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/192517/focus=192520\n---\n fast-import.c          |    2 +-\n t/t9300-fast-import.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 66 insertions(+), 2 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 8dbfd4cc..2748ced2 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3019,7 +3019,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-\tif (*p) {\n+\tif (*p || root != &b->branch_tree) {\n \t\ttree_content_get(root, p, &leaf);\n \t} else {\n \t\tleaf = *root;\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 2558a2ed..2f411fbb 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1400,8 +1400,72 @@ test_expect_success \\\n \t test_cmp expect.qux actual.qux &&\n \t test_cmp expect.qux actual.quux'\n \n+test_expect_success 'N: root of unborn branch reads as present and empty' '\n+\tempty_tree=$(git mktree </dev/null) &&\n+\techo \"040000 tree $empty_tree\t\" >expect &&\n+\tgit fast-import --cat-blob-fd=3 3>actual <<-EOF &&\n+\tcommit refs/heads/N-empty\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tread empty root directory via ls\n+\tCOMMIT\n+\n+\tls \"\"\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'N: empty root reads as present and empty' '\n+\tempty_tree=$(git mktree </dev/null) &&\n+\techo \"040000 tree $empty_tree\t\" >expect &&\n+\techo empty >msg &&\n+\tcmit=$(git commit-tree \"$empty_tree\" -p refs/heads/branch^0 <msg) &&\n+\tgit fast-import --cat-blob-fd=3 3>actual <<-EOF &&\n+\tcommit refs/heads/N-empty-existing\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tread empty root directory via ls\n+\tCOMMIT\n+\n+\tls \"\"\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'N: \"ls\" command can read subdir of named tree' '\n+\tbranch_cmit=$(git rev-parse --verify refs/heads/branch^0) &&\n+\tsubdir_tree=$(git rev-parse $branch_cmit:newdir) &&\n+\techo \"040000 tree $subdir_tree\tnewdir\" >expect &&\n+\tgit fast-import --cat-blob-fd=3 3>actual <<-EOF &&\n+\tcommit refs/heads/N-subdir-of-named-tree\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tread from commit with ls\n+\tCOMMIT\n+\n+\tls $branch_cmit \"newdir\"\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'N: \"ls\" command can read root of named commit' '\n+\tbranch_cmit=$(git rev-parse --verify refs/heads/branch^0) &&\n+\tbranch_tree=$(git rev-parse --verify $branch_cmit:) &&\n+\techo \"040000 tree $branch_tree\t\" >expect &&\n+\tgit fast-import --cat-blob-fd=3 3>actual <<-EOF &&\n+\tcommit refs/heads/N-root-of-named-tree\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tread root of commit with ls\n+\tCOMMIT\n+\n+\tls $branch_cmit \"\"\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success PIPE 'N: read and copy root' '\n-\tcat >expect <<-\\EOF\n+\tcat >expect <<-\\EOF &&\n \t:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100\tfile2/newf\tfile3/file2/newf\n \t:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100\tfile2/oldf\tfile3/file2/oldf\n \t:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100\tfile4\tfile3/file4\n-- \n1.7.9.2\n"}]}