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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2015, 21:23 UTC
Message-ID: <xmqqk2ook52u.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1449607160-20608-1-git-send-email-davidw@realtimegenomics.com>

Dave Ware <davidw@realtimegenomics.com> writes:

Show 11 quoted lines

> A bug occurs in 'git-subtree split' where a merge is skipped 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 can be pushed to
> its ancestor in this case.
>
> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
> ---

The first sentence may be made clearer if you rephrased the early part of the sentence this way:

	'git subtree split' can incorrectly skip a merge even when
        both parents ...

Show 18 quoted lines

> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 9f06571..b837531 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 --boundary $identical..$nonidentical)
> +		if [ -n "$extras" ]; then
> +			# we need to preserve history along the other branch
> +			copycommit=1
> +		fi

What is the significance of "--boundary" here? I think for the purpose of "is the identical one part of the nonidentical one?" you do not need it, but there may be something subtle I missed. I am asking this because use of "rev-list --boundary" in scripts is almost always a bug.

Also, depending on how huge the output from the rev-list could be, you might want to use "rev-list --count $i..$n" and compare it with 0 instead--that way, you would not have to be worried about having to carry around a huge string that you would otherwise not use, only to see if that string is empty.

Thanks.

Show 66 quoted lines

> +	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 9051982..710278c 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 descendent check' '
> +	mkdir git_subtree_split_check &&
> +	(
> +		cd git_subtree_split_check &&
> +		git init &&
> +
> +		mkdir folder &&
> +
> +		echo a >folder/a &&
> +		git add . &&
> +		git commit -m "first commit" &&
> +
> +		git branch branch &&
> +
> +		echo 0 >folder/0 &&
> +		git add . &&
> +		git commit -m "adding 0 to folder" &&
> +
> +		echo b >folder/b &&
> +		git add . &&
> +		git commit -m "adding b to folder" &&
> +		cherry=$(git rev-parse HEAD) &&
> +
> +		git checkout branch &&
> +		echo text >textBranch.txt &&
> +		git add . &&
> +		git commit -m "commit to fiddle with branch: branch" &&
> +
> +		git cherry-pick $cherry &&
> +		git checkout master &&
> +		git merge -m "merge" branch &&
> +
> +		git branch noop_branch &&
> +
> +		echo d >folder/d &&
> +		git add . &&
> +		git commit -m "adding d to folder" &&
> +
> +		git checkout noop_branch &&
> +		echo moreText >anotherText.txt &&
> +		git add . &&
> +		git commit -m "irrelevant" &&
> +
> +		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
> +	)
> +	'
> +
>  test_done