{"thread":{"id":"40132","subject":"[BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory","startedAt":"2015-08-19T19:46:27Z","lastAt":"2015-08-24T05:25:49Z","messageCount":4,"participants":["Anders Kaseorg","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"268342","messageId":"alpine.DEB.2.10.1508191532330.31851@buzzword-bingo.mit.edu","threadId":"40132","inReplyTo":null,"subject":"[BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2015-08-19T19:46:27Z","receivedAt":"2015-08-19T19:46:27Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"git fast-export | git fast-import fails to preserve a commit that replaces \na symlink with a directory.  Add a failing test case demonstrating this \nbug.\n\nThe fast-export output for the commit in question looks like\n\n  commit refs/heads/master\n  mark :4\n  author …\n  committer …\n  data 4\n  two\n  M 100644 :1 foo/world\n  D foo\n\nfast-import deletes the symlink foo and ignores foo/world.  Swapping the M \nline with the D line would give the correct result.\n\nSigned-off-by: Anders Kaseorg <andersk@mit.edu>\n---\n t/t9350-fast-export.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 66c8b0a..5fb8a04 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -419,6 +419,30 @@ test_expect_success 'directory becomes symlink'        '\n \t(cd result && git show master:foo)\n '\n \n+test_expect_failure 'symlink becomes directory'        '\n+\tgit init symlinktodir &&\n+\tgit init symlinktodirresult &&\n+\t(\n+\t\tcd symlinktodir &&\n+\t\tmkdir bar &&\n+\t\techo hello > bar/world &&\n+\t\ttest_ln_s_add bar foo &&\n+\t\tgit add foo bar/world &&\n+\t\tgit commit -q -mone &&\n+\t\tgit rm foo &&\n+\t\tmkdir foo &&\n+\t\techo hello > foo/world &&\n+\t\tgit add foo/world &&\n+\t\tgit commit -q -mtwo\n+\t) &&\n+\t(\n+\t\tcd symlinktodir &&\n+\t\tgit fast-export master -- foo |\n+\t\t(cd ../symlinktodirresult && git fast-import --quiet)\n+\t) &&\n+\t(cd symlinktodirresult && git show master:foo)\n+'\n+\n test_expect_success 'fast-export quotes pathnames' '\n \tgit init crazy-paths &&\n \t(cd crazy-paths &&\n-- \n2.5.0\n"},{"id":"268425","messageId":"20150821145827.GA565@sigill.intra.peff.net","threadId":"40132","inReplyTo":"alpine.DEB.2.10.1508191532330.31851@buzzword-bingo.mit.edu","subject":"Re: [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-21T14:58:27Z","receivedAt":"2015-08-21T14:58:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 19, 2015 at 03:46:27PM -0400, Anders Kaseorg wrote:\n\n> git fast-export | git fast-import fails to preserve a commit that replaces \n> a symlink with a directory.  Add a failing test case demonstrating this \n> bug.\n> \n> The fast-export output for the commit in question looks like\n> \n>   commit refs/heads/master\n>   mark :4\n>   author …\n>   committer …\n>   data 4\n>   two\n>   M 100644 :1 foo/world\n>   D foo\n> \n> fast-import deletes the symlink foo and ignores foo/world.  Swapping the M \n> line with the D line would give the correct result.\n\nThanks for providing a patch to the tests. That is my favorite form of\nbug report. :)\n\nThe problem seems to be that we output the entries in a \"depth first\"\nway; \"foo/bar\" always comes before \"foo\", to cover the cases explained\nin 060df62 (fast-export: Fix output order of D/F changes, 2010-07-09).\n\nI'm tempted to say we would want to do all deletions (at any level)\nfirst, to make room for new files. That patch looks like:\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d23f3be..336fd6f 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -267,6 +267,14 @@ static int depth_first(const void *a_, const void *b_)\n \tconst char *name_a, *name_b;\n \tint len_a, len_b, len;\n \tint cmp;\n+\tint deletion;\n+\n+\t/*\n+\t * Move all deletions first, to make room for any later modifications.\n+\t */\n+\tdeletion = (b->status == 'D') - (a->status == 'D');\n+\tif (deletion)\n+\t\treturn deletion;\n \n \tname_a = a->one ? a->one->path : a->two->path;\n \tname_b = b->one ? b->one->path : b->two->path;\n\n\nand does indeed pass your test. But I'm not sure that covers all cases,\nand I'm not sure it doesn't make some cases worse:\n\n  - if we moved a deletion to the front, is it possible for that path to\n    have been the source side of a copy or rename, that is now broken? I\n    don't _think_ so. If it's a copy, then the file by definition cannot\n    also be deleted (that would make it a rename, not a copy). We could\n    have a copy along with a rename, but again, then we don't have a\n    delete (we have a rename, which is explicitly bumped to the end for\n    this reason).\n\n  - we may still have the opposite problem with renames. That is, a\n    rename is _also_ a deletion, but will go to the end. So I would\n    expect renaming the symlink \"foo\" to \"bar\" and then adding\n    \"foo/world\" would end up with:\n\n       M 100644 :3 foo/world\n       R foo bar\n\n    (because we push renames to the end in our sort). And indeed,\n    importing that does seem to get it wrong (we end up with \"bar/world\"\n    and no symlink).\n\nWe can't fix the ordering in the second case without breaking the first\ncase. So I'm not sure it's fixable on the fast-export end.\n\n-Peff\n"},{"id":"268434","messageId":"alpine.DEB.2.10.1508211238570.31851@buzzword-bingo.mit.edu","threadId":"40132","inReplyTo":"20150821145827.GA565@sigill.intra.peff.net","subject":"Re: [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2015-08-21T16:47:30Z","receivedAt":"2015-08-21T16:47:30Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Fri, 21 Aug 2015, Jeff King wrote:\n>   - we may still have the opposite problem with renames. That is, a\n>     rename is _also_ a deletion, but will go to the end. So I would\n>     expect renaming the symlink \"foo\" to \"bar\" and then adding\n>     \"foo/world\" would end up with:\n> \n>        M 100644 :3 foo/world\n>        R foo bar\n> \n>     (because we push renames to the end in our sort). And indeed,\n>     importing that does seem to get it wrong (we end up with \"bar/world\"\n>     and no symlink).\n> \n> We can't fix the ordering in the second case without breaking the first\n> case. So I'm not sure it's fixable on the fast-export end.\n\nHmm, renames have a more fundamental ordering problem: swapping two \n(normal) files and using fast-export -C -B results in\n\n  R foo bar\n  R bar foo\n\nwhich cannot be reimported correctly without fast-import fixes.\n\nAnders\n"},{"id":"268552","messageId":"20150824052548.GA14403@sigill.intra.peff.net","threadId":"40132","inReplyTo":"alpine.DEB.2.10.1508211238570.31851@buzzword-bingo.mit.edu","subject":"Re: [BUG/PATCH] t9350-fast-export: Add failing test for symlink-to-directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-24T05:25:49Z","receivedAt":"2015-08-24T05:25:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 21, 2015 at 12:47:30PM -0400, Anders Kaseorg wrote:\n\n> On Fri, 21 Aug 2015, Jeff King wrote:\n> >   - we may still have the opposite problem with renames. That is, a\n> >     rename is _also_ a deletion, but will go to the end. So I would\n> >     expect renaming the symlink \"foo\" to \"bar\" and then adding\n> >     \"foo/world\" would end up with:\n> > \n> >        M 100644 :3 foo/world\n> >        R foo bar\n> > \n> >     (because we push renames to the end in our sort). And indeed,\n> >     importing that does seem to get it wrong (we end up with \"bar/world\"\n> >     and no symlink).\n> > \n> > We can't fix the ordering in the second case without breaking the first\n> > case. So I'm not sure it's fixable on the fast-export end.\n> \n> Hmm, renames have a more fundamental ordering problem: swapping two \n> (normal) files and using fast-export -C -B results in\n> \n>   R foo bar\n>   R bar foo\n> \n> which cannot be reimported correctly without fast-import fixes.\n\nYeah, you're right. Fast-export's view of the world comes from diff,\nwhich is that the \"source\" side is immutable. Whereas fast-import seems\nto mutate the tree in-place as it reads the set of operations. I wonder\nwhat would break if we simply fixed that. I.e., is anybody else\ndepending on:\n\n  R foo bar\n  M bar ...\n\nto modify \"foo\" and not \"bar\". I kind of wonder if it is insane to turn\non renames at all in fast-export.\n\n-Peff\n"}]}