git subtree bug produces divergent descendants

git subtree bug produces divergent descendants

[David Ware]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

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). We originally noticed this on v1.9.1 but have also checked it
on v2.6.3

When scanning the commits to produce the subtree it seems to skip
creating a new commit if any of the parent commits have the same tree
and instead uses that tree in its place. This is fine when the cause
is a branch that did not cause any changes to the subtree.  However it
creates an issue when the cause is both branches ending up with the
same tree through identical alterations (or more likely, one of the
branches has just a subset of the alterations on the other, such as a
branch just containing cherry-picks).

The attached patch (against v2.6.3) includes a test that reproduces
the problem.  The created 'master' branch has had the latest commits
on the 'branch' branch merged into it, so it follows that a subtree on
'folder/' at 'master' (subtree_tip) should contain all the commits of
a subtree on 'folder/' at 'branch' (subtree_branch). Hence it should
be possible to push subtree_tip to subtree_branch.

The attached patch also fixes the issue for the cases we've
encountered, however since we're not particularly familiar with git
internals we may not have approached this optimally. We suspect it
could be improved to also handle the cases where there are more than 2
parents.

Cheers,
Dave Ware

[-- Attachment #2: 0001-Fix-bug-in-git-subtree-split.patch --]
[-- Type: text/x-patch, Size: 3135 bytes --]

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

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
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.
---
 contrib/subtree/git-subtree.sh           | 12 +++++--
 contrib/subtree/t/t7901-subtree-split.sh | 62 ++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100755 contrib/subtree/t/t7901-subtree-split.sh

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
 		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
@@ -0,0 +1,62 @@
+#!/bin/bash
+
+test_description='Test for bug in subtree commit filtering'
+
+
+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 &&
+  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" &&
+  git rev-list HEAD -1 > cherry.rev &&
+
+  git checkout branch &&
+  echo text > textBranch.txt &&
+  git add . &&
+  git commit -m "commit to fiddle with branch: branch" &&
+
+  git cherry-pick $(cat cherry.rev) &&
+  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

Re: git subtree bug produces divergent descendants

[Eric Sunshine]

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

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

[Dave Ware]

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 adding a test case, this checks that a descendant can be pushed to
it's ancestor in this case.

Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
---
 contrib/subtree/git-subtree.sh     | 12 +++++++--
 contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

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
 		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..ea991eb 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-list HEAD -1) &&
+
+    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
-- 
1.9.1

Re: git subtree bug produces divergent descendants

[David Ware]

Thanks for taking the time to look over it. I'm not familiar with the
process here.

On Mon, Dec 7, 2015 at 5:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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 &&
>         ... &&
>         ...
>     )
>

I'm not at all familiar with this test harness so I had a few problems
here (like this, and the bash variable). Thank you for the advice.

Cheers,
Dave Ware

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

[Eric Sunshine]

On Mon, Dec 7, 2015 at 3:50 PM, Dave Ware <davidw@realtimegenomics.com> wrote:
> [PATCH] contrib/subtree: fix "subtree split" skipped-merge bug.

As an aid for reviewers, please indicate the version of this patch
submission. For instance, this is the second attempt, so the subject
would be decorated as [PATCH v2], and the next one (if submitted) will
be v3. The -v option of git-format-patch can help automate this.

Style: drop the full-stop (period) from the subject line

> 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 adding a test case, this checks that a descendant can be pushed to

s/Also adding/Also, add/
s/, this/which/

> it's ancestor in this case.

s/it's/its/

> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
> ---

Right here below the "---" line is a good place to describe what
changed since the previous version. For instance, in v2, you made
minor improvements to the commit message, added your sign-off, folded
the new test into the existing t7900-subtree.sh, added a subshell
around 'cd', and assigned the output of git-rev-list to a shell
variable rather than dumping it to a file.

Including a link to the previous version, like this[1], is also
reviewer-friendly.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/282065

As before, I'm not a git-subtree user, so this review is superficial.
More below...

>  contrib/subtree/git-subtree.sh     | 12 +++++++--
>  contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 9051982..ea991eb 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 &&

Style: indent with tabs rather than spaces

> +    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-list HEAD -1) &&

git-rev-parse would probably be more idiomatic:

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

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

[Dave Ware]

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

Notes:
    Many thanks to Eric Sunshine for his adivce on this patch
    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
    
    [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 | 52 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

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

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

[Junio C Hamano]

Dave Ware <davidw@realtimegenomics.com> writes:

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

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

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

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

[David Ware]

On Wed, Dec 9, 2015 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Ware <davidw@realtimegenomics.com> writes:
>
>> 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 ...
>

Noted.

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

The other way around actually I'm trying to determine if nonidentical
contains any commits
 not in identical.  I'll confess I don't actually know specifically
what the --boundary option
does, this probably came from a stack overflow example while we were
looking up how to
best do the check. Further experimentation with the option suggests
that it does not do what
I want, so I will remove it. Thank you.

> 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, I didn't know about that option.

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

[Dave Ware]

'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 can be pushed to
its ancestor in this case.

Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
---

Notes:
    Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch
    
    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
    
    [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 | 52 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

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 $?
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
-- 
1.9.1

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

[Eric Sunshine]

On Tue, Dec 8, 2015 at 7:19 PM, Dave Ware <davidw@realtimegenomics.com> wrote:
> '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 can be pushed to
> its ancestor in this case.
>
> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
> ---
> diff --git 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' '

s/descendent/descendant/

> +       mkdir git_subtree_split_check &&
> +       (
> +               cd git_subtree_split_check &&
> +[...]
> +               git push . subtree_tip:subtree_branch
> +       )
> +       '

Style nit: don't indent closing quotation mark

>  test_done
> --
> 1.9.1

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

[Dave Ware]

'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 can be pushed to
its ancestor in this case.

Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
---

Notes:
    Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch
    
    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
    
    [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 | 52 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

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 $?
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 &&
+
+		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
-- 
1.9.1

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

[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

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

[David Ware]

On Wed, Jan 13, 2016 at 4:27 PM, David A. Greene <greened@obbligato.org> wrote:
> 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?
>

The commit was made against v2.6.3, when I try to apply the patch
against master it fails.

However I can verify the test passes for me when applied against
v2.6.3, and it also passed if I merge my patched copy of v2.6.3 into
master. The process I'm using to run the tests is a little strange
though, it seems I have to make git, then make contrib/subtree, then
cp git-subtree to the root before running the Makefile on the tests.
Let me know if there's a less strange process for running the subtree
tests.

The test case actually began life as a bash script I was running
manually and visually inspecting. It covers the 2 cases we needed in
order to push our release
1) Merges where one parent is a superset of the changes of the other
parents regarding changes to the subtree, in this case the merge
commit should be copied (represented by "merge" in test case)
2) Merges where only one parent operate on the subtree, and the merge
commit should be skipped (represented by "second merge" in test case)

I haven't done an in depth look to verify the test checks the second
case, since this bit was never actually broken. But in terms of what
the test case should be doing only the first merge should be preserved
in the subtree

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

I'm not a regular contributor to git (this is my first). So I'm not
very familiar with the test harness.
Also as noted I created the patch against v2.6.3, which did not have
the changes you mentioned.

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

This is to create a merge that operates workflow (2) mentioned above,
i.e. a branch that has absolutely no effect on the subtree and as such
should be skipped

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

it should look like this:

R--A1--A2-----M---H
  \               /
   B-------------

Where H is subtree_tip and B is subtree_branch. So yes subtree_tip is
a descendant of subtree_branch

Agreed, it should probably be checking things more explicitly. And
ideally should also be checking that commit "irrelevant" and "second
merge" are being skipped if possible.

> 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

As I noted in my original email this patch is solely designed to fix
the issue we ran into whilst trying to make our release (essentially
(1) and (2) mentioned above) and other cases of this same issue are
not addressed.
i.e.
- The many parent case. I've made no attempt to handle this situation
properly in the presence of greater than 2 parents. In theory it will
now sometimes correctly copy the merge where it wouldn't before, and
sometimes use the old behaviour.
- This is one I've only realised since submitting the patch: The case
where both parents have an identical tree to the merge commit, they
don't necessarily have the same set of commits to achieve this state,
so this should be being checked as well. Again I don't think this
patch makes this situation worse, it will simply result in the old
behaviour being used.

Thanks for taking the time to look at this.

Cheers,
Dave Ware

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

[David A. Greene]

David Ware <davidw@realtimegenomics.com> writes:

>> 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?
>>
>
> The commit was made against v2.6.3, when I try to apply the patch
> against master it fails.

Any ideas why?

> However I can verify the test passes for me when applied against
> v2.6.3, and it also passed if I merge my patched copy of v2.6.3 into
> master.

I don't think the subtree split code has changed at all in that period
and the logs bear that out.  So there must be some change in
v2.6.3..master that confounds your patch.

Re-checking the patch submission guidelines, it looks like bugfixes
should be based against maint.  I did that and the test still fails with
your changes.  It seems like we ought to rebase to maint and continue
our investigation there.

> The process I'm using to run the tests is a little strange though, it
> seems I have to make git, then make contrib/subtree, then cp
> git-subtree to the root before running the Makefile on the tests.  Let
> me know if there's a less strange process for running the subtree
> tests.

I actually have an update that makes this easier but I haven't submitted
it yet.  But yes, you've got the current process right.

> The test case actually began life as a bash script I was running
> manually and visually inspecting. It covers the 2 cases we needed in
> order to push our release
>
> 1) Merges where one parent is a superset of the changes of the other
> parents regarding changes to the subtree, in this case the merge
> commit should be copied (represented by "merge" in test case)
>
> 2) Merges where only one parent operate on the subtree, and the merge
> commit should be skipped (represented by "second merge" in test case)
>
> I haven't done an in depth look to verify the test checks the second
> case, since this bit was never actually broken. But in terms of what
> the test case should be doing only the first merge should be preserved
> in the subtree

Ok, thanks.  More on this below.

>> 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.
>>
>
> I'm not a regular contributor to git (this is my first). So I'm not
> very familiar with the test harness.

:)  Welcome!  I'm just (re-)starting work on git-subtree myself so we're
on the same learning curve.  I inherited the code from the original
author and we're slowly cleaning it up.  The goal is to get it out of
contrib and add some useful features.

> Also as noted I created the patch against v2.6.3, which did not have
> the changes you mentioned.

Ok.  Your patch applied cleanly to maint and maint has the latest
version of the test file.  It should be just a matter of following what
the other tests do.  I'm more than happy to guide you through it.

>>> +             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?
>>
>
> This is to create a merge that operates workflow (2) mentioned above,
> i.e. a branch that has absolutely no effect on the subtree and as such
> should be skipped

Ok.  Some comments to that effect would be great.  Something like what
your wrote describing (1) and (2) about would help a lot.  I'd still
like to see these names cleaned up because they confused me when I
looked at it.  Perhaps "no_subtree_work_branch" and "Non-subtree
change?"  Feel free to pick your own names if you think of something
better.

>>> +             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.
>>
>
> it should look like this:
>
> R--A1--A2-----M---H
>   \               /
>    B-------------
>
> Where H is subtree_tip and B is subtree_branch. So yes subtree_tip is
> a descendant of subtree_branch

Ok.

> Agreed, it should probably be checking things more explicitly. And
> ideally should also be checking that commit "irrelevant" and "second
> merge" are being skipped if possible.

Right.  If you want to add those tests, great.  Otherwise, please add a
comment describing them so that others can add them later.  I just don't
want to forget to test things we know about but I don't want it to hold
up your patch.

> As I noted in my original email this patch is solely designed to fix
> the issue we ran into whilst trying to make our release (essentially
> (1) and (2) mentioned above) and other cases of this same issue are
> not addressed.
> i.e.

> - The many parent case. I've made no attempt to handle this situation
> properly in the presence of greater than 2 parents. In theory it will
> now sometimes correctly copy the merge where it wouldn't before, and
> sometimes use the old behaviour.

> - This is one I've only realised since submitting the patch: The case
> where both parents have an identical tree to the merge commit, they
> don't necessarily have the same set of commits to achieve this state,
> so this should be being checked as well. Again I don't think this
> patch makes this situation worse, it will simply result in the old
> behaviour being used.

You certainly don't have to test and/or fix every potential problem with
this patch.  Noting them in the test via comments would help guide
others to write tests for them and/or fix the problems.  Could you add a
block comment before the test that describes scenarios (1) and (2),
talks about the status of testing them in the test (i.e. (2) isn't
tested) and explains the potential problems listed above that are not
being tested?  Thanks!

Again, thank you for your contributions!

                           -David

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

[David Ware]

On Thu, Jan 14, 2016 at 4:12 PM, David A. Greene <greened@obbligato.org> wrote:
> David Ware <davidw@realtimegenomics.com> writes:
>> The commit was made against v2.6.3, when I try to apply the patch
>> against master it fails.
>
> Any ideas why?

"git am" (a command I have never used before) Fails like so

Applying: contrib/subtree: fix "subtree split" skipped-merge bug
error: patch failed: contrib/subtree/t/t7900-subtree.sh:468
error: contrib/subtree/t/t7900-subtree.sh: patch does not apply

It doesn't even put any files into a conflict state.
I guess it's because of the hefty test refactoring you mentioned.

>
>> However I can verify the test passes for me when applied against
>> v2.6.3, and it also passed if I merge my patched copy of v2.6.3 into
>> master.
>
> I don't think the subtree split code has changed at all in that period
> and the logs bear that out.  So there must be some change in
> v2.6.3..master that confounds your patch.
>
> Re-checking the patch submission guidelines, it looks like bugfixes
> should be based against maint.  I did that and the test still fails with
> your changes.  It seems like we ought to rebase to maint and continue
> our investigation there.
>

Hmm, the patch fails to apply for me there also. Same issue with
contrib/subtree/t/t7900-subtree.sh

I haven't worked with mailed patches at all before, so it is possible
I'm not using the correct workflow (I just saved the raw email I
received for the patch as txt and fed it to 'git am').
Cherrypicking the commit onto maint works fine though, and the test
passes for me in this situation.

>> The process I'm using to run the tests is a little strange though, it
>> seems I have to make git, then make contrib/subtree, then cp
>> git-subtree to the root before running the Makefile on the tests.  Let
>> me know if there's a less strange process for running the subtree
>> tests.
>
> I actually have an update that makes this easier but I haven't submitted
> it yet.  But yes, you've got the current process right.
>

That will be nice.

> Ok.  Your patch applied cleanly to maint and maint has the latest
> version of the test file.  It should be just a matter of following what
> the other tests do.  I'm more than happy to guide you through it.
>
>>>> +             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?
>>>

As noted above I can't get the patch to apply cleanly to maint for me,
but I suppose it doesn't matter since I'm about to mail in a new
version created against maint.
I've rewritten the test to use the repo/commit creation methods, and
renamed that branch. I've also added the comments you requested, and
changed the push to an ancestor check.
I'll be submitting the new version of the patch shortly.

Cheers,
Dave Ware

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

[Dave Ware]

'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>
---
 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..c5089c3 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
-- 
1.9.1

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

[Dave Ware]

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

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

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

[Eric Sunshine]

On Thu, Jan 14, 2016 at 7:41 PM, Dave Ware <davidw@realtimegenomics.com> wrote:
> '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>
> ---
> diff --git 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

s/operate/operates/

> +#    commit should be skipped
> +#
> +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

Not worth a re-roll (and probably not worthwhile anyhow since it would
be inconsistent with the rest of the script), but for these really
simple cases, you can use -C and avoid the subshell altogether:

    git -C "$subtree_test_count" 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

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

[Junio C Hamano]

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

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

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

[Eric Sunshine]

On Fri, Jan 15, 2016 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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)?

As I'm not a subtree user, I'm not qualified to give a Reviewed-by:;
my review comments were general, not specific to subtree
functionality. At best, that might qualify for a Helped-by: if my
comments had any value.

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

[David A. Greene]

David Ware <davidw@realtimegenomics.com> writes:

> On Thu, Jan 14, 2016 at 4:12 PM, David A. Greene <greened@obbligato.org> wrote:
>> David Ware <davidw@realtimegenomics.com> writes:
>>> The commit was made against v2.6.3, when I try to apply the patch
>>> against master it fails.
>>
>> Any ideas why?
>
> "git am" (a command I have never used before) Fails like so
>
> Applying: contrib/subtree: fix "subtree split" skipped-merge bug
> error: patch failed: contrib/subtree/t/t7900-subtree.sh:468
> error: contrib/subtree/t/t7900-subtree.sh: patch does not apply

Oh I'm sorry, I misunderstood.  I thought you meant that the patch
applied but the test failed.

> It doesn't even put any files into a conflict state.
> I guess it's because of the hefty test refactoring you mentioned.

You should be able to just paste your new test right to the end of the
updated test file.  The tests were refactored to make each test
independent of the other.  There's no functionality change at all.

>> Re-checking the patch submission guidelines, it looks like bugfixes
>> should be based against maint.  I did that and the test still fails with
>> your changes.  It seems like we ought to rebase to maint and continue
>> our investigation there.
>>
>
> Hmm, the patch fails to apply for me there also. Same issue with
> contrib/subtree/t/t7900-subtree.sh
>
> I haven't worked with mailed patches at all before, so it is possible
> I'm not using the correct workflow (I just saved the raw email I
> received for the patch as txt and fed it to 'git am').
> Cherrypicking the commit onto maint works fine though, and the test
> passes for me in this situation.

Ok, that's probably just fine.  I've not used git-am myself either.

>>> The process I'm using to run the tests is a little strange though, it
>>> seems I have to make git, then make contrib/subtree, then cp
>>> git-subtree to the root before running the Makefile on the tests.  Let
>>> me know if there's a less strange process for running the subtree
>>> tests.
>>
>> I actually have an update that makes this easier but I haven't submitted
>> it yet.  But yes, you've got the current process right.
>>
>
> That will be nice.

I submitted it yesterday.  Might take another round and then a few days
to get it in.  I believe it would go into master since it's a new
"feature" in the Makefile.

> I've rewritten the test to use the repo/commit creation methods, and
> renamed that branch. I've also added the comments you requested, and
> changed the push to an ancestor check.
> I'll be submitting the new version of the patch shortly.

Thank you!

                    -David

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

[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

Re: git subtree bug produces divergent descendants

[David Ware]

Sorry for the double post, I received a mail blocking notification
message (due to the attached .sh file) and erroneously thought this
message had been blocked from the entire list. My later one includes
the test case as part of the attached patch.

Cheers,
Dave Ware

On Mon, Dec 7, 2015 at 9:41 AM, David Ware <davidw@realtimegenomics.com> 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). We originally noticed this on v1.9.1 but have also checked it
> on v2.6.3
>
> When scanning the commits to produce the subtree it seems to skip
> creating a new commit if any of the parent commits have the same tree
> and instead uses that tree in its place. This is fine when the cause
> is a branch that did not cause any changes to the subtree.  However it
> creates an issue when the cause is both branches ending up with the
> same tree through identical alterations (or more likely, one of the
> branches has just a subset of the alterations on the other, such as a
> branch just containing cherry-picks).
>
> The attached bash script (makerepo.sh) reproduces the problem. To use
> create an empty directory and run the script in it. The resulting
> 'master' branch has had the latest commits on the 'branch' branch
> merged into it, so it follows that a subtree on 'folder/' at 'master'
> should contain all the commits of a subtree on 'folder/' at 'branch'.
> (These subtrees have been produced at 'subtree_tip' and
> 'subtree_branch' respectively.)
>
> The attached patch (against v2.6.3) fixes the issue for the cases
> we've encountered, however since we're not particularly familiar with
> git internals we may not have approached this optimally. We suspect it
> could be improved to also handle the cases where there are more than 2
> parents.
>
> Cheers,
> Dave Ware

git subtree bug produces divergent descendants

[David Ware]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

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). We originally noticed this on v1.9.1 but have also checked it
on v2.6.3

When scanning the commits to produce the subtree it seems to skip
creating a new commit if any of the parent commits have the same tree
and instead uses that tree in its place. This is fine when the cause
is a branch that did not cause any changes to the subtree.  However it
creates an issue when the cause is both branches ending up with the
same tree through identical alterations (or more likely, one of the
branches has just a subset of the alterations on the other, such as a
branch just containing cherry-picks).

The attached bash script (makerepo.sh) reproduces the problem. To use
create an empty directory and run the script in it. The resulting
'master' branch has had the latest commits on the 'branch' branch
merged into it, so it follows that a subtree on 'folder/' at 'master'
should contain all the commits of a subtree on 'folder/' at 'branch'.
(These subtrees have been produced at 'subtree_tip' and
'subtree_branch' respectively.)

The attached patch (against v2.6.3) fixes the issue for the cases
we've encountered, however since we're not particularly familiar with
git internals we may not have approached this optimally. We suspect it
could be improved to also handle the cases where there are more than 2
parents.

Cheers,
Dave Ware

[-- Attachment #2: makerepo.sh --]
[-- Type: application/x-sh, Size: 945 bytes --]

[-- Attachment #3: 0001-Fix-bug-in-git-subtree-split.patch --]
[-- Type: text/x-patch, Size: 1287 bytes --]

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

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
one parent is non-identical, and the non-identical parent is not an
ancestor of the identical parent.
---
 contrib/subtree/git-subtree.sh | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

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
 		echo $identical
 	else
 		copy_commit $rev $tree "$p" || exit $?
-- 
1.9.1