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

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

Junio C Hamano <gitster@pobox.com> writes:

> Dave Ware <davidw@realtimegenomics.com> 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 <davidw@realtimegenomics.com>
>> ---
>
> 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)?

Yes, this looks great to me!  Thanks Dave!

                            -David

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