{"thread":{"id":"41200","subject":"BUG: git subtree split gets confused on removed and readded directory","startedAt":"2016-01-15T16:23:24Z","lastAt":"2016-02-03T02:34:31Z","messageCount":12,"participants":["Marcus Brinkmann","Junio C Hamano","David Ware","David A. Greene"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"276165","messageId":"56991CFC.7060705@ruhr-uni-bochum.de","threadId":"41200","inReplyTo":null,"subject":"BUG: git subtree split gets confused on removed and readded directory","fromName":"Marcus Brinkmann","fromEmail":"marcus.brinkmann@ruhr-uni-bochum.de","sentAt":"2016-01-15T16:23:24Z","receivedAt":"2016-01-15T16:23:24Z","isPatch":false,"sender":{"key":"marcus.brinkmann@ruhr-uni-bochum.de","avatar":null},"body":"Hi,\n\nI made a simple test repository showing the problem here:\nhttps://github.com/lambdafu/git-subtree-split-test\n\nAfter creating the master branch, I created the split/bar branch like this:\n\n$ git subtree split -P bar -b split/bar\n\nThe resulting history is confused by the directory \"bar\" which was\nadded, removed and then re-added again. The recent history up to adding\nthe directory the second time is fine. But then it seems to loose track\nand add the parent of that commit up to the initial commit in the history.\n\nI'd expect that the parent of the readding commit is an empty tree\ncommit (which removed the last files in the directory), and that before\nthat are commits that reflect the initial creation of that directory\nwith its files, but rewritten as a subtree, of course.\n\nThanks!\nMarcus\n"},{"id":"276223","messageId":"xmqq4meeflws.fsf@gitster.mtv.corp.google.com","threadId":"41200","inReplyTo":"56991CFC.7060705@ruhr-uni-bochum.de","subject":"Re: BUG: git subtree split gets confused on removed and readded directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-15T23:44:19Z","receivedAt":"2016-01-15T23:44:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcus Brinkmann writes:\n\n> I made a simple test repository showing the problem here:\n> https://github.com/lambdafu/git-subtree-split-test\n>\n> After creating the master branch, I created the split/bar branch like this:\n>\n> $ git subtree split -P bar -b split/bar\n>\n> The resulting history is confused by the directory \"bar\" which was\n> added, removed and then re-added again. The recent history up to adding\n> the directory the second time is fine. But then it seems to loose track\n> and add the parent of that commit up to the initial commit in the history.\n>\n> I'd expect that the parent of the readding commit is an empty tree\n> commit (which removed the last files in the directory), and that before\n> that are commits that reflect the initial creation of that directory\n> with its files, but rewritten as a subtree, of course.\n\nThanks for a report.\n\nDavid, does this ring a bell?\n\nDave, does your fix \"subtree split\" we saw recently on the list\n\n http://article.gmane.org/gmane.comp.version-control.git/284125\n\nhelp this?\n"},{"id":"276251","messageId":"CAET=KiXJ4tkryy_UNWtD3dRSXXpBfL=7ZS5GNivmGi0Yx7Rv4A@mail.gmail.com","threadId":"41200","inReplyTo":"xmqq4meeflws.fsf@gitster.mtv.corp.google.com","subject":"Re: BUG: git subtree split gets confused on removed and readded directory","fromName":"David Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2016-01-17T19:34:16Z","receivedAt":"2016-01-17T19:34:16Z","isPatch":false,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"No my patch doesn't seem to fix this.\n\nCheers,\nDave Ware\n\n(sorry if you're receiving this for the second time, I'm resending\nsince the mailing list blocked my earlier reply for html content)\n\nOn Sat, Jan 16, 2016 at 12:44 PM, Junio C Hamano wrote:\n> Marcus Brinkmann writes:\n>\n>> I made a simple test repository showing the problem here:\n>> https://github.com/lambdafu/git-subtree-split-test\n>>\n>> After creating the master branch, I created the split/bar branch like this:\n>>\n>> $ git subtree split -P bar -b split/bar\n>>\n>> The resulting history is confused by the directory \"bar\" which was\n>> added, removed and then re-added again. The recent history up to adding\n>> the directory the second time is fine. But then it seems to loose track\n>> and add the parent of that commit up to the initial commit in the history.\n>>\n>> I'd expect that the parent of the readding commit is an empty tree\n>> commit (which removed the last files in the directory), and that before\n>> that are commits that reflect the initial creation of that directory\n>> with its files, but rewritten as a subtree, of course.\n>\n> Thanks for a report.\n>\n> David, does this ring a bell?\n>\n> Dave, does your fix \"subtree split\" we saw recently on the list\n>\n> http://article.gmane.org/gmane.comp.version-control.git/284125\n>\n> help this?\n"},{"id":"276258","messageId":"87twmbaizo.fsf@waller.obbligato.org","threadId":"41200","inReplyTo":"xmqq4meeflws.fsf@gitster.mtv.corp.google.com","subject":"Re: BUG: git subtree split gets confused on removed and readded directory","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-17T23:23:07Z","receivedAt":"2016-01-17T23:23:07Z","isPatch":false,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Junio C Hamano writes:\n\n> Marcus Brinkmann writes:\n>\n>> I made a simple test repository showing the problem here:\n>> https://github.com/lambdafu/git-subtree-split-test>\n>> After creating the master branch, I created the split/bar branch like this:\n>>\n>> $ git subtree split -P bar -b split/bar\n>>\n>> The resulting history is confused by the directory \"bar\" which was\n>> added, removed and then re-added again. The recent history up to adding\n>> the directory the second time is fine. But then it seems to loose track\n>> and add the parent of that commit up to the initial commit in the history.\n>>\n>> I'd expect that the parent of the readding commit is an empty tree\n>> commit (which removed the last files in the directory), and that before\n>> that are commits that reflect the initial creation of that directory\n>> with its files, but rewritten as a subtree, of course.\n>\n> Thanks for a report.\n\nYes, thank you!\n\n> David, does this ring a bell?\n\nNo, I have not run into this before. I'm actually going to be working\nin the split code starting sometime this month (work allowing, of\ncourse). So it's great to get a report like this.\n\nOne of the things I want to do is eventually move over subtree split to\nusing a proper filter-branch instead of the entirely custom code that's\ncurrently there. This does, however, appear to cause a semntic\ndifference in preliminary testing which I am still tracking down. The\nfilter-based split is *incredibly* faster than the current code. The\ncurrent code can take hours on moderately-sized histories.\n\nThis should shake out a lot of these kinds of problems since the\nfilter-branch code is heavily used and tested while the subtree split\ncode is not.\n\nAssuming this goes ahead, I plan to introduce a new switch to control\nfilter-branch vs. original code and migrate the default to filter-branch\nif all goes well.\n\nI'll write up a failing test for this so that I remember to address it\nwhen I get to the code.\n\nThanks again, Marcus!\n\n -David\n"},{"id":"276393","messageId":"569EE046.9040506@semantics.de","threadId":"41200","inReplyTo":"87twmbaizo.fsf@waller.obbligato.org","subject":"[PATCH] contrib/subtree: Split history with empty trees correctly (was: Re: BUG: git subtree split gets confused on removed and readded directory)","fromName":"Marcus Brinkmann","fromEmail":"m.brinkmann@semantics.de","sentAt":"2016-01-20T01:17:58Z","receivedAt":"2016-01-20T01:17:58Z","isPatch":true,"sender":{"key":"m.brinkmann@semantics.de","avatar":null},"body":"'git subtree split' will fail if the history of the subtree has empty\ntree commits (or trees that are considered empty, such as submodules).\nThis fix keeps track of this condition and correctly follows the history\nover such commits.\n\nSigned-off-by: Marcus Brinkmann \n---\n contrib/subtree/git-subtree.sh | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex edf36f8..b68828b 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -36,6 +36,8 @@ PATH=$PATH:$(git --exec-path)\n\n require_work_tree\n\n+EMPTY_TREE=`git hash-object -t tree /dev/null`\n+\n quiet=\n branch=\n debug=\n@@ -449,7 +451,8 @@ copy_or_skip()\n \trev=\"$1\"\n \ttree=\"$2\"\n \tnewparents=\"$3\"\n-\tassert [ -n \"$tree\" ]\n+\n+\t[ -z \"$tree\" ] && tree=$EMPTY_TREE\n\n \tidentical=\n \tnonidentical=\n@@ -603,6 +606,7 @@ cmd_split()\n \trevmax=$(eval \"$grl\" | wc -l)\n \trevcount=0\n \tcreatecount=0\n+\tfound_first_commit=\n \teval \"$grl\" |\n \twhile read rev parents; do\n \t\trevcount=$(($revcount + 1))\n@@ -625,12 +629,16 @@ cmd_split()\n \t\t\n \t\t# ugly. is there no better way to tell if this is a subtree\n \t\t# vs. a mainline commit? Does it matter?\n-\t\tif [ -z $tree ]; then\n-\t\t\tset_notree $rev\n-\t\t\tif [ -n \"$newparents\" ]; then\n-\t\t\t\tcache_set $rev $rev\n+\t\tif [ -z $found_first_commit ]; then\n+\t\t\tif [ -z $tree ]; then\n+\t\t\t\tset_notree $rev\n+\t\t\t\tif [ -n \"$newparents\" ]; then\n+\t\t\t\t\tcache_set $rev $rev\n+\t\t\t\tfi\n+\t\t\t\tcontinue\n+\t\t\telse\n+\t\t\t\tfound_first_commit=yes\n \t\t\tfi\n-\t\t\tcontinue\n \t\tfi\n\n \t\tnewrev=$(copy_or_skip \"$rev\" \"$tree\" \"$newparents\") || exit $?\n-- \n2.5.0\n"},{"id":"276411","messageId":"871t9cvqsp.fsf@waller.obbligato.org","threadId":"41200","inReplyTo":"569EE046.9040506@semantics.de","subject":"Re: [PATCH] contrib/subtree: Split history with empty trees correctly","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-20T04:05:42Z","receivedAt":"2016-01-20T04:05:42Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Marcus Brinkmann writes:\n\n> 'git subtree split' will fail if the history of the subtree has empty\n> tree commits (or trees that are considered empty, such as submodules).\n> This fix keeps track of this condition and correctly follows the history\n> over such commits.\n\nThanks for working on this! Please add a test to t7900-subtree.sh.\n\n> @@ -625,12 +629,16 @@ cmd_split()\n> \t\t\n> \t\t# ugly. is there no better way to tell if this is a subtree\n> \t\t# vs. a mainline commit? Does it matter?\n> -\t\tif [ -z $tree ]; then\n> -\t\t\tset_notree $rev\n> -\t\t\tif [ -n \"$newparents\" ]; then\n> -\t\t\t\tcache_set $rev $rev\n> +\t\tif [ -z $found_first_commit ]; then\n> +\t\t\tif [ -z $tree ]; then\n> +\t\t\t\tset_notree $rev\n> +\t\t\t\tif [ -n \"$newparents\" ]; then\n> +\t\t\t\t\tcache_set $rev $rev\n> +\t\t\t\tfi\n> +\t\t\t\tcontinue\n> +\t\t\telse\n> +\t\t\t\tfound_first_commit=yes\n> \t\t\tfi\n> -\t\t\tcontinue\n> \t\tfi\n>\n> \t\tnewrev=$(copy_or_skip \"$rev\" \"$tree\" \"$newparents\") || exit $?\n\nCan you explain the logic here? The old code appears to be using the\nlack of a tree to filter out \"mainline\" commits from the subtree history\nwhen splitting. If that test is only done before seeing a proper\nsubtree commit and never after, then any commit mainline commit\nfollowing the first subtree commit in the rev list will miss being\nmarked with set_notree and the cache will not have the identity entry\nadded.\n\nTest #36 in t7900-subtree.sh has a mainline commit listed after the\nfirst subtree commit in the rev list, I believe.\n\nI'm not positive your change is wrong, I'd just like to understand it\nbetter. I'd also like a comment explaining why it works so future\ndevelopers don't get confused. Overall, I am trying to better comment\nthe code as I make my own changes.\n\n -David\n"},{"id":"276440","messageId":"569F6DF0.60900@semantics.de","threadId":"41200","inReplyTo":"871t9cvqsp.fsf@waller.obbligato.org","subject":"Re: [PATCH] contrib/subtree: Split history with empty trees correctly","fromName":"Marcus Brinkmann","fromEmail":"m.brinkmann@semantics.de","sentAt":"2016-01-20T11:22:24Z","receivedAt":"2016-01-20T11:22:24Z","isPatch":true,"sender":{"key":"m.brinkmann@semantics.de","avatar":null},"body":"On 01/20/2016 05:05 AM, David A. Greene wrote:\n> Marcus Brinkmann writes:\n> \n>> 'git subtree split' will fail if the history of the subtree has empty\n>> tree commits (or trees that are considered empty, such as submodules).\n>> This fix keeps track of this condition and correctly follows the history\n>> over such commits.\n> \n> Thanks for working on this! Please add a test to t7900-subtree.sh.\n\nI couldn't get the tests to run and I couldn't find documentation on how\nto run it. If you enlighten me I can add a test :)\n\n>> @@ -625,12 +629,16 @@ cmd_split()\n>> \t\t\n>> \t\t# ugly. is there no better way to tell if this is a subtree\n>> \t\t# vs. a mainline commit? Does it matter?\n>> -\t\tif [ -z $tree ]; then\n>> -\t\t\tset_notree $rev\n>> -\t\t\tif [ -n \"$newparents\" ]; then\n>> -\t\t\t\tcache_set $rev $rev\n>> +\t\tif [ -z $found_first_commit ]; then\n>> +\t\t\tif [ -z $tree ]; then\n>> +\t\t\t\tset_notree $rev\n>> +\t\t\t\tif [ -n \"$newparents\" ]; then\n>> +\t\t\t\t\tcache_set $rev $rev\n>> +\t\t\t\tfi\n>> +\t\t\t\tcontinue\n>> +\t\t\telse\n>> +\t\t\t\tfound_first_commit=yes\n>> \t\t\tfi\n>> -\t\t\tcontinue\n>> \t\tfi\n>>\n>> \t\tnewrev=$(copy_or_skip \"$rev\" \"$tree\" \"$newparents\") || exit $?\n> \n> Can you explain the logic here? The old code appears to be using the\n> lack of a tree to filter out \"mainline\" commits from the subtree history\n> when splitting. If that test is only done before seeing a proper\n> subtree commit and never after, then any commit mainline commit\n> following the first subtree commit in the rev list will miss being\n> marked with set_notree and the cache will not have the identity entry\n> added.\n> \n> Test #36 in t7900-subtree.sh has a mainline commit listed after the\n> first subtree commit in the rev list, I believe.\n> \n> I'm not positive your change is wrong, I'd just like to understand it\n> better. I'd also like a comment explaining why it works so future\n> developers don't get confused. Overall, I am trying to better comment\n> the code as I make my own changes.\n\nIt's possible the patch does not work for some cases. For example, I\ndon't know how the rejoin variant of splits work.\n\nSome observations:\n\n1) The notree list is never actually used except to identify which\ncommits have been visited in check_parents.\n\n2) I have no idea what use case is covered by the \"if [ -n \"$newparents\"\n]; then cache_set $rev $rev; fi\". I left it in purely for traditional\nreasons. So, clarifying that would go a long way in understanding the\ncode, and if there is a test for that, I will figure it out.\n\n3a) The bug happens because on the first commit that deletes the subdir,\nnewparents will not be empty, and the \"cache_set $rev $rev\" will kick in\nand subsequently (when the subdir is added again) the history will\ndivert into the $rev commit which is not rewritten, but part of the\nunsplit tree. This seems very wrong to me! See 2).\n\n3b) To be very clear: It seems logically inconsistent to me to ever call\nset_notree and cache_set on the same rev. It also seems logically\ninconsistent to me to call cache_set rev1 rev2 where rev2 is not\nrewritten. Both seem to be invariant errors that could be caught by\nassertions. They probably should. In fact, I think my patch makes the\nquestionable if-case to be dead code, because newparents is never\nnon-empty before found_first_commit is true. As such, I think it could\nbe eliminated. But I am not 100% sure, as I don't know the intention of\nthe original code.\n\n4) My patch only preserves the special handling of empty trees up to the\nfirst commit that introduces subdir, because we don't want an empty\ncommit at the beginning. After that, empty subdirs are not special at\nall - the empty tree is replaced by EMPTY_TREE and handled as if it's a\nnormal subdir commit. copy_and_skip will do the right thing.\n\n5) I didn't test any case with multiple parents (merge commits). There\nare several of those cases (merge commits into empty subdirs, branches\nwith different non-empty subdirs from empty ones), and they don't apply\nto my use case (git-svn conversion). I read the copy_and_skip code and\nsee that it optimizes some of those cases, and although I didn't see an\nobvious problem, I didn't think too deeply about it.\n\nThanks,\nMarcus\n\n\n\n-- \nsmantics GmbH\nViktoriaallee 45\n52066 Aachen\nWeb: www.semantics.de\nRegistergericht : Amtsgericht Aachen, HRB 8189\nGeschäftsführer : Kay Heiligenhaus M.A.\n Dipl. Ing. José de la Rosa\n"},{"id":"276628","messageId":"56A4CC85.90705@semantics.de","threadId":"41200","inReplyTo":"871t9cvqsp.fsf@waller.obbligato.org","subject":"Re: [PATCH] contrib/subtree: Split history with empty trees correctly","fromName":"Marcus Brinkmann","fromEmail":"m.brinkmann@semantics.de","sentAt":"2016-01-24T13:07:17Z","receivedAt":"2016-01-24T13:07:17Z","isPatch":true,"sender":{"key":"m.brinkmann@semantics.de","avatar":null},"body":"With my patch, \"git subtree split -P\" produces the same result (for my\ndata set) as \"git filter-branch --subdirectory-filter\", which is much\nfaster, because it selects the revisions to rewrite before rewriting.\nAs I am not using any of the advanced features of \"git subtree\", I will\njust use \"git filter-branch\" instead.\n\nThanks!\nMarcus\n\nOn 01/20/2016 05:05 AM, David A. Greene wrote:\n> Marcus Brinkmann writes:\n> \n>> 'git subtree split' will fail if the history of the subtree has empty\n>> tree commits (or trees that are considered empty, such as submodules).\n>> This fix keeps track of this condition and correctly follows the history\n>> over such commits.\n> \n> Thanks for working on this! Please add a test to t7900-subtree.sh.\n> \n>> @@ -625,12 +629,16 @@ cmd_split()\n>> \t\t\n>> \t\t# ugly. is there no better way to tell if this is a subtree\n>> \t\t# vs. a mainline commit? Does it matter?\n>> -\t\tif [ -z $tree ]; then\n>> -\t\t\tset_notree $rev\n>> -\t\t\tif [ -n \"$newparents\" ]; then\n>> -\t\t\t\tcache_set $rev $rev\n>> +\t\tif [ -z $found_first_commit ]; then\n>> +\t\t\tif [ -z $tree ]; then\n>> +\t\t\t\tset_notree $rev\n>> +\t\t\t\tif [ -n \"$newparents\" ]; then\n>> +\t\t\t\t\tcache_set $rev $rev\n>> +\t\t\t\tfi\n>> +\t\t\t\tcontinue\n>> +\t\t\telse\n>> +\t\t\t\tfound_first_commit=yes\n>> \t\t\tfi\n>> -\t\t\tcontinue\n>> \t\tfi\n>>\n>> \t\tnewrev=$(copy_or_skip \"$rev\" \"$tree\" \"$newparents\") || exit $?\n> \n> Can you explain the logic here? The old code appears to be using the\n> lack of a tree to filter out \"mainline\" commits from the subtree history\n> when splitting. If that test is only done before seeing a proper\n> subtree commit and never after, then any commit mainline commit\n> following the first subtree commit in the rev list will miss being\n> marked with set_notree and the cache will not have the identity entry\n> added.\n> \n> Test #36 in t7900-subtree.sh has a mainline commit listed after the\n> first subtree commit in the rev list, I believe.\n> \n> I'm not positive your change is wrong, I'd just like to understand it\n> better. I'd also like a comment explaining why it works so future\n> developers don't get confused. Overall, I am trying to better comment\n> the code as I make my own changes.\n> \n> -David\n> \n\n\n-- \nsmantics GmbH\nViktoriaallee 45\n52066 Aachen\nWeb: www.semantics.de\nRegistergericht : Amtsgericht Aachen, HRB 8189\nGeschäftsführer : Kay Heiligenhaus M.A.\n Dipl. Ing. José de la Rosa\n"},{"id":"276952","messageId":"87k2mul8f2.fsf@waller.obbligato.org","threadId":"41200","inReplyTo":"569F6DF0.60900@semantics.de","subject":"Re: [PATCH] contrib/subtree: Split history with empty trees correctly","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-28T02:55:29Z","receivedAt":"2016-01-28T02:55:29Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"[ Sorry it took a few days to reply. I am absolutely slammed at work\n and will be for the next few weeks at least. The good news is that\n it's resulting in some nice work on git-subtree! :) ]\n\nMarcus Brinkmann writes:\n\n> On 01/20/2016 05:05 AM, David A. Greene wrote:\n>> Marcus Brinkmann writes:\n>> \n>>> 'git subtree split' will fail if the history of the subtree has empty\n>>> tree commits (or trees that are considered empty, such as submodules).\n>>> This fix keeps track of this condition and correctly follows the history\n>>> over such commits.\n>> \n>> Thanks for working on this! Please add a test to t7900-subtree.sh.\n>\n> I couldn't get the tests to run and I couldn't find documentation on how\n> to run it. If you enlighten me I can add a test :)\n\nJust run \"make test\" in contrib/subtree. You have to build git first.\n\n>> Can you explain the logic here? The old code appears to be using the\n>> lack of a tree to filter out \"mainline\" commits from the subtree history\n>> when splitting. If that test is only done before seeing a proper\n>> subtree commit and never after, then any commit mainline commit\n>> following the first subtree commit in the rev list will miss being\n>> marked with set_notree and the cache will not have the identity entry\n>> added.\n>> \n>> Test #36 in t7900-subtree.sh has a mainline commit listed after the\n>> first subtree commit in the rev list, I believe.\n>> \n>> I'm not positive your change is wrong, I'd just like to understand it\n>> better. I'd also like a comment explaining why it works so future\n>> developers don't get confused. Overall, I am trying to better comment\n>> the code as I make my own changes.\n>\n> It's possible the patch does not work for some cases. For example, I\n> don't know how the rejoin variant of splits work.\n\nI'm not so much worried about catching all cases of the bug you\nidentified, though it would be good if the patch did. I'm much more\nconcerned about not causing a regression in existing functionality.\n\n> Some observations:\n>\n> 1) The notree list is never actually used except to identify which\n> commits have been visited in check_parents.\n\nIt's really verifying that we visited parents before children in the\nsplit code, I think. That seems like a good check to keep.\n\nLet me make sure I understand your fix too. Are you essentially\nskipping empty commits when splitting? You original patch said that\nsplit failed but didn't say how. Did git-subtree spit out an error\nmessage, or did the failure manifest in some other way? If I knew the\nfailure mode it might help me understand your changes better.\n\nI see you explain the proble below (thanks!) but I'd still like to know\nhow you discovered it. It will help in constructing a test.\n\n> 2) I have no idea what use case is covered by the \"if [ -n \"$newparents\"\n> ]; then cache_set $rev $rev; fi\". I left it in purely for traditional\n> reasons. So, clarifying that would go a long way in understanding the\n> code, and if there is a test for that, I will figure it out.\n\nAs far as I understand things, $newparents being non-empty means that\nthe commits parents were split out and $newparents contains the hashes\nof the split commits, so that when this commit is split it can set up\nthe proper parent links.\n\nIf the commit doesn't have a tree in the subdirectory, then I *think*\nthe split codesimply sets the identity entry in the cache so that any\nfuture commit that has this (empty) one as a parent will see it in the\n$newparents list. Since copy_or_skip checks for parents with empty\ntrees and does not link split commits to them, I don't understand the\npurpose of including these commits in $newparents.\n\nSo you may very well be right that it just doesn't matter if we skip\nthese altogether.\n\n> 3a) The bug happens because on the first commit that deletes the subdir,\n> newparents will not be empty, and the \"cache_set $rev $rev\" will kick in\n> and subsequently (when the subdir is added again) the history will\n> divert into the $rev commit which is not rewritten, but part of the\n> unsplit tree. This seems very wrong to me! See 2).\n\nAh, so the problem isn't empty commits per se, it's the fact that a\nsubdirectory was deleted and re-added. That makes sense. I didn't\nunderstand that from your original commit message though I now remember\nyou discussed it in the first e-mail you sent. It would be good to\nclarify this in the final commit.\n\nI agree that the behavior you describe is wrong. So the fix basically\nrelies on the fact that there is some tree in the subdirectory, which\nlater gets deleted, but since \"found_first_commit\" triggered, we'll just\nskip those empty commits and never see them in the cache so that when a\ntree appears again we won't link to mainline commits. Have I got it\nright?\n\nCurrently the split code is all-or-nothing. You split the whole history\nor none of it. Eventually I want to make it flexible enough to allow\nsplitting ranges of commits or individual commits. I'm wondering how\nyour code will work if the split range starts or ends within the set of\nempty subdirectory commits. It's not something you really have to worry\nabout since I'd deal with it when I write the code, but it's something\nthat popped into my head.\n\n> 3b) To be very clear: It seems logically inconsistent to me to ever call\n> set_notree and cache_set on the same rev. It also seems logically\n> inconsistent to me to call cache_set rev1 rev2 where rev2 is not\n> rewritten. Both seem to be invariant errors that could be caught by\n> assertions. They probably should. In fact, I think my patch makes the\n> questionable if-case to be dead code, because newparents is never\n> non-empty before found_first_commit is true. As such, I think it could\n> be eliminated. But I am not 100% sure, as I don't know the intention of\n> the original code.\n\nAfter your walk-through and some exploring of the code, I think you are\nright. Like you I am not 100% sure but it would definitely be nice to\nclean this bit of code up.\n\n> 4) My patch only preserves the special handling of empty trees up to the\n> first commit that introduces subdir, because we don't want an empty\n> commit at the beginning. After that, empty subdirs are not special at\n> all - the empty tree is replaced by EMPTY_TREE and handled as if it's a\n> normal subdir commit. copy_and_skip will do the right thing.\n\nI agree.\n\n> 5) I didn't test any case with multiple parents (merge commits). There\n> are several of those cases (merge commits into empty subdirs, branches\n> with different non-empty subdirs from empty ones), and they don't apply\n> to my use case (git-svn conversion). I read the copy_and_skip code and\n> see that it optimizes some of those cases, and although I didn't see an\n> obvious problem, I didn't think too deeply about it.\n\nYeah, that's definitely a concern. I've run some tests on the current\ngit-subtree with merge commits and IIRC the results made sense. I\nshould add those tests to the testbase.\n\nI'm definitely inclined to accept your patch after this discussion but I\ndo want to see a few things:\n\n1. A testcase\n\n2. A more expanded commit message basically describing what you said in\n 3-4 above, plus a bit more from your original e-mail describing the\n subdirectory delete and re-creation. It will help people who come\n along later understand the history of the code.\n\n3. Remove the questionable cache_set. I agree that it seems wrong and\n copy_and_skip ignores it anyway. Let's get rid of this cruft.\n Include something in the commit message about why this was done.\n\nThanks for the thorough explanation and for your work on this. If you\ncan re-roll with the above and the existing tests pass, I think we can\npass this on to Junio.\n\nLooking forward to the re-roll!\n\n -David\n"},{"id":"276953","messageId":"87fuxil8cw.fsf@waller.obbligato.org","threadId":"41200","inReplyTo":"56A4CC85.90705@semantics.de","subject":"Re: [PATCH] contrib/subtree: Split history with empty trees correctly","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-28T02:56:47Z","receivedAt":"2016-01-28T02:56:47Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Marcus Brinkmann writes:\n\n> With my patch, \"git subtree split -P\" produces the same result (for my\n> data set) as \"git filter-branch --subdirectory-filter\", which is much\n> faster, because it selects the revisions to rewrite before rewriting.\n> As I am not using any of the advanced features of \"git subtree\", I will\n> just use \"git filter-branch\" instead.\n\nHeh. :)\n\nI hope to replace all that ugly split code with filter-branch as you\ndescribe but there are some cases where it differs. It may be that your\nchanges fix some of that.\n\nAre you still able to do a re-roll on this?\n\n -David\n"},{"id":"276955","messageId":"56A993D7.3000107@semantics.de","threadId":"41200","inReplyTo":"87fuxil8cw.fsf@waller.obbligato.org","subject":"Re: [PATCH] contrib/subtree: Split history with empty trees correctly","fromName":"Marcus Brinkmann","fromEmail":"m.brinkmann@semantics.de","sentAt":"2016-01-28T04:06:47Z","receivedAt":"2016-01-28T04:06:47Z","isPatch":true,"sender":{"key":"m.brinkmann@semantics.de","avatar":null},"body":"On 01/28/2016 03:56 AM, David A. Greene wrote:\n> Marcus Brinkmann writes:\n>\n>> With my patch, \"git subtree split -P\" produces the same result (for my\n>> data set) as \"git filter-branch --subdirectory-filter\", which is much\n>> faster, because it selects the revisions to rewrite before rewriting.\n>> As I am not using any of the advanced features of \"git subtree\", I will\n>> just use \"git filter-branch\" instead.\n>\n> Heh. :)\n>\n> I hope to replace all that ugly split code with filter-branch as you\n> describe but there are some cases where it differs. It may be that your\n> changes fix some of that.\n>\n> Are you still able to do a re-roll on this?\n\nI have to admit that my interest has declined steeply since discovering \nthat subtree-split and filter-branch --subtree-filter give different \nresults from \"git svn\" on the subdirectory. The reason is that git-svn \nincludes all commits for revisions that regular \"svn log\" gives on that \ndirectory, which includes commits that serve as branch points only or \nthat are empty except for unhandled properties.\n\nWhile empty commits for unhandled properties wouldn't be fatal, missing \nbranch points make \"git svn\" really unhappy when asked to rebuild .git/svn.\n\nAs migration from SVN is my main motivation at this point to use a \nsubtree filter at this point (git-svn is just very slow - about one week \non our repository), I am somewhat stuck and back to using git-svn. \nAlthough hacking up something with filter-branch seems like a remote \noption, it's probably nothing that generalizes.\n\nIt didn't help that \"make test\" in contrib/subtree gives me 27 out of 29 \nfailed tests (with no indication how to figure out what exactly failed).\n\nOh well :)\n\nMarcus\n"},{"id":"277294","messageId":"87egcu5xoo.fsf@waller.obbligato.org","threadId":"41200","inReplyTo":"56A993D7.3000107@semantics.de","subject":"Re: [PATCH] contrib/subtree: Split history with empty trees correctly","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-02-03T02:34:31Z","receivedAt":"2016-02-03T02:34:31Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Marcus Brinkmann writes:\n\n>> Are you still able to do a re-roll on this?\n>\n> I have to admit that my interest has declined steeply since\n> discovering that subtree-split and filter-branch --subtree-filter give\n> different results from \"git svn\" on the subdirectory. The reason is\n> that git-svn includes all commits for revisions that regular \"svn log\"\n> gives on that directory, which includes commits that serve as branch\n> points only or that are empty except for unhandled properties.\n\nWhat do you mean by \"branch points only?\"\n\nIt's ok if you can't do a reroll. I can't work on it right now but\nperhaps when I get back to cleaning up the split code I can take what\nyou have and incoporate it. I do very much appreciate your work on\nthis!\n\n> While empty commits for unhandled properties wouldn't be fatal,\n> missing branch points make \"git svn\" really unhappy when asked to\n> rebuild .git/svn.\n\n[ I may have misunderstood your intent, see below. ]\n\nI just want to make sure I understand your situation. You used git-svn\nto mirror a project to git and then used git-subtree to incorporate that\nmirror into a larger project?\n\nWhy is the split being done? If there's an active Subversion repository\nbeing mirrors it's much better to commit changes back to the Subversion\nrepository than to the git mirror.\n\n> As migration from SVN is my main motivation at this point to use a\n> subtree filter at this point (git-svn is just very slow - about one\n> week on our repository), I am somewhat stuck and back to using\n> git-svn. Although hacking up something with filter-branch seems like a\n> remote option, it's probably nothing that generalizes.\n\nOk, maybe I misunderstood your situation. Are you converting one big\nrepository via git-svn and then trying to break out individual\ndirectories into smaller projects?\n\ngit-svn + git-subtree/git-filter-branch is not the best way to do that.\nsvn-all-fast-export is far superior for a one-off conversion and makes\nsplitting repositories a breeze. It happens during conversion rather\nthan as a post-processing step.\n\nhttps://techbase.kde.org/Projects/MoveToGit/UsingSvn2Git\n\n> It didn't help that \"make test\" in contrib/subtree gives me 27 out of\n> 29 failed tests (with no indication how to figure out what exactly\n> failed).\n\nHuh. I don't know why that would happen. Did you build the git tools\nfirst? A testing run using --debug and --verbose (see the Makefile in\ncontrib/subtree/t) would be informative. I understand if you don't have\ntime to do that. I haven't seen such failures before so I'm curious as\nto what happened.\n\n -David\n"}]}