Re: git subtree bug produces divergent descendants

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Dec 07, 2015 at 11:09:48AM +1300, David Ware wrote:
> My group has run into a bug with "git-subtree split". Under some
> circumstances a split created from a descendant of another earlier
> split is not a descendant of that earlier split (thus blocking
> pushes). [...]

I'm not a git-subtree user, so this review will be superficial.

> The attached patch (against v2.6.3) includes a test that reproduces
> the problem. [...]

Please include patches inline rather than as attachments since
reviewers will want to comment on portions of the patch as part of
their response to your email. Patches as attachments make this
process more painful.

> From: Dave Ware <davidw@netvalue.net.nz>
> Date: Fri, 4 Dec 2015 16:30:03 +1300
> Subject: [PATCH] Fix bug in git-subtree split.

For the subject, mention the area you're working on, followed by a
colon, followed by a concise description of the problem. If possible,
try to say something more specific than "fix bug". You might, for
instance, say something like:

    contrib/subtree: fix "subtree split" skipped-merge bug

> 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. Fixed by copying the merge if at least

Imperative mood: s/Fixed/Fix/

> one parent is non-identical, and the non-identical parent is not an
> ancestor of the identical parent.
> 
> Also adding a test case, this checks that a descendant can be pushed to
> it's ancestor in this case.

Your Signed-off-by: is missing. See Documentation/SubmittingPatches.

> ---
> 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
> +	fi
> +	if [ -n "$identical" ] && [ -z "$copycommit" ]; then

Typically, I'd say something about how this project uses 'test'
rather than '[' and that 'then' is placed on its own line (with no
semicolon), however, in this case, you're sticking to existing style
(in this script), so I won't mention it.

>  		echo $identical
>  	else
>  		copy_commit $rev $tree "$p" || exit $?
> diff --git a/contrib/subtree/t/t7901-subtree-split.sh b/contrib/subtree/t/t7901-subtree-split.sh
> new file mode 100755
> index 0000000..0a1ea56
> --- /dev/null
> +++ b/contrib/subtree/t/t7901-subtree-split.sh

Is there a strong reason why this demands a new test script rather
than being incorporated into the existing t7900-subtree.sh?

> @@ -0,0 +1,62 @@
> +#!/bin/bash
> +
> +test_description='Test for bug in subtree commit filtering'

A somewhat strange description. Typically, scripts want to verify
correct behavior, rather than buggy behavior.

> +TEST_DIRECTORY=$(pwd)/../../../t
> +export TEST_DIRECTORY
> +
> +. ../../../t/test-lib.sh
> +
> +
> +test_expect_success 'subtree descendent check' '
> +  mkdir git_subtree_split_check &&
> +  cd git_subtree_split_check &&

Tests don't automatically return to the directory prior to the 'cd',
so when this test ends, the current directory will still be
'git_subtree_split_check'. If someone later adds a test following
this one, that test will execute within 'git_subtree_split_check',
which might not be expected by the test writer.

To ensure that the prior working directory is restored at the end of
the test (regardless of success or failure), tests typically employ a
subshell using this idiom:

    mkdir foo &&
    (
        cd foo &&
        ... &&
        ...
    )

In this case, though, I'm wondering what is the purpose of having the
'git_subtree_split_check' subdirectory at all? Is there a reason you
can't just perform the test in the existing directory created
automatically specifically for the test script (which is already the
script's current working directory)? If, on the other hand, you
incorporate this test into t7900-subtree.sh, then the separate
'git_subtree_split_check' directory may make sense if it needs to be
isolated from the other gunk in that script's test directory.

> +  git init &&
> +
> +  mkdir folder &&
> +
> +  echo a > folder/a &&

Typical style is to drop the space after the redirection operator,
however, since you're following existing style in t7900-subtree.sh, I
won't mention it.

> +  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" &&
> +  git rev-list HEAD -1 > cherry.rev &&

Can this value instead just be assigned to a shell variable rather
than being dumped to a file?

    cherryrev=$(git rev-list HEAD -1) &&
    ... &&
    git cherry-pick $cherryrev &&

> +  git checkout branch &&
> +  echo text > textBranch.txt &&
> +  git add . &&
> +  git commit -m "commit to fiddle with branch: branch" &&
> +
> +  git cherry-pick $(cat cherry.rev) &&

See above: git cherry-pick $cherryrev &&

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