Re: [PATCH v5] contrib/subtree: fix "subtree split" skipped-merge bug

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

Dave Ware <davidw@realtimegenomics.com> writes:

[ I am sorry I took so long to respond.  This one slipped by me.  Thank
  you for tracking this problem down and fixing it!  ]

> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 9f06571..ebf99d9 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -479,8 +479,16 @@ copy_or_skip()
>  			p="$p -p $parent"
>  		fi
>  	done
> -	
> -	if [ -n "$identical" ]; then
> +
> +	copycommit=
> +	if [ -n "$identical" ] && [ -n "$nonidentical" ]; then
> +		extras=$(git rev-list --count $identical..$nonidentical)
> +		if [ "$extras" -ne 0 ]; then
> +			# we need to preserve history along the other branch
> +			copycommit=1
> +		fi
> +	fi
> +	if [ -n "$identical" ] && [ -z "$copycommit" ]; then
>  		echo $identical
>  	else
>  		copy_commit $rev $tree "$p" || exit $?

I don't see anything objectionable here.  I am just learning the split
code myself.  :)

However, when I apply this against master, the test doesn't actually
pass and a gitk --all shows the merge commit still missing.  At least if
I understand the problem correctly.  Can you verify whether it works for
you?

> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 9051982..4fe4820 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '
>  	))
>  '
>  
> +test_expect_success 'subtree descendant check' '
> +	mkdir git_subtree_split_check &&
> +	(
> +		cd git_subtree_split_check &&
> +		git init &&

This shouldn't be necessary.  If you look at the other tests in
t7900-subtree.sh, they all start with:

  next_test
  test_expect_success '<blah>' '
  subtree_test_create_repo "$subtree_test_count"

The "subtree_test_create_repo" takes care of creating a subdirectory and
initializing a repository.  Perhaps you didn't (or still don't) have the
test script rewrite patch that got merged a month or so ago.  If not,
please update to it and reformulate your test to follow the established
convention.  It helps *a lot* when debugging regressions.

> +		mkdir folder &&
> +
> +		echo a >folder/a &&
> +		git add . &&
> +		git commit -m "first commit" &&

You can use "test_create_commit" to do these "generate commit"
operations.  It's on my TODO list to update the subtree tests to use
more of the standard test infrastructure.  For now, just go ahead and
use what the other tests use.

> +		git branch noop_branch &&
[...]
> +		git checkout noop_branch &&
> +		echo moreText >anotherText.txt &&
> +		git add . &&
> +		git commit -m "irrelevant" &&

This is unfortunate naming.  Why is the branch a no-op and why is the
commit irrelevant?  Does the test test the same thing without them?  I
not they should have different names.  If so, why are these needed in
the test?

> +		git checkout master &&
> +		git merge -m "second merge" noop_branch &&
> +
> +		git subtree split --prefix folder/ --branch subtree_tip master &&
> +		git subtree split --prefix folder/ --branch subtree_branch branch &&
> +		git push . subtree_tip:subtree_branch

I understand the problem was discovered because of an inability to push
and it probably makes sense to test that since that's what exposed the
bug.  However, I wonder if there are some additional checks that should
be done.  What do you expect subtree_tip and subtree_branch to look like
and how do you expect them to relate to each other?  Should
subtree_branch be an ancestor of subtree_tip?  If so we should
explicitly test that.

Again, thanks for your work on this!  I think I actually may have hit
this bug in my own work but I couldn't be sure I hadn't done something
wrong.  The sequence of commands and splits is eerily similar to
something I tried a while back.  I'm *very* glad you were able to track
it down!

                          -David