{"thread":{"id":"31010","subject":"Export from bzr / Import to git results in a deleted file re-appearing","startedAt":"2012-07-12T18:00:17Z","lastAt":"2012-07-16T00:26:52Z","messageCount":9,"participants":["Felix Natter","Jeff King","Andreas Schwab","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"194976","messageId":"87ehogrham.fsf@bitburger.home.felix","threadId":"31010","inReplyTo":null,"subject":"Export from bzr / Import to git results in a deleted file re-appearing","fromName":"Felix Natter","fromEmail":"fnatter@gmx.net","sentAt":"2012-07-12T18:00:17Z","receivedAt":"2012-07-12T18:00:17Z","isPatch":false,"sender":{"key":"fnatter@gmx.net","avatar":null},"body":"hi,\n\nI am trying to move freeplane's repository (GPL-project) from bzr to\ngit, but when I do this:\n\n$ mkdir freeplane-git1\n$ cd freeplane-git1\n$ git init .\n$ bzr fast-export --export-marks=../marks.bzr ../trunk/ | git fast-import --export-marks=../marks.git\n$ git checkout\n\nthen there are no errors, but the resulting working index is broken:\n freeplane-git1/freeplane_plugin_formula/src/org/freeplane/plugin/formula\n contains SpreadSheetUtils.java which belongs to package\n 'org.freeplane.plugin.spreadsheet' and which is no longer in the bzr\n trunk that I imported!\n\nThe freeplane repo is publically available, so you should easily be able\nto reproduce this:\n bzr branch bzr://freeplane.bzr.sourceforge.net/bzrroot/freeplane/freeplane_program/trunk\n\nI reported this as a possible bzr-fastimport bug, but didn't get a\nresponse so far:\n https://bugs.launchpad.net/bzr-fastimport/+bug/1014291\n\nIs there maybe a better way to convert a repo from bzr to git?\nThis seems to be the standard solution on the web.\n\nI am using Bazaar (bzr) 2.5.0dev6, bzr-fastimport 0.13.0 and git 1.7.10.4.\n\nThanks and Best Regards!\n-- \nFelix Natter\n"},{"id":"194985","messageId":"20120712210138.GA15283@sigill.intra.peff.net","threadId":"31010","inReplyTo":"87ehogrham.fsf@bitburger.home.felix","subject":"Re: Export from bzr / Import to git results in a deleted file re-appearing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-12T21:01:38Z","receivedAt":"2012-07-12T21:01:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 12, 2012 at 08:00:17PM +0200, Felix Natter wrote:\n\n> I am trying to move freeplane's repository (GPL-project) from bzr to\n> git, but when I do this:\n> \n> $ mkdir freeplane-git1\n> $ cd freeplane-git1\n> $ git init .\n> $ bzr fast-export --export-marks=../marks.bzr ../trunk/ | git fast-import --export-marks=../marks.git\n> $ git checkout\n> \n> then there are no errors, but the resulting working index is broken:\n> freeplane-git1/freeplane_plugin_formula/src/org/freeplane/plugin/formula\n> contains SpreadSheetUtils.java which belongs to package\n> 'org.freeplane.plugin.spreadsheet' and which is no longer in the bzr\n> trunk that I imported!\n\nIf you run only the bzr half of your command and inspect the output, you\nwill see that the file in question is mentioned twice. Once in a commit\non \"refs/heads/master\" that renames into it from another file:\n\n R freeplane_plugin_spreadsheet/src/org/freeplane/plugin/spreadsheet/SpreadSheetUtils.java\n freeplane_plugin_formula/src/org/freeplane/plugin/formula/SpreadSheetUtils.java\n\nand another similar case creating a merge commit. But it is never\ndeleted. So from a cursory inspection, it looks like git is right to\ninclude the file in the final output (but I didn't trace the complete\nhistory graph, so it's possible that those commits are somehow not used\nin the final result).\n\nWhich leads me to believe that the bug is in bzr.\n\n-Peff\n"},{"id":"195002","messageId":"m2pq80uj56.fsf@igel.home","threadId":"31010","inReplyTo":"20120712210138.GA15283@sigill.intra.peff.net","subject":"Re: Export from bzr / Import to git results in a deleted file re-appearing","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2012-07-13T09:04:21Z","receivedAt":"2012-07-13T09:04:21Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 12, 2012 at 08:00:17PM +0200, Felix Natter wrote:\n>\n>> I am trying to move freeplane's repository (GPL-project) from bzr to\n>> git, but when I do this:\n>> \n>> $ mkdir freeplane-git1\n>> $ cd freeplane-git1\n>> $ git init .\n>> $ bzr fast-export --export-marks=../marks.bzr ../trunk/ | git fast-import --export-marks=../marks.git\n>> $ git checkout\n>> \n>> then there are no errors, but the resulting working index is broken:\n>> freeplane-git1/freeplane_plugin_formula/src/org/freeplane/plugin/formula\n>> contains SpreadSheetUtils.java which belongs to package\n>> 'org.freeplane.plugin.spreadsheet' and which is no longer in the bzr\n>> trunk that I imported!\n>\n> If you run only the bzr half of your command and inspect the output, you\n> will see that the file in question is mentioned twice. Once in a commit\n> on \"refs/heads/master\" that renames into it from another file:\n>\n> R freeplane_plugin_spreadsheet/src/org/freeplane/plugin/spreadsheet/SpreadSheetUtils.java\n> freeplane_plugin_formula/src/org/freeplane/plugin/formula/SpreadSheetUtils.java\n\nThat same revision also removes it, but is uses the original name for\nthe deletion (the bzr revision actually renames the containing\ndirectory). That's probably what confuses git fast-import.\n\nHere is a test case:\n\nbzr init\nmkdir a\nbzr add a\ntouch a/b\nbzr add a/b\nbzr ci -m a\nbzr mv a b\nbzr rm b/b\nbzr ci -m b\nbzr fast-export .\n\nThe output contains these lines:\n\nR a/b b/b\nD a/b\n\nChanging the second line to D b/b fixes the bug.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"195004","messageId":"20120713130246.GB2553@sigill.intra.peff.net","threadId":"31010","inReplyTo":"m2pq80uj56.fsf@igel.home","subject":"Re: Export from bzr / Import to git results in a deleted file re-appearing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-13T13:02:47Z","receivedAt":"2012-07-13T13:02:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 13, 2012 at 11:04:21AM +0200, Andreas Schwab wrote:\n\n> > If you run only the bzr half of your command and inspect the output, you\n> > will see that the file in question is mentioned twice. Once in a commit\n> > on \"refs/heads/master\" that renames into it from another file:\n> >\n> > R freeplane_plugin_spreadsheet/src/org/freeplane/plugin/spreadsheet/SpreadSheetUtils.java\n> > freeplane_plugin_formula/src/org/freeplane/plugin/formula/SpreadSheetUtils.java\n> \n> That same revision also removes it, but is uses the original name for\n> the deletion (the bzr revision actually renames the containing\n> directory). That's probably what confuses git fast-import.\n> [...]\n> The output contains these lines:\n> \n> R a/b b/b\n> D a/b\n> \n> Changing the second line to D b/b fixes the bug.\n\nYeah, I agree that is problematic. But I do not think it is a\nfast-import bug, but rather bogus output generated by bzr fast-export (I\nam not clear from what you wrote above if you are considering it a bug\nthat fast-import is confused). It seems nonsensical to mention a file\nboth as a rename source and as deleted in the same revision, and\ncertainly I would not expect an importer to deduce a link between the\nsecond line and b/b.\n\n-Peff\n"},{"id":"195006","messageId":"m2hatbvkyh.fsf@igel.home","threadId":"31010","inReplyTo":"20120713130246.GB2553@sigill.intra.peff.net","subject":"Re: Export from bzr / Import to git results in a deleted file re-appearing","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2012-07-13T13:39:50Z","receivedAt":"2012-07-13T13:39:50Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 13, 2012 at 11:04:21AM +0200, Andreas Schwab wrote:\n>\n>> > If you run only the bzr half of your command and inspect the output, you\n>> > will see that the file in question is mentioned twice. Once in a commit\n>> > on \"refs/heads/master\" that renames into it from another file:\n>> >\n>> > R freeplane_plugin_spreadsheet/src/org/freeplane/plugin/spreadsheet/SpreadSheetUtils.java\n>> > freeplane_plugin_formula/src/org/freeplane/plugin/formula/SpreadSheetUtils.java\n>> \n>> That same revision also removes it, but is uses the original name for\n>> the deletion (the bzr revision actually renames the containing\n>> directory). That's probably what confuses git fast-import.\n>> [...]\n>> The output contains these lines:\n>> \n>> R a/b b/b\n>> D a/b\n>> \n>> Changing the second line to D b/b fixes the bug.\n>\n> Yeah, I agree that is problematic. But I do not think it is a\n> fast-import bug, but rather bogus output generated by bzr fast-export (I\n> am not clear from what you wrote above if you are considering it a bug\n> that fast-import is confused). It seems nonsensical to mention a file\n> both as a rename source and as deleted in the same revision, and\n> certainly I would not expect an importer to deduce a link between the\n> second line and b/b.\n\nIMHO fast-import should raise an error in this case, like it does when\nyou switch the lines.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"195048","messageId":"878vem1kg0.fsf@bitburger.home.felix","threadId":"31010","inReplyTo":"m2hatbvkyh.fsf@igel.home","subject":"Re: Export from bzr / Import to git results in a deleted file re-appearing","fromName":"Felix Natter","fromEmail":"fnatter@gmx.net","sentAt":"2012-07-14T14:33:35Z","receivedAt":"2012-07-14T14:33:35Z","isPatch":false,"sender":{"key":"fnatter@gmx.net","avatar":null},"body":"Andreas Schwab writes:\n\n> Jeff King writes:\n>\n>> On Fri, Jul 13, 2012 at 11:04:21AM +0200, Andreas Schwab wrote:\n>>\n>>> > If you run only the bzr half of your command and inspect the output, you\n>>> > will see that the file in question is mentioned twice. Once in a commit\n>>> > on \"refs/heads/master\" that renames into it from another file:\n>>> >\n>>> > R freeplane_plugin_spreadsheet/src/org/freeplane/plugin/spreadsheet/SpreadSheetUtils.java\n>>> > freeplane_plugin_formula/src/org/freeplane/plugin/formula/SpreadSheetUtils.java\n>>> \n>>> That same revision also removes it, but is uses the original name for\n>>> the deletion (the bzr revision actually renames the containing\n>>> directory). That's probably what confuses git fast-import.\n>>> [...]\n>>> The output contains these lines:\n>>> \n>>> R a/b b/b\n>>> D a/b\n>>> \n>>> Changing the second line to D b/b fixes the bug.\n>>\n>> Yeah, I agree that is problematic. But I do not think it is a\n>> fast-import bug, but rather bogus output generated by bzr fast-export (I\n>> am not clear from what you wrote above if you are considering it a bug\n>> that fast-import is confused). It seems nonsensical to mention a file\n>> both as a rename source and as deleted in the same revision, and\n>> certainly I would not expect an importer to deduce a link between the\n>> second line and b/b.\n>\n> IMHO fast-import should raise an error in this case, like it does when\n> you switch the lines.\n\n+1.\n\nThanks very much for fixing bzr-fastexport!\nAlso many thanks to Jeff for the analysis!\n\nBest Regards,\n-- \nFelix Natter\n"},{"id":"195080","messageId":"20120715102300.GA28667@sigill.intra.peff.net","threadId":"31010","inReplyTo":"m2hatbvkyh.fsf@igel.home","subject":"[PATCH] fast-import: catch deletion of non-existent file in input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-15T10:23:00Z","receivedAt":"2012-07-15T10:23:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 13, 2012 at 03:39:50PM +0200, Andreas Schwab wrote:\n\n> >> The output contains these lines:\n> >> \n> >> R a/b b/b\n> >> D a/b\n> >> \n> >> Changing the second line to D b/b fixes the bug.\n> >\n> > Yeah, I agree that is problematic. But I do not think it is a\n> > fast-import bug, but rather bogus output generated by bzr fast-export (I\n> > am not clear from what you wrote above if you are considering it a bug\n> > that fast-import is confused). It seems nonsensical to mention a file\n> > both as a rename source and as deleted in the same revision, and\n> > certainly I would not expect an importer to deduce a link between the\n> > second line and b/b.\n> \n> IMHO fast-import should raise an error in this case, like it does when\n> you switch the lines.\n\nHere's a patch to do so. It means that fast-import is a little more\nstrict about deletions than it used to be. I think it's probably a good\nidea for fast-import to err on the side of strictness (since erring on\nthe other side may mean corrupt input). I don't know if the omission of\nerror checking in the original was a philosophical choice or just\nrandom. :)\n\n-- >8 --\nSubject: fast-import: catch deletion of non-existent file in input\n\nFast-import does not do a lot of input verification; in\nparticular, you can mark a path as deleted by a commit even\nif that files does not exist in the parent at all.\n\nOn the one hand, it is nice for fast-import to be liberal in\nwhat it accepts, as it means we are more forgiving of\nexporter bugs or of people slicing and dicing the input. On\nthe other hand, this can mask bugs in exporters, leading to\na subtly wrong import result. In this case, bzr's\nfast-export generated a sequence like:\n\n R foo bar\n D foo\n\nwhen it meant to say:\n\n R foo bar\n D bar\n\nWe silently ignored the bogus \"D foo\" directive, and the\nresulting tree incorrectly contained \"bar\". With this patch,\nwe notice the bogus input and die.\n\nThis patch adds a new test script specifically for checking\nbogus inputs to fast-import. We also need to tweak t9300,\nwhich appeared to be accidentally deleting a non-existent\npath in an otherwise unrelated test.\n\nSigned-off-by: Jeff King \n---\nThe tweak in t9300 looks like the \"dst2\" entry was just leftover cruft\nfrom writing the test; there is no dst2 mentioned anywhere else. It\ncomes from David's 8dc6a37; pleae correct me if my assumptions isn't\ncorrect.\n\n fast-import.c | 5 ++-\n t/t9300-fast-import.sh | 1 -\n t/t9302-fast-import-bogus.sh | 77 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 81 insertions(+), 2 deletions(-)\n create mode 100755 t/t9302-fast-import-bogus.sh\n\ndiff --git a/fast-import.c b/fast-import.c\nindex eed97c8..779adfc 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2367,6 +2367,7 @@ static void file_change_d(struct branch *b)\n \tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n \tconst char *endp;\n+\tstruct tree_entry leaf;\n \n \tstrbuf_reset(&uq);\n \tif (!unquote_c_style(&uq, p, &endp)) {\n@@ -2374,7 +2375,9 @@ static void file_change_d(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_remove(&b->branch_tree, p, NULL);\n+\ttree_content_remove(&b->branch_tree, p, &leaf);\n+\tif (!leaf.versions[1].mode)\n+\t\tdie(\"Path %s not in branch\", p);\n }\n \n static void file_change_cr(struct branch *b, int rename)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 2fcf269..7624ba5 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1203,7 +1203,6 @@ test_expect_success PIPE 'N: empty directory reads as missing' '\n \t\tprintf \"%s\\n\" \"$line\" >response &&\n \t\tcat <<-\\EOF\n \t\tD dst1\n-\t\tD dst2\n \t\tEOF\n \t) |\n \tgit fast-import --cat-blob-fd=3 3>backflow &&\ndiff --git a/t/t9302-fast-import-bogus.sh b/t/t9302-fast-import-bogus.sh\nnew file mode 100755\nindex 0000000..95c5196\n--- /dev/null\n+++ b/t/t9302-fast-import-bogus.sh\n@@ -0,0 +1,77 @@\n+#!/bin/sh\n+\n+test_description='test that fast-import handles bogus input correctly'\n+. ./test-lib.sh\n+\n+# A few shorthands to make writing sample input easier\n+counter=0\n+mark() {\n+\tcounter=$(( $counter + 1)) &&\n+\techo \"mark :$counter\"\n+}\n+\n+blob() {\n+\techo blob &&\n+\tmark &&\n+\tcat <<-\\EOF\n+\tdata 4\n+\tfoo\n+\n+\tEOF\n+}\n+\n+ident() {\n+\ttest_tick &&\n+\techo \"author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\"\n+\techo \"committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\"\n+}\n+\n+commit() {\n+\techo \"commit refs/heads/master\" &&\n+\tmark &&\n+\tident &&\n+\tcat <<-\\EOF\n+\tdata 8\n+\tmessage\n+\tEOF\n+}\n+\n+root() {\n+\tblob &&\n+\tm=$counter &&\n+\techo \"reset refs/heads/master\" &&\n+\tcommit &&\n+\techo \"M 100644 :$m file\" &&\n+\techo\n+}\n+\n+test_expect_success 'deleting a nonexistent file is an error' '\n+\t{\n+\t\troot &&\n+\t\tcommit &&\n+\t\techo \"D does-not-exist\"\n+\t} >input &&\n+\ttest_must_fail git fast-import --force input &&\n+\ttest_must_fail git fast-import --force input &&\n+\ttest_must_fail git fast-import --force Subject: fast-import: catch deletion of non-existent file in input\n[...]\n> We silently ignored the bogus \"D foo\" directive, and the\n> resulting tree incorrectly contained \"bar\". With this patch,\n> we notice the bogus input and die.\n\nThis breaks svn-fe, which relies on the existing semantics when asked\nto copy an empty directory.\n\nThat's my fault because we never check that in the testsuite, but I\nalso wouldn't be surprised if other importers were relying on the same\nthing.\n\nAny API break this big without a justification along the lines\n\n\tWe can be confident that no existing importer uses this\n\tconstruct because ...\n\n_needs_ to be guarded by a new \"feature\" to be safe for existing\nimporters.\n\nLet's repeat that for emphasis: API breaks in fast-import not guarded\nwith a new \"feature\" type are not ok.\n\nSorry,\nJonathan\n"},{"id":"195092","messageId":"20120716002652.GA27304@sigill.intra.peff.net","threadId":"31010","inReplyTo":"20120715181151.GA1986@burratino","subject":"Re: [PATCH] fast-import: catch deletion of non-existent file in input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-16T00:26:52Z","receivedAt":"2012-07-16T00:26:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 15, 2012 at 01:11:51PM -0500, Jonathan Nieder wrote:\n\n> > Subject: fast-import: catch deletion of non-existent file in input\n> [...]\n> > We silently ignored the bogus \"D foo\" directive, and the\n> > resulting tree incorrectly contained \"bar\". With this patch,\n> > we notice the bogus input and die.\n> \n> This breaks svn-fe, which relies on the existing semantics when asked\n> to copy an empty directory.\n\nThanks for the report. I had a worry while writing this that somebody\nwas relying on the behavior. Let's just drop it, then. It's nice to\ncatch errors in exporters, but not at the expense of compatibility\nissues.\n\nWe could introduce a new feature bit, but I'm not sure it is really\nworthwhile. The older versions of bzr-fast-export would not set the bit\nanyway, and newer versions are already fixed, so it is kind of closing\nthe barn door after the horse has left (we might catch other bugs, but\nthis one is kind of oddly specific; if somebody wanted to audit\nfast-import for other similar cases and introduce a \"strict\" feature\nbit, that might be worthwhile. But for this single change, I don't think\nso).\n\n> Let's repeat that for emphasis: API breaks in fast-import not guarded\n> with a new \"feature\" type are not ok.\n\nTotally agree. The question in my mind was whether this was a bug fix or\nan API change, and it sounds like it is too far towards the latter.\n\n-Peff\n"}]}