threads / bug / 40946

git subtree bug produces divergent descendants

Subject: git subtree bug produces divergent descendants

## tl;dr

22 messages between Dec 6, 2015 and Jan 17, 2016.

replies: 21people: 4as markdown or json

David Ware· Dec 6, 2015, 22:09 UTC · lore

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

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
Eric Sunshine· Dec 7, 2015, 04:53 UTC · re: David Ware · lore

Re: git subtree bug produces divergent descendants

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/
Show 5 quoted lines
> 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.
Show 21 quoted lines
> ---
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 9f06571..b837531 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -479,8 +479,16 @@ copy_or_skip()
>  			p="$p -p $parent"
>  		fi
>  	done
> -	
> -	if [ -n "$identical" ]; then
> +
> +	copycommit=
> +	if [ -n "$identical" ] && [ -n "$nonidentical" ]; then
> +		extras=$(git rev-list --boundary $identical..$nonidentical)
> +		if [ -n "$extras" ]; then
> +			# we need to preserve history along the other branch
> +			copycommit=1
> +		fi
> +	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.

Show 8 quoted lines
>  		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.

Show 9 quoted lines
> +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.

Show 5 quoted lines
> +  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.

Show 13 quoted lines
> +  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 &&
Show 6 quoted lines
> +  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 &&
Show 25 quoted lines
> +  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
Dave Ware· Dec 7, 2015, 20:50 UTC · re: Eric Sunshine · lore

[PATCH] 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. 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
Eric Sunshine· Dec 8, 2015, 06:49 UTC · re: Dave Ware · lore

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

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
Show 7 quoted lines
> A bug occurs in 'git-subtree split' where a merge is skipped even when
> both parents act on the subtree, provided the merge results in a tree
> identical to one of the parents. Fix by copying the merge if at least
> one parent is non-identical, and the non-identical parent is not an
> ancestor of the identical parent.
>
> Also 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...

Show 16 quoted lines
>  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
Show 18 quoted lines
> +    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)
Show 32 quoted lines
> +    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
Dave Ware· Dec 8, 2015, 20:39 UTC · re: Eric Sunshine · lore

[PATCH v3] 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. 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
Junio C Hamano· Dec 8, 2015, 21:23 UTC · re: Dave Ware · lore

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

Dave Ware <davidw@realtimegenomics.com> writes:
Show 11 quoted lines
> A bug occurs in 'git-subtree split' where a merge is skipped even when
> both parents act on the subtree, provided the merge results in a tree
> identical to one of the parents. Fix by copying the merge if at least
> one parent is non-identical, and the non-identical parent is not an
> ancestor of the identical parent.
>
> Also, add a test case which checks that a descendant can be pushed to
> its ancestor in this case.
>
> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
> ---

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

	'git subtree split' can incorrectly skip a merge even when
        both parents ...
Show 18 quoted lines
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 9f06571..b837531 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -479,8 +479,16 @@ copy_or_skip()
>  			p="$p -p $parent"
>  		fi
>  	done
> -	
> -	if [ -n "$identical" ]; then
> +
> +	copycommit=
> +	if [ -n "$identical" ] && [ -n "$nonidentical" ]; then
> +		extras=$(git rev-list --boundary $identical..$nonidentical)
> +		if [ -n "$extras" ]; then
> +			# we need to preserve history along the other branch
> +			copycommit=1
> +		fi

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

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

Thanks.
Show 66 quoted lines
> +	fi
> +	if [ -n "$identical" ] && [ -z "$copycommit" ]; then
>  		echo $identical
>  	else
>  		copy_commit $rev $tree "$p" || exit $?
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 9051982..710278c 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '
>  	))
>  '
>  
> +test_expect_success 'subtree descendent check' '
> +	mkdir git_subtree_split_check &&
> +	(
> +		cd git_subtree_split_check &&
> +		git init &&
> +
> +		mkdir folder &&
> +
> +		echo a >folder/a &&
> +		git add . &&
> +		git commit -m "first commit" &&
> +
> +		git branch branch &&
> +
> +		echo 0 >folder/0 &&
> +		git add . &&
> +		git commit -m "adding 0 to folder" &&
> +
> +		echo b >folder/b &&
> +		git add . &&
> +		git commit -m "adding b to folder" &&
> +		cherry=$(git rev-parse HEAD) &&
> +
> +		git checkout branch &&
> +		echo text >textBranch.txt &&
> +		git add . &&
> +		git commit -m "commit to fiddle with branch: branch" &&
> +
> +		git cherry-pick $cherry &&
> +		git checkout master &&
> +		git merge -m "merge" branch &&
> +
> +		git branch noop_branch &&
> +
> +		echo d >folder/d &&
> +		git add . &&
> +		git commit -m "adding d to folder" &&
> +
> +		git checkout noop_branch &&
> +		echo moreText >anotherText.txt &&
> +		git add . &&
> +		git commit -m "irrelevant" &&
> +
> +		git checkout master &&
> +		git merge -m "second merge" noop_branch &&
> +
> +		git subtree split --prefix folder/ --branch subtree_tip master &&
> +		git subtree split --prefix folder/ --branch subtree_branch branch &&
> +		git push . subtree_tip:subtree_branch
> +	)
> +	'
> +
>  test_done
David Ware· Dec 9, 2015, 00:16 UTC · re: Junio C Hamano · lore

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

On Wed, Dec 9, 2015 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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.
Show 25 quoted lines
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 9f06571..b837531 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -479,8 +479,16 @@ copy_or_skip()
>>                       p="$p -p $parent"
>>               fi
>>       done
>> -
>> -     if [ -n "$identical" ]; then
>> +
>> +     copycommit=
>> +     if [ -n "$identical" ] && [ -n "$nonidentical" ]; then
>> +             extras=$(git rev-list --boundary $identical..$nonidentical)
>> +             if [ -n "$extras" ]; then
>> +                     # we need to preserve history along the other branch
>> +                     copycommit=1
>> +             fi
>
> What is the significance of "--boundary" here?  I think for the
> purpose of "is the identical one part of the nonidentical one?" you
> do not need it, but there may be something subtle I missed.  I am
> asking this because use of "rev-list --boundary" in scripts is
> almost always a bug.
>
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.
Show 5 quoted lines
> 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.
Dave Ware· Dec 9, 2015, 00:19 UTC · re: Junio C Hamano · lore

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

'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
Eric Sunshine· Dec 9, 2015, 07:52 UTC · re: Dave Ware · lore

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

On Tue, Dec 8, 2015 at 7:19 PM, Dave Ware <davidw@realtimegenomics.com> wrote:
Show 17 quoted lines
> '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/
Show 7 quoted lines
> +       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
Dave Ware· Dec 9, 2015, 21:17 UTC · re: Eric Sunshine · lore

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

'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
David A. Greene· Jan 13, 2016, 03:27 UTC · re: Dave Ware · lore

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

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!  ]
Show 23 quoted lines
> 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?

Show 13 quoted lines
> 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.

Show 5 quoted lines
> +		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?

Show 6 quoted lines
> +		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
David Ware· Jan 13, 2016, 19:33 UTC · re: David A. Greene · lore

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

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

Show 27 quoted lines
>> 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.

Show 23 quoted lines
>> +             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

Show 15 quoted lines
>> +             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.

Show 7 quoted lines
> 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

David A. Greene· Jan 14, 2016, 03:12 UTC · re: David Ware · lore

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

David Ware <davidw@realtimegenomics.com> writes:
Show 8 quoted lines
>> 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.

Show 5 quoted lines
> 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.

Show 15 quoted lines
> 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.
Show 9 quoted lines
>> 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.

Show 16 quoted lines
>>> +             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.

Show 24 quoted lines
>>> +             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.

Show 5 quoted lines
> 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.
Show 6 quoted lines
> - 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
David Ware· Jan 14, 2016, 20:45 UTC · re: David A. Greene · lore

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

On Thu, Jan 14, 2016 at 4:12 PM, David A. Greene <greened@obbligato.org> wrote:
Show 5 quoted lines
> 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.

Show 14 quoted lines
>
>> 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.

Show 9 quoted lines
>> 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.
Show 16 quoted lines
> 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

David A. Greene· Jan 17, 2016, 22:40 UTC · re: David Ware · lore

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

David Ware <davidw@realtimegenomics.com> writes:
Show 12 quoted lines
> 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.

Show 14 quoted lines
>> 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.
Show 11 quoted lines
>>> 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
Dave Ware· Jan 14, 2016, 21:26 UTC · re: David A. Greene · lore

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

'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
Dave Ware· Jan 15, 2016, 00:41 UTC · re: Dave Ware · lore

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

'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
Eric Sunshine· Jan 15, 2016, 01:06 UTC · re: Dave Ware · lore

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

On Thu, Jan 14, 2016 at 7:41 PM, Dave Ware <davidw@realtimegenomics.com> wrote:
Show 18 quoted lines
> '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/
Show 9 quoted lines
> +#    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
Show 34 quoted lines
> +       ) &&
> +       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
Junio C Hamano· Jan 15, 2016, 18:58 UTC · re: Dave Ware · lore

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

Dave Ware <davidw@realtimegenomics.com> writes:
Show 11 quoted lines
> '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)?
Show 133 quoted lines
>
> 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
Eric Sunshine· Jan 15, 2016, 23:24 UTC · re: Junio C Hamano · lore

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

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

David A. Greene· Jan 17, 2016, 22:41 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> 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
Show 127 quoted lines
>>
>> 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
David Ware· Dec 7, 2015, 21:01 UTC · re: Eric Sunshine · lore

Re: git subtree bug produces divergent descendants

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:
Show 17 quoted lines
> 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

← back to recent threads