{"thread":{"id":"29878","subject":"[BUG] fast-import: ls command on commit root returns missing (was: Bug in svn-fe: copying the root directory acts as if it's an empty directory)","startedAt":"2012-03-08T03:13:13Z","lastAt":"2013-06-21T16:33:19Z","messageCount":23,"participants":["David Barr","Jonathan Nieder","Sverre Rabbelier","Junio C Hamano","Dmitry Ivankov","Dave Abrahams"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"186371","messageId":"CAFfmPPMxcs0ySgnD7UfUS1yq=qaqfn1qCxdh1HYgFu6WPfpWQg@mail.gmail.com","threadId":"29878","inReplyTo":null,"subject":"[BUG] fast-import: ls command on commit root returns missing (was: Bug in svn-fe: copying the root directory acts as if it's an empty directory)","fromName":"David Barr","fromEmail":"davidbarr@google.com","sentAt":"2012-03-08T03:13:13Z","receivedAt":"2012-03-08T03:13:13Z","isPatch":false,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"On Thu, Mar 8, 2012 at 11:46 AM, David Barr <davidbarr@google.com> wrote:\n> Hi Andrew,\n>\n> On Thu, Mar 8, 2012 at 10:13 AM, Andrew Sayers\n> <andrew-git@pileofstuff.org> wrote:\n>> Here's a bug with svn-fe that I stumbled over while snorkelling through\n>> repo madness.  I've tested it with the version of svn-fe in git.git's\n>> master branch.\n>>\n>> Copying the root directory to a sub-directory (e.g. doing `svn cp .\n>> trunk` to standardise your layout) doesn't correctly initialise the new\n>> directory.\n>\n> This issue sounds very familiar, I wonder if there's an existing test\n> or pending patch for it? Maybe Dmitry or Jonathan can recall.\n\nI've stepped through the reproduction and the bug seems to arise when\nthe following command is sent to git-fast-import:\n\n  'ls' SP ':1' SP LF\n\nThe expected output in this example is:\n\n  '400000' SP 'tree' SP 'dd59323fe27c5647cb7ef15ce4637faae199c5f0' HT LF\n\nThe actual output is:\n\n  'missing' SP LF\n\n--\nDavid Barr\n"},{"id":"186374","messageId":"1331184656-98629-1-git-send-email-davidbarr@google.com","threadId":"29878","inReplyTo":"CAFfmPPMxcs0ySgnD7UfUS1yq=qaqfn1qCxdh1HYgFu6WPfpWQg@mail.gmail.com","subject":"[PATCH] fast-import: fix ls command with empty path","fromName":"David Barr","fromEmail":"davidbarr@google.com","sentAt":"2012-03-08T05:30:56Z","receivedAt":"2012-03-08T05:30:56Z","isPatch":true,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"There is a pathological Subversion operation that svn-fe handles\nincorrectly due to an unexpected response from fast-import:\n\n  svn cp $SVN_ROOT $SVN_ROOT/subdirectory\n\nWhen the following command is sent to fast-import:\n\n 'ls' SP ':1' SP LF\n\nThe expected output is:\n\n '040000' SP 'tree' SP <dataref> HT LF\n\nThe actual output is:\n\n 'missing' SP LF\n\nThis is because tree_content_get() is called but expects a non-empty\npath. Instead, copy the root entry and force the mode to S_IFDIR.\n\nReported-by: Andrew Sayers <andrew-git@pileofstuff.org>\nSigned-off-by: David Barr <davidbarr@google.com>\n---\n fast-import.c          |    7 ++++++-\n t/t9300-fast-import.sh |   31 +++++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex c1486ca..8dbfd4c 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3019,7 +3019,12 @@ 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\ttree_content_get(root, p, &leaf);\n+\t} else {\n+\t\tleaf = *root;\n+\t\tleaf.versions[1].mode = S_IFDIR;\n+\t}\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 438aaf6..2558a2e 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1400,6 +1400,37 @@ test_expect_success \\\n \t test_cmp expect.qux actual.qux &&\n \t test_cmp expect.qux actual.quux'\n \n+test_expect_success PIPE 'N: read and copy root' '\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+\t:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100\tnewdir/exec.sh\tfile3/newdir/exec.sh\n+\t:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100\tnewdir/interesting\tfile3/newdir/interesting\n+\tEOF\n+\tgit update-ref -d refs/heads/N12 &&\n+\trm -f backflow &&\n+\tmkfifo backflow &&\n+\t(\n+\t\texec <backflow &&\n+\t\tcat <<-EOF &&\n+\t\tcommit refs/heads/N12\n+\t\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\t\tdata <<COMMIT\n+\t\tcopy root directory by tree hash read via ls\n+\t\tCOMMIT\n+\n+\t\tfrom refs/heads/branch^0\n+\t\tls \"\"\n+\t\tEOF\n+\t\tread mode type tree filename &&\n+\t\techo \"M 040000 $tree file3\"\n+\t) |\n+\tgit fast-import --cat-blob-fd=3 3>backflow &&\n+\tgit diff-tree -C --find-copies-harder -r N12^ N12 >actual &&\n+\tcompare_diff_raw expect actual\n+'\n+\n ###\n ### series O\n ###\n-- \n1.7.9.3\n"},{"id":"186383","messageId":"20120308070951.GA2181@burratino","threadId":"29878","inReplyTo":"1331184656-98629-1-git-send-email-davidbarr@google.com","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T07:09:51Z","receivedAt":"2012-03-08T07:09:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Sverre)\nDavid Barr wrote:\n\n> When the following command is sent to fast-import:\n>\n>  'ls' SP ':1' SP LF\n>\n> The expected output is:\n>\n>  '040000' SP 'tree' SP <dataref> HT LF\n>\n> The actual output is:\n>\n>  'missing' SP LF\n>\n> This is because tree_content_get() is called but expects a non-empty\n> path. Instead, copy the root entry and force the mode to S_IFDIR.\n>\n> Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>\n> Signed-off-by: David Barr <davidbarr@google.com>\n\nFor what it's worth,\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks very much for taking care of it.\n\n> [Subject: fast-import: fix ls command with empty path]\n\nI would s/fix/accept/ to be more precise about the nature of the\nbreakage.  (In other words: rather than mishandling ls with an empty\npath, fast-import was not handling it at all.)\n\n[...]\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -3019,7 +3019,12 @@ 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\ttree_content_get(root, p, &leaf);\n> +\t} else {\n> +\t\tleaf = *root;\n> +\t\tleaf.versions[1].mode = S_IFDIR;\n> +\t}\n\nIf this special case were implemented in tree_content_get(), we would\nget support for paths with a trailing '/' (allows useless\nincompatibility, bad) and support for requests like\n\n\tC \"\" some/subdir\n\n(good).  What do you think?\n\n-- >8 --\nSubject: fast-import: allow filecopy to copy from root\n\nSome subversion users apparently use \"svn copy $SVN_ROOT\n$SVN_ROOT/subdirectory\" from time to time.  svn-fe handles this fine\nalready since it translates the subversion copy instruction to\n\n\tls \"\"\n\tM 040000 <returned tree name> subdirectory\n\nWe can easily imagine an alternate importer that would write\n\n\tC \"\" subdirectory\n\ninstead, so handle that, too.\n\nA naive implementation would also mean gaining support for copies\nwhere the source has a trailing '/', as in\n\n\tC onedir/ anotherdir\n\nbut in the spirit of 34215783 (fast-import: tighten M 040000 syntax,\n2010-10-17), this patch is careful to reject that syntax to avoid\nmaking it too easy for frontends to introduce unnecessary\nincompatibilities with git fast-import 1.7.8 and older and other\nfast-import consumers.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n fast-import.c          |   32 ++++++++++++++-----------\n t/t9300-fast-import.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 79 insertions(+), 15 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 8dbfd4cc..5ce61bca 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1636,6 +1636,10 @@ static int tree_content_get(\n \tunsigned int i, n;\n \tstruct tree_entry *e;\n \n+\tif (!*p) {\n+\t\te = root;\n+\t\tgoto last_component;\n+\t}\n \tslash1 = strchr(p, '/');\n \tif (slash1)\n \t\tn = slash1 - p;\n@@ -1648,14 +1652,10 @@ static int tree_content_get(\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 last_component;\n+\t\t\tif (!slash1[1])\t/* paths with trailing '/' do not match */\n+\t\t\t\treturn 0;\n \t\t\tif (!S_ISDIR(e->versions[1].mode))\n \t\t\t\treturn 0;\n \t\t\tif (!e->tree)\n@@ -1664,6 +1664,14 @@ static int tree_content_get(\n \t\t}\n \t}\n \treturn 0;\n+\n+last_component:\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@@ -3005,6 +3013,7 @@ 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\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@@ -3019,12 +3028,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-\t\ttree_content_get(root, p, &leaf);\n-\t} else {\n-\t\tleaf = *root;\n-\t\tleaf.versions[1].mode = S_IFDIR;\n-\t}\n+\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 2558a2ed..5316b73c 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1047,6 +1047,66 @@ test_expect_success \\\n \t git diff-tree -C --find-copies-harder -r N1^ N1 >actual &&\n \t compare_diff_raw expect actual'\n \n+test_tick\n+cat >input <<INPUT_END\n+commit refs/heads/N-root-to-subdir\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+copy to subdir\n+COMMIT\n+\n+from refs/heads/branch^0\n+C \"\" subdir\n+\n+INPUT_END\n+\n+cat >expect <<\\EOF\n+:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100\tfile2/newf\tsubdir/file2/newf\n+:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100\tfile2/oldf\tsubdir/file2/oldf\n+:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100\tfile4\tsubdir/file4\n+:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100\tnewdir/exec.sh\tsubdir/newdir/exec.sh\n+:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100\tnewdir/interesting\tsubdir/newdir/interesting\n+EOF\n+test_expect_success \\\n+\t'N: copy with empty source path' \\\n+\t'git fast-import <input &&\n+\t git diff-tree -C -C -r --no-commit-id N-root-to-subdir >actual &&\n+\t compare_diff_raw expect actual'\n+\n+test_tick\n+cat >input <<INPUT_END\n+commit refs/heads/N-unquoted-root-to-subdir\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+increase nesting\n+COMMIT\n+\n+from refs/heads/branch^0\n+C  subdir\n+\n+INPUT_END\n+test_expect_success \\\n+\t'N: copy with unquoted empty source path' \\\n+\t'git fast-import <input &&\n+\t git diff --exit-code N-root-to-subdir N-unquoted-root-to-subdir'\n+\n+test_tick\n+cat >input <<INPUT_END\n+commit refs/heads/N-trailing-slash-in-src\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+copy newdir to newerdir\n+COMMIT\n+\n+from refs/heads/branch^0\n+C newdir/ newerdir\n+\n+INPUT_END\n+test_expect_success \\\n+\t'N: reject foo/ as source path' \\\n+\t'# fatal: path not in branch\n+\t test_must_fail git fast-import <input'\n+\n cat >input <<INPUT_END\n commit refs/heads/N2\n committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -1293,7 +1353,7 @@ test_expect_success \\\n \t compare_diff_raw expect actual'\n \n test_expect_success \\\n-\t'N: reject foo/ syntax' \\\n+\t'N: filemodify: reject foo/ syntax' \\\n \t'subdir=$(git rev-parse refs/heads/branch^0:file2) &&\n \t test_must_fail git fast-import <<-INPUT_END\n \tcommit refs/heads/N5B\n-- \n1.7.9.2\n"},{"id":"186457","messageId":"CAGdFq_iKaruvsLi73F6oVwLs95Ka0AbSRx3UsFv-HVr9OzNqHw@mail.gmail.com","threadId":"29878","inReplyTo":"20120308070951.GA2181@burratino","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2012-03-08T15:29:38Z","receivedAt":"2012-03-08T15:29:38Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Thu, Mar 8, 2012 at 01:09, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> [Subject: fast-import: fix ls command with empty path]\n>\n> I would s/fix/accept/ to be more precise about the nature of the\n> breakage.  (In other words: rather than mishandling ls with an empty\n> path, fast-import was not handling it at all.)\n\nMakes sense to me :)\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"186463","messageId":"7vty1zdp2b.fsf@alter.siamese.dyndns.org","threadId":"29878","inReplyTo":"20120308070951.GA2181@burratino","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-08T16:39:56Z","receivedAt":"2012-03-08T16:39:56Z","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> For what it's worth,\n> Acked-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> Thanks very much for taking care of it.\n>\n>> [Subject: fast-import: fix ls command with empty path]\n>\n> I would s/fix/accept/ to be more precise about the nature of the\n> breakage.  (In other words: rather than mishandling ls with an empty\n> path, fast-import was not handling it at all.)\n>\n> ...\n> (good).  What do you think?\n>\n> -- >8 --\n> Subject: fast-import: allow filecopy to copy from root\n\nSo what do you guys want to do with topic?  My gut feeling is that\nthis is not a new regression and can wait until the next cycle.  I\ncould certainly carry David's patch in 'pu' if doing so helps the\ndiscussion to come up with the right solution, though.\n"},{"id":"186466","messageId":"CA+gfSn8bh-tV+uduM7xsuwqXQW2a57yvVmRXjXjp9JaO779bUg@mail.gmail.com","threadId":"29878","inReplyTo":"7vty1zdp2b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-03-08T17:33:10Z","receivedAt":"2012-03-08T17:33:10Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Don't quite have the time to run tests, but maybe the issue is solved\n in a stalled series [1]. At least [1] is worth looking at with regard\n to this bug.\n\n One more quick thought. \"force the root mode to S_IFDIR\" part doesn't\n look obviously good for me. First, isn't it already ensured that the\n root is a directory? Second, if it is allowed to be a file I'm not\n sure it's ok to silently make it a directory on a root-to-subtree\n operation, do we do it for subtree-to-subsubtree?\n\n P.S. looking at [1] now I'd say the commit messages could be improved there\n\n [1] http://thread.gmane.org/gmane.comp.version-control.git/179426\n\nOn Thu, Mar 8, 2012 at 10:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>> For what it's worth,\n>> Acked-by: Jonathan Nieder <jrnieder@gmail.com>\n>>\n>> Thanks very much for taking care of it.\n>>\n>>> [Subject: fast-import: fix ls command with empty path]\n>>\n>> I would s/fix/accept/ to be more precise about the nature of the\n>> breakage.  (In other words: rather than mishandling ls with an empty\n>> path, fast-import was not handling it at all.)\n>>\n>> ...\n>> (good).  What do you think?\n>>\n>> -- >8 --\n>> Subject: fast-import: allow filecopy to copy from root\n>\n> So what do you guys want to do with topic?  My gut feeling is that\n> this is not a new regression and can wait until the next cycle.  I\n> could certainly carry David's patch in 'pu' if doing so helps the\n> discussion to come up with the right solution, though.\n>\n>\n"},{"id":"186473","messageId":"20120308181551.GA17838@burratino","threadId":"29878","inReplyTo":"7vty1zdp2b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T18:15:51Z","receivedAt":"2012-03-08T18:15:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> So what do you guys want to do with topic?  My gut feeling is that\n> this is not a new regression and can wait until the next cycle.\n\nYes, I think you are very right.\n\n. I have a vague fear that my \"allow filecopy to copy from root\" patch\n  on top of David's is missing some handling of the empty src case,\n  along the same lines as 8fe533f6 (fast-import: treat filemodify with\n  empty tree as delete.\n\n. After looking closer at David's patch, it does not seem to handle\n  'ls <tree> \"\"' carefully enough.  It probably needs something\n  like Dmitry's [1].\n\nSo please backburner this, and we can try for something better by\nnext cycle.\n\nThanks for a sanity check.\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/179426/focus=179425\n"},{"id":"186482","messageId":"20120308193220.GA27916@burratino","threadId":"29878","inReplyTo":"CA+gfSn8bh-tV+uduM7xsuwqXQW2a57yvVmRXjXjp9JaO779bUg@mail.gmail.com","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T19:32:20Z","receivedAt":"2012-03-08T19:32:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n\n>  One more quick thought. \"force the root mode to S_IFDIR\" part doesn't\n>  look obviously good for me.\n\nIt was just a problematic and incomplete version of what your \"be\nsaner with temporary trees\" does properly. ;-)\n\n[...]\n>  P.S. looking at [1] now I'd say the commit messages could be improved there\n>\n>  [1] http://thread.gmane.org/gmane.comp.version-control.git/179426\n\nYes, please.  Or patch 2/2 could be split into multiple patches,\nperhaps along the following lines:\n\n - ls \"\" support, as in David's patch\n - ls <dataref> \"\" support, which requires the \"be saner with\n   temporaries\" fix\n - D \"\" support, maybe.  (As you mentioned, we have deleteall so\n   compatibility would dictate not supporting it unless some frontend\n   is using it already by mistake in some circumstance.)\n - C \"\" <dest> support, as in my reply to David's patch\n - R \"\" <dest> support\n\nThanks,\nJonathan\n"},{"id":"186489","messageId":"20120308202721.GA8992@burratino","threadId":"29878","inReplyTo":"1331184656-98629-1-git-send-email-davidbarr@google.com","subject":"[PATCH v2 0/2] Re: fast-import: fix ls command with empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T20:27:21Z","receivedAt":"2012-03-08T20:27:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Barr wrote:\n\n> When the following command is sent to fast-import:\n>\n>  'ls' SP ':1' SP LF\n>\n> The expected output is:\n>\n>  '040000' SP 'tree' SP <dataref> HT LF\n>\n> The actual output is:\n>\n>  'missing' SP LF\n>\n> This is because tree_content_get() is called but expects a non-empty\n> path. Instead, copy the root entry and force the mode to S_IFDIR.\n\nThanks again for your help.  I've pushed the following changes to\n\n  git://repo.or.cz/git/jrn.git fast-import-pu\n\nTesting, review, and improvements welcome.\n\nDavid Barr (1):\n  fast-import: teach ls command to accept empty path\n\nJonathan Nieder (1):\n  fast-import: plug leak of dirty trees in 'ls' command\n\n fast-import.c          |   19 +++++++++-\n t/t9300-fast-import.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 113 insertions(+), 1 deletion(-)\n"},{"id":"186490","messageId":"20120308203139.GB8992@burratino","threadId":"29878","inReplyTo":"20120308202721.GA8992@burratino","subject":"[PATCH 1/2] fast-import: plug leak of dirty trees in 'ls' command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T20:31:39Z","receivedAt":"2012-03-08T20:31:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When the named directory has changed since it was last written to\npack, \"tree_content_get\" makes a deep copy of the list of tree entries\nwhich we forgot to free.\n\nThis memory leak has been present since the \"ls\" command was\nintroduced in v1.7.5-rc0~3^2~33 (fast-import: add 'ls' command,\n2010-12-02).\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nAfter rediscovering this, I found [1] which mentions the same bug.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/178007/focus=178044\n\n fast-import.c |    2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex c1486cab..1758da94 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3028,6 +3028,8 @@ static void parse_ls(struct branch *b)\n \t\tstore_tree(&leaf);\n \n \tprint_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);\n+\tif (leaf.tree)\n+\t\trelease_tree_content_recursive(leaf.tree);\n \tif (!b || root != &b->branch_tree)\n \t\trelease_tree_entry(root);\n }\n-- \n1.7.9.2\n"},{"id":"186491","messageId":"20120308203330.GC8992@burratino","threadId":"29878","inReplyTo":"20120308202721.GA8992@burratino","subject":"[PATCH 2/2] fast-import: teach ls command to accept empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T20:33:30Z","receivedAt":"2012-03-08T20:33:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: David Barr <davidbarr@google.com>\n\nThere is a pathological Subversion operation that svn-fe handles\nincorrectly due to an unexpected response from fast-import:\n\n  svn cp $SVN_ROOT $SVN_ROOT/subdirectory\n\nWhen the following command is sent to fast-import:\n\n 'ls' SP ':1' SP LF\n\nThe expected output is:\n\n '040000' SP 'tree' SP <dataref> HT LF\n\nThe actual output is:\n\n 'missing' SP LF\n\nThis is because tree_content_get() is called but expects a non-empty\npath. Instead, copy the root entry.\n\n[jn: using a deep copy; w/ more tests]\n[jn: with a fix from Dmitry to fully initialize root->versions[0]\n and versions[1] now that root can be passed to store_tree]\n\nReported-by: Andrew Sayers <andrew-git@pileofstuff.org>\nSigned-off-by: David Barr <davidbarr@google.com>\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n fast-import.c          |   17 ++++++++-\n t/t9300-fast-import.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+), 1 deletion(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 1758da94..31857e95 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3004,7 +3004,10 @@ 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\thashcpy(root->versions[1].sha1, e->idx.sha1);\n+\t\troot->versions[0].mode = 0;\n+\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@@ -3019,7 +3022,19 @@ 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\ttree_content_get(root, p, &leaf);\n+\t} else {\n+\t\tmemcpy(&leaf, root, sizeof(leaf));\n+\t\t/*\n+\t\t * store_tree scribbles over version[0] in leaf.tree's\n+\t\t * entries, so we need a deep copy.\n+\t\t */\n+\t\tif (root->tree && is_null_sha1(root->versions[1].sha1))\n+\t\t\tleaf.tree = dup_tree_content(root->tree);\n+\t\telse\n+\t\t\tleaf.tree = NULL;\n+\t}\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 438aaf6b..635bfadb 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1400,6 +1400,101 @@ 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_success '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^{tree}) &&\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 directory 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+\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+\t:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100\tnewdir/exec.sh\tfile3/newdir/exec.sh\n+\t:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100\tnewdir/interesting\tfile3/newdir/interesting\n+\tEOF\n+\tgit update-ref -d refs/heads/N12 &&\n+\trm -f backflow &&\n+\tmkfifo backflow &&\n+\t(\n+\t\texec <backflow &&\n+\t\tcat <<-EOF &&\n+\t\tcommit refs/heads/N12\n+\t\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\t\tdata <<COMMIT\n+\t\tcopy root directory by tree hash read via ls\n+\t\tCOMMIT\n+\n+\t\tfrom refs/heads/branch^0\n+\t\tls \"\"\n+\t\tEOF\n+\t\tread mode type tree filename &&\n+\t\techo \"M 040000 $tree file3\"\n+\t) |\n+\tgit fast-import --cat-blob-fd=3 3>backflow &&\n+\tgit diff-tree -C --find-copies-harder -r N12^ N12 >actual &&\n+\tcompare_diff_raw expect actual\n+'\n+\n ###\n ### series O\n ###\n-- \n1.7.9.2\n"},{"id":"186520","messageId":"CAFfmPPNnQ21qwKb_w1FCRL7Vx7CSQKYurM2zqziTw01kkRoMog@mail.gmail.com","threadId":"29878","inReplyTo":"20120308203330.GC8992@burratino","subject":"Re: [PATCH 2/2] fast-import: teach ls command to accept empty path","fromName":"David Barr","fromEmail":"davidbarr@google.com","sentAt":"2012-03-09T04:28:02Z","receivedAt":"2012-03-09T04:28:02Z","isPatch":true,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"On Fri, Mar 9, 2012 at 7:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> From: David Barr <davidbarr@google.com>\n>\n> There is a pathological Subversion operation that svn-fe handles\n> incorrectly due to an unexpected response from fast-import:\n>\n>  svn cp $SVN_ROOT $SVN_ROOT/subdirectory\n>\n> When the following command is sent to fast-import:\n>\n>  'ls' SP ':1' SP LF\n>\n> The expected output is:\n>\n>  '040000' SP 'tree' SP <dataref> HT LF\n>\n> The actual output is:\n>\n>  'missing' SP LF\n>\n> This is because tree_content_get() is called but expects a non-empty\n> path. Instead, copy the root entry.\n>\n> [jn: using a deep copy; w/ more tests]\n> [jn: with a fix from Dmitry to fully initialize root->versions[0]\n>  and versions[1] now that root can be passed to store_tree]\n>\n> Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>\n> Signed-off-by: David Barr <davidbarr@google.com>\n> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> +               /*\n> +                * store_tree scribbles over version[0] in leaf.tree's\n> +                * entries, so we need a deep copy.\n> +                */\n> +               if (root->tree && is_null_sha1(root->versions[1].sha1))\n> +                       leaf.tree = dup_tree_content(root->tree);\n\nIs it ok to call store_tree(root)? If so, could we not introduce a\npointer rather than a deep copy?\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 94d7037..eab24f3 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2993,7 +2993,8 @@ static void parse_ls(struct branch *b)\n {\n \tconst char *p;\n \tstruct tree_entry *root = NULL;\n-\tstruct tree_entry leaf = {NULL};\n+\tstruct tree_entry tmp_tree = {NULL};\n+\tstruct tree_entry *leaf = &tmp_tree;\n\n \t/* ls SP (<treeish> SP)? <path> */\n \tp = command_buf.buf + strlen(\"ls \");\n@@ -3022,27 +3023,18 @@ 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-\t\ttree_content_get(root, p, &leaf);\n-\t} else {\n-\t\tmemcpy(&leaf, root, sizeof(leaf));\n-\t\t/*\n-\t\t * store_tree scribbles over version[0] in leaf.tree's\n-\t\t * entries, so we need a deep copy.\n-\t\t */\n-\t\tif (root->tree && is_null_sha1(root->versions[1].sha1))\n-\t\t\tleaf.tree = dup_tree_content(root->tree);\n-\t\telse\n-\t\t\tleaf.tree = NULL;\n-\t}\n+\tif (*p)\n+\t\ttree_content_get(root, p, leaf);\n+\telse\n+\t\tleaf = root;\n \t/*\n \t * A directory in preparation would have a sha1 of zero\n \t * until it is saved.  Save, for simplicity.\n \t */\n-\tif (S_ISDIR(leaf.versions[1].mode))\n-\t\tstore_tree(&leaf);\n+\tif (S_ISDIR(leaf->versions[1].mode))\n+\t\tstore_tree(leaf);\n\n-\tprint_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);\n+\tprint_ls(leaf->versions[1].mode, leaf->versions[1].sha1, p);\n \tif (!b || root != &b->branch_tree)\n \t\trelease_tree_entry(root);\n }\n\n--\nDavid Barr\n"},{"id":"186544","messageId":"20120309092940.GB2229@burratino","threadId":"29878","inReplyTo":"CAFfmPPNnQ21qwKb_w1FCRL7Vx7CSQKYurM2zqziTw01kkRoMog@mail.gmail.com","subject":"Re: [PATCH 2/2] fast-import: teach ls command to accept empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-09T09:29:40Z","receivedAt":"2012-03-09T09:29:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Barr wrote:\n> On Fri, Mar 9, 2012 at 7:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> +               /*\n>> +                * store_tree scribbles over version[0] in leaf.tree's\n>> +                * entries, so we need a deep copy.\n>> +                */\n>> +               if (root->tree && is_null_sha1(root->versions[1].sha1))\n>> +                       leaf.tree = dup_tree_content(root->tree);\n>\n> Is it ok to call store_tree(root)?\n\nYes.\n\n>                                     If so, could we not introduce a\n> pointer rather than a deep copy?\n\nIf using 'ls' with an empty path after dirtying the root tree is\ncommon, then that would work as an optimization.  The fussy bit is\nmaking sure the call to\n\n\trelease_tree_content_recursive(leaf.tree);\n\nis skipped in this case and not skipped when tree_content_get() made a\ncopy.  That is, something like this (patch against fast-import-pu on\nrepo.or.cz/git/jrn.git):\n\n-- >8 --\nFrom: David Barr <davidbarr@google.com>\nSubject: fast-import: optimize 'ls' command with empty path to avoid a copy\n\nfast-import's \"ls\" command normally copies a tree (implicitly, by\ncalling tree_content_get) before passing it to store_tree.  Otherwise:\n\n - after versions[0] is overwritten by versions[1] in child\n   directories, it would be impossible to rebuild the tree object for\n   version 0 of the current tree, so parse_ls would need to\n\n\thashcpy(leaf.versions[0].sha1, leaf.versions[1].sha1)\n\n   so version 0 points to a tree that can be rebuilt.\n\n - in turn, that would make it impossible to rebuild the tree object\n   for version 0 of the parent tree.  And so on.\n\nThe above considerations do not apply when the tree we are examining\nwith 'ls' has no parent.  Avoid a copy in that case.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n fast-import.c |   22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 1e5d59b4..28fe4c35 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3002,7 +3002,8 @@ static void parse_ls(struct branch *b)\n {\n \tconst char *p;\n \tstruct tree_entry *root = NULL;\n-\tstruct tree_entry leaf = {NULL};\n+\tstruct tree_entry leaf_storage = {NULL};\n+\tstruct tree_entry *leaf = &leaf_storage;\n \n \t/* ls SP (<treeish> SP)? <path> */\n \tp = command_buf.buf + strlen(\"ls \");\n@@ -3029,17 +3030,24 @@ 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\ttree_content_get(root, p, leaf);\n+\telse\n+\t\tleaf = root;\n+\n \t/*\n \t * A directory in preparation would have a sha1 of zero\n \t * until it is saved.  Save, for simplicity.\n \t */\n-\tif (S_ISDIR(leaf.versions[1].mode))\n-\t\tstore_tree(&leaf);\n+\tif (S_ISDIR(leaf->versions[1].mode)\n+\t    && is_null_sha1(leaf->versions[1].sha1)) {\n+\t\tstore_tree(leaf);\n+\t\thashcpy(leaf->versions[0].sha1, leaf->versions[1].sha1);\n+\t}\n \n-\tprint_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);\n-\tif (leaf.tree)\n-\t\trelease_tree_content_recursive(leaf.tree);\n+\tprint_ls(leaf->versions[1].mode, leaf->versions[1].sha1, p);\n+\tif (*p && leaf->tree)\n+\t\trelease_tree_content_recursive(leaf->tree);\n \tif (!b || root != &b->branch_tree)\n \t\trelease_tree_entry(root);\n }\n-- \n1.7.9.2\n"},{"id":"186607","messageId":"20120310031228.GA3008@burratino","threadId":"29878","inReplyTo":"7vty1zdp2b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T03:12:28Z","receivedAt":"2012-03-10T03:12:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>>> [Subject: fast-import: fix ls command with empty path]\n>>\n>> I would s/fix/accept/ to be more precise about the nature of the\n>> breakage.  (In other words: rather than mishandling ls with an empty\n>> path, fast-import was not handling it at all.)\n[...]\n> So what do you guys want to do with topic?  My gut feeling is that\n> this is not a new regression and can wait until the next cycle.\n\nThanks again for the advice so far.\n\nAfter sleeping on it, here are two patches for 'maint'.  One plugs a\nmemory leak.  The other makes my above comment actually true, so\ntrying to use this missing feature results in an error message that\ncan help the frontend author instead of the silently broken conversion\nAndrew found.\n\nThen we can carefully add 'ls \"\"' support in 1.7.11.\n\nsvn-fe should probably also be tweaked to handle this case without\ndemanding support for the (nice) 'ls empty path' extension in\nfast-import backends, and this could even happen before 1.7.10.\nI can't promise to get to that quickly enough, though.\n\nSensible?\n\nJonathan\n"},{"id":"186608","messageId":"20120310032034.GB3008@burratino","threadId":"29878","inReplyTo":"20120310031228.GA3008@burratino","subject":"[PATCH maint-1.7.6] fast-import: leakfix for 'ls' of dirty trees","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T03:20:34Z","receivedAt":"2012-03-10T03:20:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When the chosen directory has changed since it was last written to\npack, \"tree_content_get\" makes a deep copy of its content to scribble\non while computing the tree name, which we forgot to free.\n\nThis leak has been present since the 'ls' command was introduced in\nv1.7.5-rc0~3^2~33 (fast-import: add 'ls' command, 2010-12-02).\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nSorry to have missed this buglet for so long.  (Doubly so because it\nwas noticed and commented on half a year ago before being promptly\nforgotten.)  Patch is against commit 8dc6a373d2 which introduced the\nleaky 'ls' support.\n\n fast-import.c |    2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 6c37b840..fff285cd 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2987,6 +2987,8 @@ static void parse_ls(struct branch *b)\n \t\tstore_tree(&leaf);\n \n \tprint_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);\n+\tif (leaf.tree)\n+\t\trelease_tree_content_recursive(leaf.tree);\n \tif (!b || root != &b->branch_tree)\n \t\trelease_tree_entry(root);\n }\n-- \n1.7.9.2\n"},{"id":"186609","messageId":"20120310040049.GC3008@burratino","threadId":"29878","inReplyTo":"20120310031228.GA3008@burratino","subject":"[PATCH maint-1.7.6] fast-import: don't allow 'ls' of path with empty components","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T04:00:49Z","receivedAt":"2012-03-10T04:00:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"As the fast-import manual explains:\n\n\tThe value of <path> must be in canonical form. That is it must\n\tnot:\n\t. contain an empty directory component (e.g. foo//bar is invalid),\n\t. end with a directory separator (e.g. foo/ is invalid),\n\t. start with a directory separator (e.g. /foo is invalid),\n\nUnfortunately the \"ls\" command accepts these invalid syntaxes and\nresponds by declaring that the indicated path is missing.  This is too\nsubtle and causes importers to silently misbehave; better to error out\nso the operator knows what's happening.\n\nThe C, R, and M commands already error out for such paths.\n\nBased on initial analysis by David Barr.\n\nReported-by: Andrew Sayers <andrew-git@pileofstuff.org>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nAlso against on 8dc6a373d (fast-import: add 'ls' command, 2010-12-02).\n\n fast-import.c          |    2 ++\n t/t9300-fast-import.sh |   39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex fff285cd..47f61f3c 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1640,6 +1640,8 @@ static int tree_content_get(\n \t\tn = slash1 - p;\n \telse\n \t\tn = strlen(p);\n+\tif (!n)\n+\t\tdie(\"Empty path component found in input\");\n \n \tif (!root->tree)\n \t\tload_tree(root);\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 6b1ba6c8..2cd0f061 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1088,6 +1088,45 @@ test_expect_success \\\n \tINPUT_END'\n \n+test_expect_success \\\n+\t'N: reject foo/ syntax in copy source' \\\n+\t'test_must_fail git fast-import <<-INPUT_END\n+\tcommit refs/heads/N5C\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tcopy with invalid syntax\n+\tCOMMIT\n+\n+\tfrom refs/heads/branch^0\n+\tC file2/ file3\n+\tINPUT_END'\n+\n+test_expect_success \\\n+\t'N: reject foo/ syntax in rename source' \\\n+\t'test_must_fail git fast-import <<-INPUT_END\n+\tcommit refs/heads/N5D\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\trename with invalid syntax\n+\tCOMMIT\n+\n+\tfrom refs/heads/branch^0\n+\tR file2/ file3\n+\tINPUT_END'\n+\n+test_expect_success \\\n+\t'N: reject foo/ syntax in ls argument' \\\n+\t'test_must_fail git fast-import <<-INPUT_END\n+\tcommit refs/heads/N5E\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tcopy with invalid syntax\n+\tCOMMIT\n+\n+\tfrom refs/heads/branch^0\n+\tls \"file2/\"\n+\tINPUT_END'\n+\n test_expect_success \\\n \t'N: copy to root by id and modify' \\\n \t'echo \"hello, world\" >expect.foo &&\n \t echo hello >expect.bar &&\n-- \n1.7.9.2\n"},{"id":"186610","messageId":"20120310043027.GA1992@burratino","threadId":"29878","inReplyTo":"20120310031228.GA3008@burratino","subject":"[PULL maint] two fast-import \"ls\" fixes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T04:30:27Z","receivedAt":"2012-03-10T04:30:27Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> After sleeping on it, here are two patches for 'maint'.\n\nFor your convenience, these changes can also be found at:\n\n  git://repo.or.cz/git/jrn.git tags/fast-import-ls-fixes\n\n      fast-import: leakfix for 'ls' of dirty trees\n      fast-import: don't allow 'ls' of path with empty components\n\n fast-import.c          |    4 ++++\n t/t9300-fast-import.sh |   39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+)\n"},{"id":"186614","messageId":"20120310070016.GC1992@burratino","threadId":"29878","inReplyTo":"20120310031228.GA3008@burratino","subject":"[NON-PATCH] vcs-svn: avoid 'ls' and filedelete with empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T07:00:16Z","receivedAt":"2012-03-10T07:00:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> svn-fe should probably also be tweaked to handle this case without\n> demanding support for the (nice) 'ls empty path' extension in\n> fast-import backends, and this could even happen before 1.7.10.\n> I can't promise to get to that quickly enough, though.\n\nOn second thought, svn-fe will need the 'ls empty path' extension\nbefore it can handle this case again. :(\n\n-- >8 --\nThere is a pathological Subversion operation that svn-fe handles\nincorrectly due to an unexpected response from fast-import:\n\n  svn cp $SVN_ROOT $SVN_ROOT/subdirectory\n\nsvn-fe tries to handle this as a copy from \"\" to \"subdirectory\", in\ntwo steps: first, \"ls :1 \" to retrieve a <dataref> for the root, and\nthen \"M 040000 <dataref> subdirectory\" to use it in the active commit.\n\nIn git 1.7.9.3 and earlier, the fast-import \"ls\" command does not\nunderstand that by the empty path we mean the root of the tree.  The\nunrecognized path is reported as \"missing\", so svn-fe emits \"D\nsubdirectory\" and continues unaware of the miscommunication that has\ntaken place.\n\nThis is a regression introduced by commit 723b7a27 (vcs-svn: eliminate\nrepo_tree structure, 2010-12-10).\n\nA patch in flight teaches fast-import to error out, which is a little\nbetter, but still does not win us a successful and accurate import.\nBecause the meaning of empty paths was not specified in the\nfast-import manual until recently, other backends are likely to handle\nthis construct inconsistently, too.\n\nsvn-fe never actually needs to pass \"\" as an argument to 'ls',\n'C', 'R', or 'D'. (*)  There is always another way to spell what it is\ntrying to do:\n\n. Making a 'ls <foo> \"\"' request and waiting for a response is a\n  complicated way to spell the identity operation.  When <foo> is\n  a commit, <foo> can be used directly to name the root of the\n  corresponding tree. (*)\n\n. Emitting 'ls \"\"' to get a name for the root of the current tree\n  would be useful in general and there is no other command that\n  does that.  Svn-fe never does that: translating non-copy nodes from\n  the Subversion dump format does not involve retrieving current\n  directory listings to apply a delta to them, and for copies, the\n  copyfrom information refers to a specific previous revision.\n\n. svn-fe does not use the filerename (R) and filecopy (C) commands.\n\n. The intent of the command 'D \"\"' is more clearly written as\n  'deleteall'.\n\nReported-by: Andrew Sayers <andrew-git@pileofstuff.org>\nExplained-by: David Barr <davidbarr@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n\nToy patch, not intended for application.\n\n(*) Lies.\n---\n t/t9010-svn-fe.sh     |   69 +++++++++++++++++++++++++++++++++++++++++++++++++\n vcs-svn/fast_export.c |   25 +++++++++++++++++-\n vcs-svn/fast_export.h |    2 +-\n vcs-svn/repo_tree.c   |    6 +++--\n vcs-svn/repo_tree.h   |    2 +-\n vcs-svn/svndump.c     |    3 ++-\n 6 files changed, 101 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh\nindex b7eed248..45706bde 100755\n--- a/t/t9010-svn-fe.sh\n+++ b/t/t9010-svn-fe.sh\n@@ -271,6 +271,75 @@ test_expect_success PIPE 'directory with files' '\n \ttest_cmp hi directory/file2\n '\n \n+test_expect_success PIPE 'copy from root to directory' '\n+\treinit_git &&\n+\techo hello >hello &&\n+\thello_blob=$(git hash-object -w -t blob hello) &&\n+\tsubtree=$(\n+\t\techo \"100644 blob $hello_blob\tREADME.txt\" |\n+\t\tgit mktree\n+\t) &&\n+\texpect=$(\n+\t\tgit mktree <<-EOF\n+\t\t\t100644 blob $hello_blob\tREADME.txt\n+\t\t\t040000 tree $subtree\ttrunk\n+\t\tEOF\n+\t) &&\n+\n+\t{\n+\t\tproperties \\\n+\t\t\tsvn:author author@example.com \\\n+\t\t\tsvn:date \"2012-10-10T00:01:003.000000Z\" \\\n+\t\t\tsvn:log \"created README.txt\" &&\n+\t\techo PROPS-END\n+\t} >r1.props &&\n+\t{\n+\t\tproperties \\\n+\t\t\tsvn:author author@example.com \\\n+\t\t\tsvn:date \"2012-10-10T00:02:005.000000Z\" \\\n+\t\t\tsvn:log \"created trunk\" &&\n+\t\techo PROPS-END\n+\t} >r2.props &&\n+\t{\n+\t\tcat <<-EOF &&\n+\t\tSVN-fs-dump-format-version: 3\n+\n+\t\tRevision-number: 1\n+\t\tEOF\n+\t\techo Prop-content-length: $(wc -c <r1.props) &&\n+\t\techo Content-length: $(wc -c <r1.props) &&\n+\t\techo &&\n+\t\tcat r1.props &&\n+\t\tcat <<-\\EOF &&\n+\n+\t\tNode-path: README.txt\n+\t\tNode-kind: file\n+\t\tNode-action: add\n+\t\tEOF\n+\t\ttext_no_props hello &&\n+\t\techo Revision-number: 2\n+\t\techo Prop-content-length: $(wc -c <r2.props) &&\n+\t\techo Content-length: $(wc -c <r2.props) &&\n+\t\techo &&\n+\t\tcat r2.props &&\n+\t\tsed -e \"s/X\\$//\" <<-\\EOF\n+\n+\t\tNode-path: trunk\n+\t\tNode-kind: dir\n+\t\tNode-action: add\n+\t\tNode-copyfrom-rev: 1\n+\t\tNode-copyfrom-path: X\n+\t\tProp-content-length: 10\n+\t\tContent-length: 10\n+\n+\t\tPROPS-END\n+\t\tEOF\n+\t} >copy-root.dump &&\n+\ttry_dump copy-root.dump &&\n+\n+\tgit diff-tree --exit-code $expect HEAD\n+'\n+\n test_expect_success PIPE 'branch name with backslash' '\n \treinit_git &&\n \tsort <<-\\EOF >expect.branch-files &&\ndiff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\nindex 19d7c34c..24232618 100644\n--- a/vcs-svn/fast_export.c\n+++ b/vcs-svn/fast_export.c\n@@ -49,6 +49,12 @@ void fast_export_reset(void)\n \n void fast_export_delete(const char *path)\n {\n+\t/* delete(\"\") means to return to a clean slate. */\n+\tif (!*path) {\n+\t\tprintf(\"deleteall\\n\");\n+\t\treturn;\n+\t}\n+\n \tputchar('D');\n \tputchar(' ');\n \tquote_c_style(path, NULL, stdout, 0);\n@@ -113,6 +119,8 @@ void fast_export_end_commit(uint32_t revision)\n \n static void ls_from_rev(uint32_t rev, const char *path)\n {\n+\tassert(*path);\n+\n \t/* ls :5 path/to/old/file */\n \tprintf(\"ls :%\"PRIu32\" \", rev);\n \tquote_c_style(path, NULL, stdout, 0);\n@@ -122,6 +130,8 @@ static void ls_from_rev(uint32_t rev, const char *path)\n \n static void ls_from_active_commit(const char *path)\n {\n+\tassert(*path);\n+\n \t/* ls \"path/to/file\" */\n \tprintf(\"ls \\\"\");\n \tquote_c_style(path, NULL, stdout, 1);\n@@ -285,12 +295,25 @@ static int parse_ls_response(const char *response, uint32_t *mode,\n int fast_export_ls_rev(uint32_t rev, const char *path,\n \t\t\t\tuint32_t *mode, struct strbuf *dataref)\n {\n+\tif (!*path) {\n+\t\t/*\n+\t\t * The easy case: when path is \"\", the caller can use\n+\t\t * :<rev> directly to refer to the root of the tree. (*)\n+\t\t */\n+\t\tstrbuf_addf(dataref, \":%\"PRIu32, rev);\n+\t\t*mode = REPO_MODE_DIR;\n+\t\treturn 0;\n+\t}\n+\n \tls_from_rev(rev, path);\n \treturn parse_ls_response(get_response_line(), mode, dataref);\n }\n \n-int fast_export_ls(const char *path, uint32_t *mode, struct strbuf *dataref)\n+int fast_export_ls_nonroot(const char *path, uint32_t *mode,\n+\t\t\t\tstruct strbuf *dataref)\n {\n+\tassert(*path);\n+\n \tls_from_active_commit(path);\n \treturn parse_ls_response(get_response_line(), mode, dataref);\n }\ndiff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h\nindex 43d05b65..53e208e2 100644\n--- a/vcs-svn/fast_export.h\n+++ b/vcs-svn/fast_export.h\n@@ -22,7 +22,7 @@ void fast_export_blob_delta(uint32_t mode,\n /* If there is no such file at that rev, returns -1, errno == ENOENT. */\n int fast_export_ls_rev(uint32_t rev, const char *path,\n \t\t\tuint32_t *mode_out, struct strbuf *dataref_out);\n-int fast_export_ls(const char *path,\n+int fast_export_ls_nonroot(const char *path,\n \t\t\tuint32_t *mode_out, struct strbuf *dataref_out);\n \n #endif\ndiff --git a/vcs-svn/repo_tree.c b/vcs-svn/repo_tree.c\nindex 67d27f0b..1547b8ce 100644\n--- a/vcs-svn/repo_tree.c\n+++ b/vcs-svn/repo_tree.c\n@@ -8,13 +8,15 @@\n #include \"repo_tree.h\"\n #include \"fast_export.h\"\n \n-const char *repo_read_path(const char *path, uint32_t *mode_out)\n+const char *repo_read_nonroot_path(const char *path, uint32_t *mode_out)\n {\n \tint err;\n \tstatic struct strbuf buf = STRBUF_INIT;\n \n+\tassert(*path);\n+\n \tstrbuf_reset(&buf);\n-\terr = fast_export_ls(path, mode_out, &buf);\n+\terr = fast_export_ls_nonroot(path, mode_out, &buf);\n \tif (err) {\n \t\tif (errno != ENOENT)\n \t\t\tdie_errno(\"BUG: unexpected fast_export_ls error\");\ndiff --git a/vcs-svn/repo_tree.h b/vcs-svn/repo_tree.h\nindex 889c6a3c..466d5a63 100644\n--- a/vcs-svn/repo_tree.h\n+++ b/vcs-svn/repo_tree.h\n@@ -11,7 +11,7 @@ struct strbuf;\n uint32_t next_blob_mark(void);\n void repo_copy(uint32_t revision, const char *src, const char *dst);\n void repo_add(const char *path, uint32_t mode, uint32_t blob_mark);\n-const char *repo_read_path(const char *path, uint32_t *mode_out);\n+const char *repo_read_nonroot_path(const char *path, uint32_t *mode_out);\n void repo_delete(const char *path);\n void repo_commit(uint32_t revision, const char *author,\n \t\tconst struct strbuf *log, const char *uuid, const char *url,\ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex ca63760f..d610184d 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -248,7 +248,8 @@ static void handle_node(void)\n \t\told_data = NULL;\n \t} else if (node_ctx.action == NODEACT_CHANGE) {\n \t\tuint32_t mode;\n-\t\told_data = repo_read_path(node_ctx.dst.buf, &mode);\n+\t\tassert(*node_ctx.dst.buf);\n+\t\told_data = repo_read_nonroot_path(node_ctx.dst.buf, &mode);\n \t\tif (mode == REPO_MODE_DIR && type != REPO_MODE_DIR)\n \t\t\tdie(\"invalid dump: cannot modify a directory into a file\");\n \t\tif (mode != REPO_MODE_DIR && type == REPO_MODE_DIR)\n-- \n1.7.9.2\n"},{"id":"186617","messageId":"20120310085354.GE1992@burratino","threadId":"29878","inReplyTo":"1331184656-98629-1-git-send-email-davidbarr@google.com","subject":"[PATCH v3] fast-import: allow 'ls' and filecopy to read the root","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T08:53:55Z","receivedAt":"2012-03-10T08:53:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"In the same spirit as v1.7.4-rc0~177 (fast-import: Allow filemodify to\nset the root, 2010-10-10), teach the 'ls' and 'C' commands the\nfollowing syntax:\n\n\tls \"\"\n\tls <dataref> \"\"\n\tC \"\" <path>\n\nAll three are requests to read from the directory at the top of the\nhierarchy.\n\nThe potential usefulness of this extension was discovered by using\nsvn-fe to import from a repository whose history included a\npathological Subversion operation:\n\n  svn cp $SVN_ROOT $SVN_ROOT/subdirectory\n\nSince v1.7.10-rc0~118^2~4^2~5^2~4 (vcs-svn: eliminate repo_tree\nstructure, 2010-12-10) svn-fe handles this by sending the command\n'ls :1 ' to fast-import, expecting output in the form\n\n  '040000' SP 'tree' SP <dataref> HT LF\n\ndescribing the toplevel directory so it can be copied.  After this\npatch, the import works, with no modification to svn-fe needed.\n\nSubtleties:\n\nThe 'ls <dataref> \"\"' command involves printing the makeshift \"root\"\ntree that represents <dataref>, so we need to initialize its mode.\n\nThe 'C \"\" <path>' command needs to be careful not to copy an empty\ntree to a subdirectory, as explained in v1.7.4~2^2~2^2 (fast-import:\ntreat filemodify with empty tree as delete, 2011-01-27).\n\nBased on a patch by David Barr that made the same change at the\nparse_ls level.  David's tests were carried over and some new ones\nadded.\n\nReported-by: Andrew Sayers <andrew-git@pileofstuff.org>\nSigned-off-by: David Barr <davidbarr@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nImproved-by: Dmitry Ivankov <divanorama@gmail.com>\n---\nOk, here's a patch for the svn-fe bug that I could live with.\n\nIt has a semantic conflict with the fast-import-ls-fixes series that\nI sent separately, which is fixed by adding\n\n\t\t\tif (!slash1[1])\n\t\t\t\tdie(\"Empty path component found in input\");\n\nafter\n\n\t\t\tif (!slash1)\n\t\t\t\tgoto last_component;\n\nand removing the now-useless\n\n\tif (!n)\n\t\tdie(\"Empty path component found in input\");\n\nI'll send a fixup patch as a reply, for squashing into the merge or\nthis patch, whichever is the first commit that contains both topics.\n\n fast-import.c          |   43 ++++++++----\n t/t9010-svn-fe.sh      |   69 +++++++++++++++++++\n t/t9300-fast-import.sh |  174 ++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 272 insertions(+), 14 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex c1486cab..75da2954 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1473,6 +1473,9 @@ static void tree_content_replace(\n \troot->tree = newtree;\n }\n \n+static int tree_content_remove(struct tree_entry *,\n+\t\t\t\t\tconst char *, struct tree_entry *);\n+\n static int tree_content_set(\n \tstruct tree_entry *root,\n \tconst char *p,\n@@ -1495,6 +1498,15 @@ static int tree_content_set(\n \tif (!slash1 && !S_ISDIR(mode) && subtree)\n \t\tdie(\"Non-directories cannot have subtrees\");\n \n+\t/* Git does not track empty directories. */\n+\tif (S_ISDIR(mode)) {\n+\t\tif ((is_null_sha1(sha1) && !subtree->entry_count)\n+\t\t    || !memcmp(sha1, EMPTY_TREE_SHA1_BIN, 20)) {\n+\t\t\ttree_content_remove(root, p, NULL);\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n \tif (!root->tree)\n \t\tload_tree(root);\n \tt = root->tree;\n@@ -1636,6 +1648,11 @@ static int tree_content_get(\n \tunsigned int i, n;\n \tstruct tree_entry *e;\n \n+\tif (!*p) {\n+\t\te = root;\n+\t\tgoto last_component;\n+\t}\n+\n \tslash1 = strchr(p, '/');\n \tif (slash1)\n \t\tn = slash1 - p;\n@@ -1648,14 +1665,8 @@ static int tree_content_get(\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 last_component;\n \t\t\tif (!S_ISDIR(e->versions[1].mode))\n \t\t\t\treturn 0;\n \t\t\tif (!e->tree)\n@@ -1664,6 +1675,14 @@ static int tree_content_get(\n \t\t}\n \t}\n \treturn 0;\n+\n+last_component:\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@@ -2256,12 +2275,6 @@ static void file_change_m(struct branch *b)\n \t\tp = uq.buf;\n \t}\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\treturn;\n-\t}\n-\n \tif (S_ISGITLINK(mode)) {\n \t\tif (inline_data)\n \t\t\tdie(\"Git links cannot be specified 'inline': %s\",\n@@ -2369,6 +2382,7 @@ static void file_change_cr(struct branch *b, int rename)\n \t\t\tleaf.tree);\n \t\treturn;\n \t}\n+\n \ttree_content_set(&b->branch_tree, d,\n \t\tleaf.versions[1].sha1,\n \t\tleaf.versions[1].mode,\n@@ -3005,6 +3019,7 @@ 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\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);\ndiff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh\nindex b7eed248..45706bde 100755\n--- a/t/t9010-svn-fe.sh\n+++ b/t/t9010-svn-fe.sh\n@@ -271,6 +271,75 @@ test_expect_success PIPE 'directory with files' '\n \ttest_cmp hi directory/file2\n '\n \n+test_expect_success PIPE 'copy from root to directory' '\n+\treinit_git &&\n+\techo hello >hello &&\n+\thello_blob=$(git hash-object -w -t blob hello) &&\n+\tsubtree=$(\n+\t\techo \"100644 blob $hello_blob\tREADME.txt\" |\n+\t\tgit mktree\n+\t) &&\n+\texpect=$(\n+\t\tgit mktree <<-EOF\n+\t\t\t100644 blob $hello_blob\tREADME.txt\n+\t\t\t040000 tree $subtree\ttrunk\n+\t\tEOF\n+\t) &&\n+\n+\t{\n+\t\tproperties \\\n+\t\t\tsvn:author author@example.com \\\n+\t\t\tsvn:date \"2012-10-10T00:01:003.000000Z\" \\\n+\t\t\tsvn:log \"created README.txt\" &&\n+\t\techo PROPS-END\n+\t} >r1.props &&\n+\t{\n+\t\tproperties \\\n+\t\t\tsvn:author author@example.com \\\n+\t\t\tsvn:date \"2012-10-10T00:02:005.000000Z\" \\\n+\t\t\tsvn:log \"created trunk\" &&\n+\t\techo PROPS-END\n+\t} >r2.props &&\n+\t{\n+\t\tcat <<-EOF &&\n+\t\tSVN-fs-dump-format-version: 3\n+\n+\t\tRevision-number: 1\n+\t\tEOF\n+\t\techo Prop-content-length: $(wc -c <r1.props) &&\n+\t\techo Content-length: $(wc -c <r1.props) &&\n+\t\techo &&\n+\t\tcat r1.props &&\n+\t\tcat <<-\\EOF &&\n+\n+\t\tNode-path: README.txt\n+\t\tNode-kind: file\n+\t\tNode-action: add\n+\t\tEOF\n+\t\ttext_no_props hello &&\n+\t\techo Revision-number: 2\n+\t\techo Prop-content-length: $(wc -c <r2.props) &&\n+\t\techo Content-length: $(wc -c <r2.props) &&\n+\t\techo &&\n+\t\tcat r2.props &&\n+\t\tsed -e \"s/X\\$//\" <<-\\EOF\n+\n+\t\tNode-path: trunk\n+\t\tNode-kind: dir\n+\t\tNode-action: add\n+\t\tNode-copyfrom-rev: 1\n+\t\tNode-copyfrom-path: X\n+\t\tProp-content-length: 10\n+\t\tContent-length: 10\n+\n+\t\tPROPS-END\n+\t\tEOF\n+\t} >copy-root.dump &&\n+\ttry_dump copy-root.dump &&\n+\n+\tgit diff-tree --exit-code $expect HEAD\n+'\n+\n test_expect_success PIPE 'branch name with backslash' '\n \treinit_git &&\n \tsort <<-\\EOF >expect.branch-files &&\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 438aaf6b..e5460994 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1047,6 +1047,49 @@ test_expect_success \\\n \t git diff-tree -C --find-copies-harder -r N1^ N1 >actual &&\n \t compare_diff_raw expect actual'\n \n+test_tick\n+cat >input <<INPUT_END\n+commit refs/heads/N-root-to-subdir\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+copy to subdir\n+COMMIT\n+\n+from refs/heads/branch^0\n+C \"\" subdir\n+\n+INPUT_END\n+\n+cat >expect <<\\EOF\n+:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100\tfile2/newf\tsubdir/file2/newf\n+:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100\tfile2/oldf\tsubdir/file2/oldf\n+:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100\tfile4\tsubdir/file4\n+:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100\tnewdir/exec.sh\tsubdir/newdir/exec.sh\n+:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100\tnewdir/interesting\tsubdir/newdir/interesting\n+EOF\n+test_expect_success \\\n+\t'N: copy with empty source path' \\\n+\t'git fast-import <input &&\n+\t git diff-tree -C -C -r --no-commit-id N-root-to-subdir >actual &&\n+\t compare_diff_raw expect actual'\n+\n+test_tick\n+cat >input <<INPUT_END\n+commit refs/heads/N-unquoted-root-to-subdir\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+increase nesting\n+COMMIT\n+\n+from refs/heads/branch^0\n+C  subdir\n+\n+INPUT_END\n+test_expect_success \\\n+\t'N: copy with unquoted empty source path' \\\n+\t'git fast-import <input &&\n+\t git diff --exit-code N-root-to-subdir N-unquoted-root-to-subdir'\n+\n cat >input <<INPUT_END\n commit refs/heads/N2\n committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -1400,6 +1443,137 @@ 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: copying unborn branch root has no effect' '\n+\tempty_tree=$(git mktree </dev/null) &&\n+\techo tree $empty_tree >expect &&\n+\tgit fast-import <<-EOF &&\n+\tcommit refs/heads/N-copy-unborn\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tcopy empty root directory\n+\tCOMMIT\n+\n+\tC \"\" subdir\n+\tEOF\n+\tgit cat-file commit N-copy-unborn >cmit &&\n+\thead -n1 cmit >actual &&\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: copying empty root has no effect' '\n+\tempty_tree=$(git mktree </dev/null) &&\n+\techo tree $empty_tree >expect &&\n+\techo empty >msg &&\n+\tcmit=$(git commit-tree \"$empty_tree\" -p refs/heads/branch^0 <msg) &&\n+\tgit fast-import <<-EOF &&\n+\tcommit refs/heads/N-copy-empty\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tcopy empty root directory\n+\tCOMMIT\n+\n+\tC \"\" subdir\n+\tEOF\n+\tgit cat-file commit N-copy-empty >cmit &&\n+\thead -n1 cmit >actual &&\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_success '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^{tree}) &&\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 directory 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+\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+\t:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100\tnewdir/exec.sh\tfile3/newdir/exec.sh\n+\t:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100\tnewdir/interesting\tfile3/newdir/interesting\n+\tEOF\n+\tgit update-ref -d refs/heads/N12 &&\n+\trm -f backflow &&\n+\tmkfifo backflow &&\n+\t(\n+\t\texec <backflow &&\n+\t\tcat <<-EOF &&\n+\t\tcommit refs/heads/N12\n+\t\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\t\tdata <<COMMIT\n+\t\tcopy root directory by tree hash read via ls\n+\t\tCOMMIT\n+\n+\t\tfrom refs/heads/branch^0\n+\t\tls \"\"\n+\t\tEOF\n+\t\tread mode type tree filename &&\n+\t\techo \"M 040000 $tree file3\"\n+\t) |\n+\tgit fast-import --cat-blob-fd=3 3>backflow &&\n+\tgit diff-tree -C --find-copies-harder -r N12^ N12 >actual &&\n+\tcompare_diff_raw expect actual\n+'\n+\n ###\n ### series O\n ###\n-- \n1.7.9.2\n"},{"id":"186618","messageId":"20120310090132.GF1992@burratino","threadId":"29878","inReplyTo":"20120310085354.GE1992@burratino","subject":"Re: [PATCH v3] fast-import: allow 'ls' and filecopy to read the root","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T09:01:33Z","receivedAt":"2012-03-10T09:01:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> It has a semantic conflict with the fast-import-ls-fixes series that\n> I sent separately\n[...]\n> I'll send a fixup patch as a reply, for squashing into the merge or\n> this patch, whichever is the first commit that contains both topics.\n\nWith this tweak, the merge passes the merged set of tests.\n\ndiff --git i/fast-import.c c/fast-import.c\nindex 51cdda29..fe1c8643 100644\n--- i/fast-import.c\n+++ c/fast-import.c\n@@ -1658,8 +1658,6 @@ static int tree_content_get(\n \t\tn = slash1 - p;\n \telse\n \t\tn = strlen(p);\n-\tif (!n)\n-\t\tdie(\"Empty path component found in input\");\n \n \tif (!root->tree)\n \t\tload_tree(root);\n@@ -1669,6 +1667,8 @@ static int tree_content_get(\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\tgoto last_component;\n+\t\t\tif (!slash1[1])\n+\t\t\t\tdie(\"Empty path component found in input\");\n \t\t\tif (!S_ISDIR(e->versions[1].mode))\n \t\t\t\treturn 0;\n \t\t\tif (!e->tree)\n"},{"id":"186620","messageId":"20120310091812.GG1992@burratino","threadId":"29878","inReplyTo":"20120310085354.GE1992@burratino","subject":"[PATCH 2/1] fixup! fast-import: allow 'ls' and filecopy to read the root","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T09:18:12Z","receivedAt":"2012-03-10T09:18:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2369,6 +2382,7 @@ static void file_change_cr(struct branch *b, int rename)\n>  \t\t\tleaf.tree);\n>  \t\treturn;\n>  \t}\n> +\n>  \ttree_content_set(&b->branch_tree, d,\n>  \t\tleaf.versions[1].sha1,\n>  \t\tleaf.versions[1].mode,\n\nMaybe next time I will send the patch to myself and bounce it to the\nlist.  Sorry for the noise.\n\ndiff --git i/fast-import.c w/fast-import.c\nindex 51cdda29..013cbd5e 100644\n--- i/fast-import.c\n+++ w/fast-import.c\n@@ -2384,7 +2384,6 @@ static void file_change_cr(struct branch *b, int rename)\n \t\t\tleaf.tree);\n \t\treturn;\n \t}\n-\n \ttree_content_set(&b->branch_tree, d,\n \t\tleaf.versions[1].sha1,\n \t\tleaf.versions[1].mode,\n"},{"id":"186621","messageId":"20120310094803.GA1969@burratino","threadId":"29878","inReplyTo":"CA+gfSn8bh-tV+uduM7xsuwqXQW2a57yvVmRXjXjp9JaO779bUg@mail.gmail.com","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-10T09:48:03Z","receivedAt":"2012-03-10T09:48:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Dmitry Ivankov wrote:\n\n>>  One more quick thought. \"force the root mode to S_IFDIR\" part doesn't\n>>  look obviously good for me. First, isn't it already ensured that the\n>>  root is a directory?\n[...]\n> It was just a problematic and incomplete version of what your \"be\n> saner with temporary trees\" does properly. \n\nTo tie up this loose end: looks like David's patch was ok in this\nrespect and my worries unfounded.  What I was missing is that\nstore_tree() does nothing unless its tree argument is dirty, and the\ntemporary tree used to repesent <treeish> in \"ls <treeish> <path>\" is\nnever dirty.\n\nOf course, the reminder of the \"be saner\" patch and the tree delta\ndiscussion was still very useful.\n\nThanks for your thoughtfulness.\nJonathan\n"},{"id":"221609","messageId":"loom.20130621T183212-109@post.gmane.org","threadId":"29878","inReplyTo":"20120310031228.GA3008@burratino","subject":"Re: [PATCH] fast-import: fix ls command with empty path","fromName":"Dave Abrahams","fromEmail":"dave@boostpro.com","sentAt":"2013-06-21T16:33:19Z","receivedAt":"2013-06-21T16:33:19Z","isPatch":true,"sender":{"key":"dave@boostpro.com","avatar":"https://gravatar.com/avatar/df0921f05114687777894565de21c052fb137ba7c303a399528b43d08833f065?d=mp&s=160"},"body":"Jonathan Nieder <jrnieder <at> gmail.com> writes:\n\n> \n> After sleeping on it, here are two patches for 'maint'.  One plugs a\n> memory leak.  The other makes my above comment actually true, so\n> trying to use this missing feature results in an error message that\n> can help the frontend author instead of the silently broken conversion\n> Andrew found.\n> \n> Then we can carefully add 'ls \"\"' support in 1.7.11.\n\nThe support for 'ls \"\"' was nevre actually added, was it?\n"}]}