{"thread":{"id":"59422","subject":"bug? round-trip through fast-import/fast-export loses files","startedAt":"2023-03-20T18:13:46Z","lastAt":"2023-03-22T03:08:12Z","messageCount":6,"participants":["Priedhorsky, Reid","Elijah Newren","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"473794","messageId":"BBB169A5-0665-47C9-819B-6409A22AB699@lanl.gov","threadId":"59422","inReplyTo":null,"subject":"bug? round-trip through fast-import/fast-export loses files","fromName":"Priedhorsky, Reid","fromEmail":"reidpr@lanl.gov","sentAt":"2023-03-20T17:10:56Z","receivedAt":"2023-03-20T18:13:46Z","isPatch":false,"sender":{"key":"reidpr@lanl.gov","avatar":null},"body":"  Hello,\n\n  I believe I’ve found a bug in Git. It seems that (1) round-tripping through\n  fast-export/fast-import a repository (2) that contains a commit that changes\n  a file to a directory (3) deletes the contents of that directory from the\n  repository.\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n  Run this shell script:\n\n  ~~~~\n  #!/bin/bash\n\n  set -ex\n\n  mkdir -p /tmp/weirdal\n  cd /tmp/weirdal\n  git --version\n\n  # init repo\n  rm -Rf wd\n  mkdir wd\n  cd wd\n  git init -b main\n\n  # first commit - foo is a file\n  touch foo\n  git add -A\n  git commit -m 'file'\n\n  # second commit - foo is a directory\n  rm foo\n  mkdir foo\n  touch foo/bar\n  git add -A\n  git commit -m 'directory'\n\n  # the contents of foo are in the working dir and the repo\n  git status\n  ls -lR\n  git ls-tree --name-only -r HEAD\n\n  # import/export repository (add --full-tree to work around bug)\n  git fast-export --no-data -- --all > ../export\n  cat ../export\n  git fast-import --force --quiet < ../export\n\n  # bug: foo is still in the WD but not the repo; should still be both\n  git status\n  ls -lR\n  git ls-tree --name-only -r HEAD\n  #git fast-export --no-data -- --all | diff -u --text ../export - || true\n  ~~~~\n\nWhat did you expect to happen? (Expected behavior)\n\n  Repo should be unchanged, i.e.:\n\n  + git status\n  On branch main\n  nothing to commit, working tree clean\n\nWhat happened instead? (Actual behavior)\n\n  Git thinks foo/bar has been staged:\n\n  + git status\n  On branch main\n  Changes to be committed:\n    (use \"git restore --staged <file>...\" to unstage)\n          new file:   foo/bar\n\nWhat's different between what you expected and what actually happened?\n\n  File foo/bar is staged when it should be unchanged.\n\nAnything else you want to add:\n\n  This also happens in 2.38.1 built from source.\n\n  The bad behavior can be worked around with “--full-tree” on fast-export, but\n  the real repo where I want to do this is pretty large, so I’d prefer not to.\n\n  Note the “git fast-export” output:\n\n    commit refs/heads/main\n    mark :2\n    author Reid Priedhorsky <reidpr@lanl.gov> 1679330805 -0600\n    committer Reid Priedhorsky <reidpr@lanl.gov> 1679330805 -0600\n    data 10\n    directory\n    from :1\n    M 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 foo/bar\n    D foo\n\n  It looks to me like the “M ... foo/bar” is being processed before “D foo”\n  when it should happen in the opposite order.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.40.0.71.g950264636c\ncpu: x86_64\nbuilt from commit: 950264636c68591989456e3ba0a5442f93152c1a\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.10.0-21-amd64 #1 SMP Debian 5.10.162-1 (2023-01-21) x86_64\ncompiler info: gnuc: 10.2\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\nThanks,\nReid\n\n—\nhe/his\n\n"},{"id":"473821","messageId":"CABPp-BEG+vp-UcpVfcZecPBnfcuTjO6JYCo7wEU5ZrDUHBUd9g@mail.gmail.com","threadId":"59422","inReplyTo":"BBB169A5-0665-47C9-819B-6409A22AB699@lanl.gov","subject":"Re: bug? round-trip through fast-import/fast-export loses files","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-03-21T01:57:21Z","receivedAt":"2023-03-21T01:57:39Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Mon, Mar 20, 2023 at 11:23 AM Priedhorsky, Reid <reidpr@lanl.gov> wrote:\n>\n>   Hello,\n>\n>   I believe I’ve found a bug in Git. It seems that (1) round-tripping through\n>   fast-export/fast-import a repository (2) that contains a commit that changes\n>   a file to a directory (3) deletes the contents of that directory from the\n>   repository.\n>\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n>   Run this shell script:\n>\n>   ~~~~\n>   #!/bin/bash\n>\n>   set -ex\n>\n>   mkdir -p /tmp/weirdal\n>   cd /tmp/weirdal\n>   git --version\n>\n>   # init repo\n>   rm -Rf wd\n>   mkdir wd\n>   cd wd\n>   git init -b main\n>\n>   # first commit - foo is a file\n>   touch foo\n>   git add -A\n>   git commit -m 'file'\n>\n>   # second commit - foo is a directory\n>   rm foo\n>   mkdir foo\n>   touch foo/bar\n>   git add -A\n>   git commit -m 'directory'\n>\n>   # the contents of foo are in the working dir and the repo\n>   git status\n>   ls -lR\n>   git ls-tree --name-only -r HEAD\n>\n>   # import/export repository (add --full-tree to work around bug)\n>   git fast-export --no-data -- --all > ../export\n>   cat ../export\n>   git fast-import --force --quiet < ../export\n>\n>   # bug: foo is still in the WD but not the repo; should still be both\n>   git status\n>   ls -lR\n>   git ls-tree --name-only -r HEAD\n>   #git fast-export --no-data -- --all | diff -u --text ../export - || true\n>   ~~~~\n>\n> What did you expect to happen? (Expected behavior)\n>\n>   Repo should be unchanged, i.e.:\n>\n>   + git status\n>   On branch main\n>   nothing to commit, working tree clean\n>\n> What happened instead? (Actual behavior)\n>\n>   Git thinks foo/bar has been staged:\n>\n>   + git status\n>   On branch main\n>   Changes to be committed:\n>     (use \"git restore --staged <file>...\" to unstage)\n>           new file:   foo/bar\n>\n> What's different between what you expected and what actually happened?\n>\n>   File foo/bar is staged when it should be unchanged.\n>\n> Anything else you want to add:\n>\n>   This also happens in 2.38.1 built from source.\n>\n>   The bad behavior can be worked around with “--full-tree” on fast-export, but\n>   the real repo where I want to do this is pretty large, so I’d prefer not to.\n>\n>   Note the “git fast-export” output:\n>\n>     commit refs/heads/main\n>     mark :2\n>     author Reid Priedhorsky <reidpr@lanl.gov> 1679330805 -0600\n>     committer Reid Priedhorsky <reidpr@lanl.gov> 1679330805 -0600\n>     data 10\n>     directory\n>     from :1\n>     M 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 foo/bar\n>     D foo\n>\n>   It looks to me like the “M ... foo/bar” is being processed before “D foo”\n>   when it should happen in the opposite order.\n\nThanks for the well-written bug report, including not only a testcase\nbut even the relevant bits of the fast-export output.  I thought I had\nfixed D/F issues in fast-export & fast-import before, and indeed a\nsearch turns up both of\n\n253fb5f889 (fast-import: Improve robustness when D->F changes provided\nin wrong order, 2010-07-09)\n060df62422 (fast-export: Fix output order of D/F changes, 2010-07-09)\n\nHowever, it looks like both of those only considered D->F (directory\nbecomes a file) changes, whereas you specifically have a case of F->D\n(file becoming a directory).\n\nHonestly, looking back at those two patches of mine, I think both were\nrather suboptimal.  A better solution that would handle both F->D and\nD->F would be having fast-export sort the diff_filepairs such that it\nprocesses the deletes before the modifies.  Another improved solution\nwould be having fast-import sort the files given to it and handling\ndeletes first.  Either should fix this.\n\nMight be a good task for a new contributor.  Any takers?  (Tagging as\n#leftoverbits.)\n"},{"id":"473844","messageId":"6486D136-23D8-4C90-AEDA-DD037A5CD2B5@lanl.gov","threadId":"59422","inReplyTo":"CABPp-BEG+vp-UcpVfcZecPBnfcuTjO6JYCo7wEU5ZrDUHBUd9g@mail.gmail.com","subject":"Re: bug? round-trip through fast-import/fast-export loses files","fromName":"Priedhorsky, Reid","fromEmail":"reidpr@lanl.gov","sentAt":"2023-03-21T15:54:03Z","receivedAt":"2023-03-21T15:54:15Z","isPatch":false,"sender":{"key":"reidpr@lanl.gov","avatar":null},"body":"\n> On Mar 20, 2023, at 7:57 PM, Elijah Newren <newren@gmail.com> wrote:\n> \n> Thanks for the well-written bug report, including not only a testcase\n> but even the relevant bits of the fast-export output.\n\n😁\n\n> A better solution that would handle both F->D and\n> D->F would be having fast-export sort the diff_filepairs such that it\n> processes the deletes before the modifies.\n\nIn fact, the successful workaround in my own code is to re-order the fast-export output in exactly this way (i.e., sort the lines so all the D lines come before all the M lines).\n\n> Another improved solution\n> would be having fast-import sort the files given to it and handling\n> deletes first.  Either should fix this.\n> \n> Might be a good task for a new contributor.  Any takers?  (Tagging as\n> #leftoverbits.)\n\nLet me take a look, but I can’t guarantee anything.\n\nHTH,\nReid\n\n—\nhe/his\n\n"},{"id":"473850","messageId":"xmqqh6uejamu.fsf@gitster.g","threadId":"59422","inReplyTo":"CABPp-BEG+vp-UcpVfcZecPBnfcuTjO6JYCo7wEU5ZrDUHBUd9g@mail.gmail.com","subject":"Re: bug? round-trip through fast-import/fast-export loses files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-21T17:07:53Z","receivedAt":"2023-03-21T17:09:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Honestly, looking back at those two patches of mine, I think both were\n> rather suboptimal.  A better solution that would handle both F->D and\n> D->F would be having fast-export sort the diff_filepairs such that it\n> processes the deletes before the modifies.  Another improved solution\n> would be having fast-import sort the files given to it and handling\n> deletes first.  Either should fix this.\n\nReminds me of the trick we had to invent for \"git apply\", where we\nprocess deletions first and then creation, to avoid the exact\nproblem of creating \"foo\" while removing \"foo/bar\" ;-)\n"},{"id":"473872","messageId":"20230321183138.GL3119834@coredump.intra.peff.net","threadId":"59422","inReplyTo":"CABPp-BEG+vp-UcpVfcZecPBnfcuTjO6JYCo7wEU5ZrDUHBUd9g@mail.gmail.com","subject":"Re: bug? round-trip through fast-import/fast-export loses files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T18:31:38Z","receivedAt":"2023-03-21T18:31:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 06:57:21PM -0700, Elijah Newren wrote:\n\n> Honestly, looking back at those two patches of mine, I think both were\n> rather suboptimal.  A better solution that would handle both F->D and\n> D->F would be having fast-export sort the diff_filepairs such that it\n> processes the deletes before the modifies.  Another improved solution\n> would be having fast-import sort the files given to it and handling\n> deletes first.  Either should fix this.\n> \n> Might be a good task for a new contributor.  Any takers?  (Tagging as\n> #leftoverbits.)\n\nThere was a patch a while ago, but it didn't get applied:\n\n  https://lore.kernel.org/git/1493079137-1838-1-git-send-email-miguel.torroja@gmail.com/\n\nIt got hung up on the fact that fast-export can also generate renames,\nand ordering there is tricky. I stand by the sentiment from back then\nthat it is still worth it to order things to make the no-rename case\nwork, even if there are still corner cases with renames (since you can\nhave a cycle of renames, you should not use them if you want to be\nrobust against this kind of ordering dependency).\n\n-Peff\n"},{"id":"473897","messageId":"CABPp-BGvC9wGwUYPGLXkPZH-stg9KxwFFJSCk2dXhDitOFCWoA@mail.gmail.com","threadId":"59422","inReplyTo":"20230321183138.GL3119834@coredump.intra.peff.net","subject":"Re: bug? round-trip through fast-import/fast-export loses files","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-03-22T03:07:50Z","receivedAt":"2023-03-22T03:08:12Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 21, 2023 at 11:31 AM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Mar 20, 2023 at 06:57:21PM -0700, Elijah Newren wrote:\n>\n> > Honestly, looking back at those two patches of mine, I think both were\n> > rather suboptimal.  A better solution that would handle both F->D and\n> > D->F would be having fast-export sort the diff_filepairs such that it\n> > processes the deletes before the modifies.  Another improved solution\n> > would be having fast-import sort the files given to it and handling\n> > deletes first.  Either should fix this.\n> >\n> > Might be a good task for a new contributor.  Any takers?  (Tagging as\n> > #leftoverbits.)\n>\n> There was a patch a while ago, but it didn't get applied:\n>\n>   https://lore.kernel.org/git/1493079137-1838-1-git-send-email-miguel.torroja@gmail.com/\n>\n> It got hung up on the fact that fast-export can also generate renames,\n> and ordering there is tricky. I stand by the sentiment from back then\n> that it is still worth it to order things to make the no-rename case\n> work, even if there are still corner cases with renames (since you can\n> have a cycle of renames, you should not use them if you want to be\n> robust against this kind of ordering dependency).\n\nAh, thanks for the heads up.  Stinkin' renames.  ;-)  I tend to forget\nthose in the fast-export/fast-import context since fast-export doesn't\ndetect renames by default (and e.g. filter-repo would turn them off if\nthey were on by default).\n\nSo, I think we probably need to have the fix in fast-import.\nCurrently, parse_new_commit() calls the various file_change_*()\nfunctions immediately, which in turn immediately call the various\ntree_content_*() functions (e.g. file_change_cr() for a rename will\ncall both tree_content_remove() and tree_content_set()).  Instead of\nimmediately handling all these, I think that queueing all the\nfile_change_*() calls up, splitting the renames into delete + modify,\nand sorting all the deletes first should fix things for both F->D and\nD->F, even when cycles of renames are present as per your example in\nthat thread.\n"}]}