Re: [PATCH] contrib/subtree: Split history with empty trees correctly

[greened@obbligato.org (David A. Greene)]

Marcus Brinkmann <m.brinkmann@semantics.de> writes:

> '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.

> @@ -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
> +					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