{"thread":{"id":"51369","subject":"[BUG] Symbolic links break \"git fast-export\"?","startedAt":"2019-06-24T11:03:35Z","lastAt":"2019-07-01T18:02:33Z","messageCount":9,"participants":["Lars Schneider","Elijah Newren","Jeff King","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"377861","messageId":"95EF0665-9882-4707-BB6A-94182C01BE91@gmail.com","threadId":"51369","inReplyTo":null,"subject":"[BUG] Symbolic links break \"git fast-export\"?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2019-06-24T11:03:29Z","receivedAt":"2019-06-24T11:03:35Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi folks,\n\nIs my understanding correct, that `git fast-export | git fast-import` \nshould not modify the repository? If yes, then we might have a bug in \n`git fast-export` if symbolic directory links are removed and converted \nto a real directory.\n\nConsider this test case:\n\n    # Create test repo\n    git init .\n    mkdir foo\n    echo \"foo\" >foo/baz\n    git add .\n    git commit -m \"add foo dir\"\n    ln -s foo bar\n    git add .\n    git commit -m \"add bar dir as link\"\n    rm bar\n    mkdir bar\n    echo \"bar\" >bar/baz\n    git add .\n    git commit -m \"remove link and make bar dir real\"\n\n    printf \"BEFORE: \"\n    git rev-parse HEAD\n\n    # Fast export, import ... that should not change anything!\n    git fast-export --no-data --all --signed-tags=warn-strip \\\n        --tag-of-filtered-object=rewrite | git fast-import --force --quiet\n\n    printf \"AFTER: \"\n\nI would assume that the BEFORE/AFTER hashes match. Unfortunately, with \nGit 2.22.0 they do no. The problem is this export output I think:\n\n    remove link and make bar dir real\n    from :2\n    M 100644 5716ca5987cbf97d6bb54920bea6adde242d87e6 bar/baz\n    D bar\n\nThe new file in the `bar` directory is added to the repo first and\nafterwards the path `bar` is deleted. I think that deletes the entire \ndirectory `bar`?\n\nIf you confirm that this is a bug, then I will try to provide a fix.\n\nThanks,\nLars\n"},{"id":"377868","messageId":"CABPp-BE8um5g98jqWawsuG2dAvO6AZcR54vrRzAkJbq+L3K6Zw@mail.gmail.com","threadId":"51369","inReplyTo":"95EF0665-9882-4707-BB6A-94182C01BE91@gmail.com","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-06-24T12:33:38Z","receivedAt":"2019-06-24T12:33:51Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jun 24, 2019 at 5:05 AM Lars Schneider <larsxschneider@gmail.com> wrote:\n>\n> Hi folks,\n>\n> Is my understanding correct, that `git fast-export | git fast-import`\n> should not modify the repository? If yes, then we might have a bug in\n> `git fast-export` if symbolic directory links are removed and converted\n> to a real directory.\n>\n> Consider this test case:\n>\n>     # Create test repo\n>     git init .\n>     mkdir foo\n>     echo \"foo\" >foo/baz\n>     git add .\n>     git commit -m \"add foo dir\"\n>     ln -s foo bar\n>     git add .\n>     git commit -m \"add bar dir as link\"\n>     rm bar\n>     mkdir bar\n>     echo \"bar\" >bar/baz\n>     git add .\n>     git commit -m \"remove link and make bar dir real\"\n>\n>     printf \"BEFORE: \"\n>     git rev-parse HEAD\n>\n>     # Fast export, import ... that should not change anything!\n>     git fast-export --no-data --all --signed-tags=warn-strip \\\n>         --tag-of-filtered-object=rewrite | git fast-import --force --quiet\n>\n>     printf \"AFTER: \"\n>\n> I would assume that the BEFORE/AFTER hashes match. Unfortunately, with\n> Git 2.22.0 they do no. The problem is this export output I think:\n>\n>     remove link and make bar dir real\n>     from :2\n>     M 100644 5716ca5987cbf97d6bb54920bea6adde242d87e6 bar/baz\n>     D bar\n>\n> The new file in the `bar` directory is added to the repo first and\n> afterwards the path `bar` is deleted. I think that deletes the entire\n> directory `bar`?\n>\n> If you confirm that this is a bug, then I will try to provide a fix.\n\nMy first reaction was, \"we regressed on this again?\", but it looks\nlike my original fix for directory/file changes only handled one\ndirection.  Thus, my commit 060df6242281 (\"fast-export: Fix output\norder of D/F changes\", 2010-07-09) probably *caused* this bug.  We\nshould probably just sort not based on filename, but on changetype --\nsend all the deletes to fast-import before we send the modifies.\n\nWe should probably also make a corresponding improvement to\nfast-import; it also makes some attempts to be smart about handling\norder of modifies and deletes, but misses this case.  See commit\n253fb5f8897d (\"fast-import: Improve robustness when D->F changes\nprovided in wrong order\", 2010-07-09).  It'd be nice if fast-import\ncould go through the list of changes, apply the deletes first, then\nthe modifies -- although I'm not sure where renames go in the order\noff the top of my head.\n\nThanks for flagging this and working on it.\n\nElijah\n"},{"id":"377873","messageId":"CABPp-BFLJ48BZ97Y9mr4i3q7HMqjq18cXMgSYdxqD1cMzH8Spg@mail.gmail.com","threadId":"51369","inReplyTo":"95EF0665-9882-4707-BB6A-94182C01BE91@gmail.com","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-06-24T12:55:15Z","receivedAt":"2019-06-24T12:55:29Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jun 24, 2019 at 5:05 AM Lars Schneider <larsxschneider@gmail.com> wrote:\n>\n> Hi folks,\n>\n> Is my understanding correct, that `git fast-export | git fast-import`\n> should not modify the repository?\n\nI forgot to respond to this part.  The answer is mostly yes, but actually no:\n\n* fast-export strips any extended commit headers, if any (it only\nlooks for expected headers and handles those)\n* fast-export omits any tags of trees\n* fast-export sometimes rewrites tags of tags of commits to tags of\ncommits (depends on options passed; it either mistakenly does this, or\ndies with an error)\n* fast-export defaults to re-encoding commits to be in UTF-8, with no\noption in git <= 2.22.0 to leave the encoding alone\n* annotated tags outside of the refs/tags/ namespace will have their\nlocation mangled\n* references that include commit cycles in their history (which can be\ncreated with git-replace(1)) will not be flagged to the user as an\nerror but will be silently deleted by fast-export as though the branch\nor tag contained no interesting files\n* fast-export or fast-import may die on some repositories (e.g. if\nthere are tags of blobs, if there are signed tags and no\n--signed-tags= option passed, if a commit has improperly formatted\ninfo such as a timezone with more than four digits (which exist in the\nwild, e.g. in the rails repo, and google will find you more), or in\nlatest git master if the commit has a recorded encoding and no\n--reencode option is passed)\n* ...and the bug you just found.\n\nIt's a valid question whether these are bugs or not; I'd say that some\ndefinitely are, but not all.  I had to document all of these up at\nhttps://github.com/newren/git-filter-repo#inherited-limitations, among\none or two others based on the options I default to passing to\nfast-export.\n\nHope that helps,\nElijah\n"},{"id":"377922","messageId":"20190624185835.GA11720@sigill.intra.peff.net","threadId":"51369","inReplyTo":"CABPp-BE8um5g98jqWawsuG2dAvO6AZcR54vrRzAkJbq+L3K6Zw@mail.gmail.com","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-24T18:58:35Z","receivedAt":"2019-06-24T18:58:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 24, 2019 at 06:33:38AM -0600, Elijah Newren wrote:\n\n> We should probably also make a corresponding improvement to\n> fast-import; it also makes some attempts to be smart about handling\n> order of modifies and deletes, but misses this case.  See commit\n> 253fb5f8897d (\"fast-import: Improve robustness when D->F changes\n> provided in wrong order\", 2010-07-09).  It'd be nice if fast-import\n> could go through the list of changes, apply the deletes first, then\n> the modifies -- although I'm not sure where renames go in the order\n> off the top of my head.\n\nYou'd have to split the renames into separate delete/adds, since they\ncan have a circular dependency. E.g. renaming \"foo\" to \"bar\" and \"bar\"\nto \"foo\", you must remove \"foo\" and \"bar\" both, and then add them back\nin.\n\n-Peff\n"},{"id":"378343","messageId":"25624E1D-55F1-466D-92E0-F06C1909F920@gmail.com","threadId":"51369","inReplyTo":"20190624185835.GA11720@sigill.intra.peff.net","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2019-06-30T13:05:29Z","receivedAt":"2019-06-30T13:06:13Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n\n> On Jun 24, 2019, at 11:58 AM, Jeff King <peff@peff.net> wrote:\n> \n> On Mon, Jun 24, 2019 at 06:33:38AM -0600, Elijah Newren wrote:\n> \n>> We should probably also make a corresponding improvement to\n>> fast-import; it also makes some attempts to be smart about handling\n>> order of modifies and deletes, but misses this case.  See commit\n>> 253fb5f8897d (\"fast-import: Improve robustness when D->F changes\n>> provided in wrong order\", 2010-07-09).  It'd be nice if fast-import\n>> could go through the list of changes, apply the deletes first, then\n>> the modifies -- although I'm not sure where renames go in the order\n>> off the top of my head.\n> \n> You'd have to split the renames into separate delete/adds, since they\n> can have a circular dependency. E.g. renaming \"foo\" to \"bar\" and \"bar\"\n> to \"foo\", you must remove \"foo\" and \"bar\" both, and then add them back\n> in.\n\n@peff: Can you give me a hint how one would perform this circular\ndependency in a single commit? I try to write a test case for this.\n\nThank you,\nLars\n\n"},{"id":"378344","messageId":"3AA3CC52-84FE-4FD0-8977-D4FBCF0DCE2C@gmail.com","threadId":"51369","inReplyTo":"CABPp-BE8um5g98jqWawsuG2dAvO6AZcR54vrRzAkJbq+L3K6Zw@mail.gmail.com","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2019-06-30T14:01:06Z","receivedAt":"2019-06-30T14:01:09Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n\n> On Jun 24, 2019, at 5:33 AM, Elijah Newren <newren@gmail.com> wrote:\n> \n> On Mon, Jun 24, 2019 at 5:05 AM Lars Schneider <larsxschneider@gmail.com> wrote:\n>> \n>> Hi folks,\n>> \n>> Is my understanding correct, that `git fast-export | git fast-import`\n>> should not modify the repository? If yes, then we might have a bug in\n>> `git fast-export` if symbolic directory links are removed and converted\n>> to a real directory.\n>> \n>> ...\n> \n> My first reaction was, \"we regressed on this again?\", but it looks\n> like my original fix for directory/file changes only handled one\n> direction.  Thus, my commit 060df6242281 (\"fast-export: Fix output\n> order of D/F changes\", 2010-07-09) probably *caused* this bug.  We\n> should probably just sort not based on filename, but on changetype --\n> send all the deletes to fast-import before we send the modifies.\n\n060df6242281 is interesting! If I revert the changes in builtin/fast-export.c,\nthen the \"t9350:directory becomes symlink\" test still passes nowadays. \n\nPlus, my my new test case passes too:\n\n\ttest_expect_success 'when transforming a symlink to a directory' '\n\t\ttest_create_repo src &&\n\n\t   \tmkdir src/foo &&\n\t\techo a_line >src/foo/file.txt &&\n\t\tgit -C src add foo/file.txt &&\n\t\tgit -C src commit -m 1st_commit &&\n\n\t\tln -s src/foo src/bar &&\n\t\tgit -C src add bar &&\n\t\tgit -C src commit -m 2nd_commit &&\n\n\t\trm src/bar &&\n\t   \tmkdir src/bar &&\n\t   \techo b_line >src/bar/b_file.txt &&\n\t\tgit -C src add . &&\n\t\tgit -C src commit -m 3rd_commit &&\n\n\t\ttest_create_repo dst &&\n\t\tgit -C src fast-export --all &&\n\t\tgit -C src fast-export --all | git -C dst fast-import &&\n\t\tgit -C src show >expected &&\n\t\tgit -C dst show >actual &&\n\t\ttest_cmp expected actual\n\t'\n\nDo you think it would make sense to revert the qsort change\nin fast-export? I haven't bisected yet which other change made\nthe qsort change obsolete.\n\nThanks,\nLars"},{"id":"378347","messageId":"5b00da7c-6836-0e61-8262-a9035126b658@kdbg.org","threadId":"51369","inReplyTo":"25624E1D-55F1-466D-92E0-F06C1909F920@gmail.com","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-06-30T18:28:25Z","receivedAt":"2019-06-30T18:28:31Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.06.19 um 15:05 schrieb Lars Schneider:\n>> On Jun 24, 2019, at 11:58 AM, Jeff King <peff@peff.net> wrote:\n>> You'd have to split the renames into separate delete/adds, since they\n>> can have a circular dependency. E.g. renaming \"foo\" to \"bar\" and \"bar\"\n>> to \"foo\", you must remove \"foo\" and \"bar\" both, and then add them back\n>> in.\n> \n> @peff: Can you give me a hint how one would perform this circular\n> dependency in a single commit? I try to write a test case for this.\n\ngit mv Makefile foo\ngit mv COPYING Makefile\ngit mv foo COPYING\ngit diff -B HEAD\n\n-- Hannes\n"},{"id":"378417","messageId":"CABPp-BE0wbgn1OaEH-6TgBaEDrab9C-ZiQLbT9sS8PTf8E=CNQ@mail.gmail.com","threadId":"51369","inReplyTo":"5b00da7c-6836-0e61-8262-a9035126b658@kdbg.org","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-07-01T17:45:52Z","receivedAt":"2019-07-01T17:46:05Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 30, 2019 at 12:28 PM Johannes Sixt <j6t@kdbg.org> wrote:\n>\n> Am 30.06.19 um 15:05 schrieb Lars Schneider:\n> >> On Jun 24, 2019, at 11:58 AM, Jeff King <peff@peff.net> wrote:\n> >> You'd have to split the renames into separate delete/adds, since they\n> >> can have a circular dependency. E.g. renaming \"foo\" to \"bar\" and \"bar\"\n> >> to \"foo\", you must remove \"foo\" and \"bar\" both, and then add them back\n> >> in.\n> >\n> > @peff: Can you give me a hint how one would perform this circular\n> > dependency in a single commit? I try to write a test case for this.\n>\n> git mv Makefile foo\n> git mv COPYING Makefile\n> git mv foo COPYING\n> git diff -B HEAD\n>\n> -- Hannes\n\nInterestingly, fast-export has special code to handle cases like this;\npossibly due to the understanding of how all\nfilemodify/filedelete/filerename commands take effect immediately (see\nbelow for more on that).  If I make the above changes in git.git and\ncommit them, then:\n\n$ git diff --name-status -B HEAD~1 HEAD\nR100    Makefile        COPYING\nR100    COPYING Makefile\n\nBUT:\n\n$ git fast-export -B -M --no-data HEAD~2..HEAD | tail -n 10\ncommit refs/heads/master\nmark :7\nauthor Elijah Newren <newren@gmail.com> 1562000065 -0600\ncommitter Elijah Newren <newren@gmail.com> 1562000065 -0600\ndata 8\nTesting\nfrom :6\nR COPYING Makefile\nM 100644 8a7e2353520ddd7e0c8074d2b32d0441d97c1597 COPYING\n\nI.e. fast-export breaks the rename and translates it into a modify\ninstead.  This comes from here:\n\n        case DIFF_STATUS_COPIED:\n        case DIFF_STATUS_RENAMED:\n                /*\n                 * If a change in the file corresponding to ospec->path\n                 * has been observed, we cannot trust its contents\n                 * because the diff is calculated based on the prior\n                 * contents, not the current contents.  So, declare a\n                 * copy or rename only if there was no change observed.\n                 */\n                if (!string_list_has_string(changed, ospec->path)) {\n                        <snipped code for handling rename/copy>\n                }\n                /* fallthrough */\n        case DIFF_STATUS_TYPE_CHANGED:\n        case DIFF_STATUS_MODIFIED:\n        case DIFF_STATUS_ADDED:\n\nThere is a question of whether fast-import should try to handle\ndifferent exporters that aren't as careful; e.g. if one gets a stream\nlike:\n\ncommit refs/heads/master\nmark :4\nauthor Me My <self@and.eye> 110000000 -0700\ncommitter Me My <self@and.eye> 110000000 -0700\ndata 11\ncorrection\nR letters numbers\nR numbers letters\n\nShould git-fast-import attempt to divine the user's intent to swap\nthese two files (though it's not clear if that is the intent; see\nbelow), or would that violate the documented behavior:\n\n           A filerename command takes effect immediately. Once the source\n           location has been renamed to the destination any future commands\n           applied to the source location will create new files there and not\n           impact the destination of the rename.\n\n(I'm pretty sure Shawn would have said the latter; see e.g.\nhttps://public-inbox.org/git/20100706193455.GA19476@spearce.org/ and\nthe follow-ups.)  I think the view of \"immediately taking effect\"\nimplies that this is a rename of 'letters' to 'numbers' which deletes\n'numbers', and that the subsequent entry just renames the file back,\nmaking it an expensive almost no-op; almost because it has the\nside-effect of deleting the original 'numbers' file.  This is\ncertainly what fast-import does right now.\n\nElijah\n"},{"id":"378419","messageId":"CABPp-BERu_Sd6YYtVPBrjuHz_Yc0_f3rgvYESQB8EuUs-8jCUQ@mail.gmail.com","threadId":"51369","inReplyTo":"3AA3CC52-84FE-4FD0-8977-D4FBCF0DCE2C@gmail.com","subject":"Re: [BUG] Symbolic links break \"git fast-export\"?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-07-01T18:02:19Z","receivedAt":"2019-07-01T18:02:33Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 30, 2019 at 8:01 AM Lars Schneider <larsxschneider@gmail.com> wrote:\n> > On Jun 24, 2019, at 5:33 AM, Elijah Newren <newren@gmail.com> wrote:\n> >\n> > On Mon, Jun 24, 2019 at 5:05 AM Lars Schneider <larsxschneider@gmail.com> wrote:\n> >>\n> >> Hi folks,\n> >>\n> >> Is my understanding correct, that `git fast-export | git fast-import`\n> >> should not modify the repository? If yes, then we might have a bug in\n> >> `git fast-export` if symbolic directory links are removed and converted\n> >> to a real directory.\n> >>\n> >> ...\n> >\n> > My first reaction was, \"we regressed on this again?\", but it looks\n> > like my original fix for directory/file changes only handled one\n> > direction.  Thus, my commit 060df6242281 (\"fast-export: Fix output\n> > order of D/F changes\", 2010-07-09) probably *caused* this bug.  We\n> > should probably just sort not based on filename, but on changetype --\n> > send all the deletes to fast-import before we send the modifies.\n>\n> 060df6242281 is interesting! If I revert the changes in builtin/fast-export.c,\n> then the \"t9350:directory becomes symlink\" test still passes nowadays.\n>\n> Plus, my my new test case passes too:\n>\n>         test_expect_success 'when transforming a symlink to a directory' '\n>                 test_create_repo src &&\n>\n>                 mkdir src/foo &&\n>                 echo a_line >src/foo/file.txt &&\n>                 git -C src add foo/file.txt &&\n>                 git -C src commit -m 1st_commit &&\n>\n>                 ln -s src/foo src/bar &&\n>                 git -C src add bar &&\n>                 git -C src commit -m 2nd_commit &&\n>\n>                 rm src/bar &&\n>                 mkdir src/bar &&\n>                 echo b_line >src/bar/b_file.txt &&\n>                 git -C src add . &&\n>                 git -C src commit -m 3rd_commit &&\n>\n>                 test_create_repo dst &&\n>                 git -C src fast-export --all &&\n>                 git -C src fast-export --all | git -C dst fast-import &&\n>                 git -C src show >expected &&\n>                 git -C dst show >actual &&\n>                 test_cmp expected actual\n>         '\n>\n> Do you think it would make sense to revert the qsort change\n> in fast-export? I haven't bisected yet which other change made\n> the qsort change obsolete.\n\nNo need to bisect; it was the other commit I pointed out in my last\nemail, commit 253fb5f8897d (\"fast-import: Improve robustness when D->F\nchanges\nprovided in wrong order\", 2010-07-09).  The original bug was\nfixed/worked around in two places, because Shawn specifically said the\nfast-import side being more robust was okay but that fast-export was\nbuggy and needed fixing.  See\nhttps://public-inbox.org/git/20100706193455.GA19476@spearce.org/ and\nthe thread around there.  (It'd be really nice to be able to cc Shawn\non this...sigh.)  Reverting the fast-export change would be okay if we\nreplaced it with a better change (something like sorting deletes\nbefore modifies, though maybe with extra work around renames as\ndiscussion elsewhere in this thread is touching on), but if we\nstraight up revert that change it would leave fast-export in violation\nof the stream format.\n\nHope that helps,\nElijah\n"}]}