BUG: git subtree split gets confused on removed and readded directory

I made a simple test repository showing the problem here:
https://github.com/lambdafu/git-subtree-split-test

After creating the master branch, I created the split/bar branch like this:

$ git subtree split -P bar -b split/bar

The resulting history is confused by the directory "bar" which was added, removed and then re-added again. The recent history up to adding the directory the second time is fine. But then it seems to loose track and add the parent of that commit up to the initial commit in the history.

I'd expect that the parent of the readding commit is an empty tree commit (which removed the last files in the directory), and that before that are commits that reflect the initial creation of that directory with its files, but rewritten as a subtree, of course.

Quoted 11 lines · Marcus Brinkmann

I made a simple test repository showing the problem here:
https://github.com/lambdafu/git-subtree-split-test>
After creating the master branch, I created the split/bar branch like this:

$ git subtree split -P bar -b split/bar

The resulting history is confused by the directory "bar" which was added, removed and then re-added again. The recent history up to adding the directory the second time is fine. But then it seems to loose track and add the parent of that commit up to the initial commit in the history.

Quoted 4 lines

I'd expect that the parent of the readding commit is an empty tree commit (which removed the last files in the directory), and that before that are commits that reflect the initial creation of that directory with its files, but rewritten as a subtree, of course.

Thanks for a report.

David, does this ring a bell?

'git subtree split' will fail if the history of the subtree has empty tree commits (or trees that are considered empty, such as submodules). This fix keeps track of this condition and correctly follows the history over such commits.

contrib/subtree/git-subtree.sh +4 -4

@@ -625,12 +629,16 @@ cmd_split()
 
 		# ugly.  is there no better way to tell if this is a subtree
 		# vs. a mainline commit?  Does it matter?
-		if [ -z $tree ]; then
-			set_notree $rev
-			if [ -n "$newparents" ]; then
-				cache_set $rev $rev
+		if [ -z $found_first_commit ]; then
+			if [ -z $tree ]; then
+				set_notree $rev
+				if [ -n "$newparents" ]; then

Quoted 4 lines · Marcus Brinkmann

'git subtree split' will fail if the history of the subtree has empty tree commits (or trees that are considered empty, such as submodules). This fix keeps track of this condition and correctly follows the history over such commits.

Thanks for working on this! Please add a test to t7900-subtree.sh.

Quoted 12 lines

contrib/subtree/git-subtree.sh +4 -4

@@ -625,12 +629,16 @@ cmd_split()
 
 		# ugly.  is there no better way to tell if this is a subtree
 		# vs. a mainline commit?  Does it matter?
-		if [ -z $tree ]; then
-			set_notree $rev
-			if [ -n "$newparents" ]; then
-				cache_set $rev $rev
+		if [ -z $found_first_commit ]; then
+			if [ -z $tree ]; then
+				set_notree $rev
+				if [ -n "$newparents" ]; then

contrib/subtree/git-subtree.sh cmd_split() · line 644

Quoted 10 lines

contrib/subtree/git-subtree.sh +5 -1

+					cache_set $rev $rev
+				fi
+				continue
+			else
+				found_first_commit=yes
 			fi
-			continue
 		fi
 
 		newrev=$(copy_or_skip "$rev" "$tree" "$newparents") || exit $?

Can you explain the logic here? The old code appears to be using the lack of a tree to filter out "mainline" commits from the subtree history when splitting. If that test is only done before seeing a proper subtree commit and never after, then any commit mainline commit following the first subtree commit in the rev list will miss being marked with set_notree and the cache will not have the identity entry added.

Test #36 in t7900-subtree.sh has a mainline commit listed after the first subtree commit in the rev list, I believe.

I'm not positive your change is wrong, I'd just like to understand it better. I'd also like a comment explaining why it works so future developers don't get confused. Overall, I am trying to better comment the code as I make my own changes.

Quoted 7 lines · David A. Greene

Quoted 4 lines · Marcus Brinkmann

'git subtree split' will fail if the history of the subtree has empty tree commits (or trees that are considered empty, such as submodules). This fix keeps track of this condition and correctly follows the history over such commits.

Quoted 1 line

Thanks for working on this! Please add a test to t7900-subtree.sh.

I couldn't get the tests to run and I couldn't find documentation on how to run it. If you enlighten me I can add a test :)

Quoted 11 lines

Can you explain the logic here? The old code appears to be using the lack of a tree to filter out "mainline" commits from the subtree history when splitting. If that test is only done before seeing a proper subtree commit and never after, then any commit mainline commit following the first subtree commit in the rev list will miss being marked with set_notree and the cache will not have the identity entry added.

Test #36 in t7900-subtree.sh has a mainline commit listed after the first subtree commit in the rev list, I believe.

Quoted 4 lines

I'm not positive your change is wrong, I'd just like to understand it better. I'd also like a comment explaining why it works so future developers don't get confused. Overall, I am trying to better comment the code as I make my own changes.

It's possible the patch does not work for some cases. For example, I don't know how the rejoin variant of splits work.

Some observations:

1) The notree list is never actually used except to identify which commits have been visited in check_parents.

2) I have no idea what use case is covered by the "if [ -n "$newparents" ]; then cache_set $rev $rev; fi". I left it in purely for traditional reasons. So, clarifying that would go a long way in understanding the code, and if there is a test for that, I will figure it out.

3a) The bug happens because on the first commit that deletes the subdir, newparents will not be empty, and the "cache_set $rev $rev" will kick in and subsequently (when the subdir is added again) the history will divert into the $rev commit which is not rewritten, but part of the unsplit tree. This seems very wrong to me! See 2).

3b) To be very clear: It seems logically inconsistent to me to ever call set_notree and cache_set on the same rev. It also seems logically inconsistent to me to call cache_set rev1 rev2 where rev2 is not rewritten. Both seem to be invariant errors that could be caught by assertions. They probably should. In fact, I think my patch makes the questionable if-case to be dead code, because newparents is never non-empty before found_first_commit is true. As such, I think it could be eliminated. But I am not 100% sure, as I don't know the intention of the original code.

4) My patch only preserves the special handling of empty trees up to the first commit that introduces subdir, because we don't want an empty commit at the beginning. After that, empty subdirs are not special at all - the empty tree is replaced by EMPTY_TREE and handled as if it's a normal subdir commit. copy_and_skip will do the right thing.

5) I didn't test any case with multiple parents (merge commits). There are several of those cases (merge commits into empty subdirs, branches with different non-empty subdirs from empty ones), and they don't apply to my use case (git-svn conversion). I read the copy_and_skip code and see that it optimizes some of those cases, and although I didn't see an obvious problem, I didn't think too deeply about it.

Quoted 4 lines · Marcus Brinkmann

'git subtree split' will fail if the history of the subtree has empty tree commits (or trees that are considered empty, such as submodules). This fix keeps track of this condition and correctly follows the history over such commits.

Thanks for working on this! Please add a test to t7900-subtree.sh.

Quoted 12 lines

contrib/subtree/git-subtree.sh +4 -4

@@ -625,12 +629,16 @@ cmd_split()
 
 		# ugly.  is there no better way to tell if this is a subtree
 		# vs. a mainline commit?  Does it matter?
-		if [ -z $tree ]; then
-			set_notree $rev
-			if [ -n "$newparents" ]; then
-				cache_set $rev $rev
+		if [ -z $found_first_commit ]; then
+			if [ -z $tree ]; then
+				set_notree $rev
+				if [ -n "$newparents" ]; then

contrib/subtree/git-subtree.sh cmd_split() · line 644

Quoted 10 lines

contrib/subtree/git-subtree.sh +5 -1

+					cache_set $rev $rev
+				fi
+				continue
+			else
+				found_first_commit=yes
 			fi
-			continue
 		fi
 
 		newrev=$(copy_or_skip "$rev" "$tree" "$newparents") || exit $?

Can you explain the logic here? The old code appears to be using the lack of a tree to filter out "mainline" commits from the subtree history when splitting. If that test is only done before seeing a proper subtree commit and never after, then any commit mainline commit following the first subtree commit in the rev list will miss being marked with set_notree and the cache will not have the identity entry added.

Test #36 in t7900-subtree.sh has a mainline commit listed after the first subtree commit in the rev list, I believe.

I'm not positive your change is wrong, I'd just like to understand it better. I'd also like a comment explaining why it works so future developers don't get confused. Overall, I am trying to better comment the code as I make my own changes.

-David

With my patch, "git subtree split -P" produces the same result (for my data set) as "git filter-branch --subdirectory-filter", which is much faster, because it selects the revisions to rewrite before rewriting. As I am not using any of the advanced features of "git subtree", I will just use "git filter-branch" instead.

Quoted 5 lines · Marcus Brinkmann

With my patch, "git subtree split -P" produces the same result (for my data set) as "git filter-branch --subdirectory-filter", which is much faster, because it selects the revisions to rewrite before rewriting. As I am not using any of the advanced features of "git subtree", I will just use "git filter-branch" instead.

Heh. :)

I hope to replace all that ugly split code with filter-branch as you describe but there are some cases where it differs. It may be that your changes fix some of that.

Are you still able to do a re-roll on this?

Quoted 1 line

Are you still able to do a re-roll on this?

I have to admit that my interest has declined steeply since discovering that subtree-split and filter-branch --subtree-filter give different results from "git svn" on the subdirectory. The reason is that git-svn includes all commits for revisions that regular "svn log" gives on that directory, which includes commits that serve as branch points only or that are empty except for unhandled properties.

While empty commits for unhandled properties wouldn't be fatal, missing branch points make "git svn" really unhappy when asked to rebuild .git/svn.

As migration from SVN is my main motivation at this point to use a subtree filter at this point (git-svn is just very slow - about one week on our repository), I am somewhat stuck and back to using git-svn. Although hacking up something with filter-branch seems like a remote option, it's probably nothing that generalizes.

It didn't help that "make test" in contrib/subtree gives me 27 out of 29 failed tests (with no indication how to figure out what exactly failed).

Quoted 12 lines · Marcus Brinkmann

I made a simple test repository showing the problem here:
https://github.com/lambdafu/git-subtree-split-test

After creating the master branch, I created the split/bar branch like this:

$ git subtree split -P bar -b split/bar

The resulting history is confused by the directory "bar" which was added, removed and then re-added again. The recent history up to adding the directory the second time is fine. But then it seems to loose track and add the parent of that commit up to the initial commit in the history.

Quoted 4 lines

I'd expect that the parent of the readding commit is an empty tree commit (which removed the last files in the directory), and that before that are commits that reflect the initial creation of that directory with its files, but rewritten as a subtree, of course.

Thanks for a report.

David, does this ring a bell?

Dave, does your fix "subtree split" we saw recently on the list

    http://article.gmane.org/gmane.comp.version-control.git/284125

help this?