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

From: Junio C Hamano <[email-masked]>
Date: Fri, 15 Jan 2016 10:58:03 -0800

Dave Ware <[email-masked]> writes:

> 'git subtree split' can incorrectly skip a merge even when both parents
> act on the subtree, provided the merge results in a tree identical to
> one of the parents. Fix by copying the merge if at least one parent is
> non-identical, and the non-identical parent is not an ancestor of the
> identical parent.
>
> Also, add a test case which checks that a descendant remains a
> descendent on the subtree in this case.
>
> Signed-off-by: Dave Ware <[email-masked]>
> ---

David, how does this round look?  Can we proceed with your (and Eric's)
Reviewed-by: with this version (with one grammo fix Eric pointed out)?

>
> Notes:
>     Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch
>     Also many thanks to David A. Greene for help with subtree test style
>     
>     Changes since v6
>     - I forgot the notes when I sumbitted v6. (I have now set notes.rewriteRef,
>       so hopefully this wont happen again).
>     - Fixed some missing && in my test rewrite.
>     Changes since v5
>     - Rewrote test case to use subtree test repo and commit creation methods
>     - Added comments on what the test does and which bits are checked
>     - Added comments to test on related bugs which aren't fixed yet
>     Changes since v4
>     - Minor spelling and style fixes to test case
>     Changes since v3:
>     - Improvements to commit message
>     - Removed incorrect use of --boundary on rev-list
>     - Changed use of rev-list to use --count
>     Changes since v2:
>     - Minor improvements to commit message
>     - Changed space indentation to tab indentation in test case
>     - Changed use of rev-list for obtaining commit id to use rev-parse instead
>     Changes since v1:
>     - Minor improvements to commit message
>     - Added sign off
>     - Moved test case from own file into t7900-subtree.sh
>     - Added subshell to test around 'cd'
>     - Moved record of commit for cherry-pick to variable instead of dumping into file
>     
>     [v6]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=284095
>     [v5]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282197
>     [v4]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282182
>     [v3]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282176
>     [v2]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282121
>     [v1]: http://thread.gmane.org/gmane.comp.version-control.git/282065
>
>  contrib/subtree/git-subtree.sh     | 12 ++++++--
>  contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index edf36f8..5c83727 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 $?
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 751aee3..3bf96a9 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -1014,4 +1014,64 @@ test_expect_success 'push split to subproj' '
>  	)
>  '
>  
> +#
> +# This test covers 2 cases in subtree split copy_or_skip code
> +# 1) Merges where one parent is a superset of the changes of the other
> +#    parent regarding changes to the subtree, in this case the merge
> +#    commit should be copied
> +# 2) Merges where only one parent operate on the subtree, and the merge
> +#    commit should be skipped
> +#
> +# (1) is checked by ensuring subtree_tip is a descendent of subtree_branch
> +# (2) should have a check added (not_a_subtree_change shouldn't be present
> +#     on the produced subtree)
> +#
> +# Other related cases which are not tested (or currently handled correctly)
> +# - Case (1) where there are more than 2 parents, it will sometimes correctly copy
> +#   the merge, and sometimes not
> +# - Merge commit where both parents have same tree as the merge, currently
> +#   will always be skipped, even if they reached that state via different
> +#   set of commits.
> +#
> +
> +next_test
> +test_expect_success 'subtree descendant check' '
> +	subtree_test_create_repo "$subtree_test_count" &&
> +	test_create_commit "$subtree_test_count" folder_subtree/a &&
> +	(
> +		cd "$subtree_test_count" &&
> +		git branch branch
> +	) &&
> +	test_create_commit "$subtree_test_count" folder_subtree/0 &&
> +	test_create_commit "$subtree_test_count" folder_subtree/b &&
> +	cherry=$(cd "$subtree_test_count"; git rev-parse HEAD) &&
> +	(
> +		cd "$subtree_test_count" &&
> +		git checkout branch
> +	) &&
> +	test_create_commit "$subtree_test_count" commit_on_branch &&
> +	(
> +		cd "$subtree_test_count" &&
> +		git cherry-pick $cherry &&
> +		git checkout master &&
> +		git merge -m "merge should be kept on subtree" branch &&
> +		git branch no_subtree_work_branch
> +	) &&
> +	test_create_commit "$subtree_test_count" folder_subtree/d &&
> +	(
> +		cd "$subtree_test_count" &&
> +		git checkout no_subtree_work_branch
> +	) &&
> +	test_create_commit "$subtree_test_count" not_a_subtree_change &&
> +	(
> +		cd "$subtree_test_count" &&
> +		git checkout master &&
> +		git merge -m "merge should be skipped on subtree" no_subtree_work_branch &&
> +
> +		git subtree split --prefix folder_subtree/ --branch subtree_tip master &&
> +		git subtree split --prefix folder_subtree/ --branch subtree_branch branch &&
> +		check_equal $(git rev-list --count subtree_tip..subtree_branch) 0
> +	)
> +'
> +
>  test_done