{"thread":{"id":"40946","subject":"git subtree bug produces divergent descendants","startedAt":"2015-12-06T22:09:48Z","lastAt":"2016-01-17T22:41:40Z","messageCount":22,"participants":["David Ware","Eric Sunshine","Dave Ware","Junio C Hamano","David A. Greene"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"274083","messageId":"CAET=KiVXh2UZwRSpM_+wX_QpfjBsyfdPPUVDSDoCRVe_0wbhCg@mail.gmail.com","threadId":"40946","inReplyTo":null,"subject":"git subtree bug produces divergent descendants","fromName":"David Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2015-12-06T22:09:48Z","receivedAt":"2015-12-06T22:09:48Z","isPatch":false,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"My group has run into a bug with \"git-subtree split\". Under some\ncircumstances a split created from a descendant of another earlier\nsplit is not a descendant of that earlier split (thus blocking\npushes). We originally noticed this on v1.9.1 but have also checked it\non v2.6.3\n\nWhen scanning the commits to produce the subtree it seems to skip\ncreating a new commit if any of the parent commits have the same tree\nand instead uses that tree in its place. This is fine when the cause\nis a branch that did not cause any changes to the subtree.  However it\ncreates an issue when the cause is both branches ending up with the\nsame tree through identical alterations (or more likely, one of the\nbranches has just a subset of the alterations on the other, such as a\nbranch just containing cherry-picks).\n\nThe attached patch (against v2.6.3) includes a test that reproduces\nthe problem.  The created 'master' branch has had the latest commits\non the 'branch' branch merged into it, so it follows that a subtree on\n'folder/' at 'master' (subtree_tip) should contain all the commits of\na subtree on 'folder/' at 'branch' (subtree_branch). Hence it should\nbe possible to push subtree_tip to subtree_branch.\n\nThe attached patch also fixes the issue for the cases we've\nencountered, however since we're not particularly familiar with git\ninternals we may not have approached this optimally. We suspect it\ncould be improved to also handle the cases where there are more than 2\nparents.\n\nCheers,\nDave Ware\n\n\nFrom ce6e2bcb2116624082bf46663aa33c706fcab930 Mon Sep 17 00:00:00 2001\nFrom: Dave Ware <davidw@netvalue.net.nz>\nDate: Fri, 4 Dec 2015 16:30:03 +1300\nSubject: [PATCH] Fix bug in git-subtree split.\n\nA bug occurs in 'git-subtree split' where a merge is skipped even when\nboth parents act on the subtree, provided the merge results in a tree\nidentical to one of the parents. Fixed by copying the merge if at least\none parent is non-identical, and the non-identical parent is not an\nancestor of the identical parent.\n\nAlso adding a test case, this checks that a descendant can be pushed to\nit's ancestor in this case.\n---\n contrib/subtree/git-subtree.sh           | 12 +++++--\n contrib/subtree/t/t7901-subtree-split.sh | 62 ++++++++++++++++++++++++++++++++\n 2 files changed, 72 insertions(+), 2 deletions(-)\n create mode 100755 contrib/subtree/t/t7901-subtree-split.sh\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 9f06571..b837531 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -479,8 +479,16 @@ copy_or_skip()\n \t\t\tp=\"$p -p $parent\"\n \t\tfi\n \tdone\n-\t\n-\tif [ -n \"$identical\" ]; then\n+\n+\tcopycommit=\n+\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n+\t\textras=$(git rev-list --boundary $identical..$nonidentical)\n+\t\tif [ -n \"$extras\" ]; then\n+\t\t\t# we need to preserve history along the other branch\n+\t\t\tcopycommit=1\n+\t\tfi\n+\tfi\n+\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n \t\techo $identical\n \telse\n \t\tcopy_commit $rev $tree \"$p\" || exit $?\ndiff --git a/contrib/subtree/t/t7901-subtree-split.sh b/contrib/subtree/t/t7901-subtree-split.sh\nnew file mode 100755\nindex 0000000..0a1ea56\n--- /dev/null\n+++ b/contrib/subtree/t/t7901-subtree-split.sh\n@@ -0,0 +1,62 @@\n+#!/bin/bash\n+\n+test_description='Test for bug in subtree commit filtering'\n+\n+\n+TEST_DIRECTORY=$(pwd)/../../../t\n+export TEST_DIRECTORY\n+\n+. ../../../t/test-lib.sh\n+\n+\n+test_expect_success 'subtree descendent check' '\n+  mkdir git_subtree_split_check &&\n+  cd git_subtree_split_check &&\n+  git init &&\n+\n+  mkdir folder &&\n+\n+  echo a > folder/a &&\n+  git add . &&\n+  git commit -m \"first commit\" &&\n+\n+  git branch branch &&\n+\n+  echo 0 > folder/0 &&\n+  git add . &&\n+  git commit -m \"adding 0 to folder\" &&\n+\n+  echo b > folder/b &&\n+  git add . &&\n+  git commit -m \"adding b to folder\" &&\n+  git rev-list HEAD -1 > cherry.rev &&\n+\n+  git checkout branch &&\n+  echo text > textBranch.txt &&\n+  git add . &&\n+  git commit -m \"commit to fiddle with branch: branch\" &&\n+\n+  git cherry-pick $(cat cherry.rev) &&\n+  git checkout master &&\n+  git merge -m \"merge\" branch &&\n+\n+  git branch noop_branch &&\n+\n+  echo d > folder/d &&\n+  git add . &&\n+  git commit -m \"adding d to folder\" &&\n+\n+  git checkout noop_branch &&\n+  echo moreText > anotherText.txt &&\n+  git add . &&\n+  git commit -m \"irrelevant\" &&\n+\n+  git checkout master &&\n+  git merge -m \"second merge\" noop_branch &&\n+\n+  git subtree split --prefix folder/ --branch subtree_tip master &&\n+  git subtree split --prefix folder/ --branch subtree_branch branch &&\n+  git push . subtree_tip:subtree_branch\n+  '\n+\n+test_done\n-- \n1.9.1\n\n"},{"id":"274095","messageId":"20151207045307.GA624@flurp.local","threadId":"40946","inReplyTo":"CAET=KiVXh2UZwRSpM_+wX_QpfjBsyfdPPUVDSDoCRVe_0wbhCg@mail.gmail.com","subject":"Re: git subtree bug produces divergent descendants","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-07T04:53:07Z","receivedAt":"2015-12-07T04:53:07Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 07, 2015 at 11:09:48AM +1300, David Ware wrote:\n> My group has run into a bug with \"git-subtree split\". Under some\n> circumstances a split created from a descendant of another earlier\n> split is not a descendant of that earlier split (thus blocking\n> pushes). [...]\n\nI'm not a git-subtree user, so this review will be superficial.\n\n> The attached patch (against v2.6.3) includes a test that reproduces\n> the problem. [...]\n\nPlease include patches inline rather than as attachments since\nreviewers will want to comment on portions of the patch as part of\ntheir response to your email. Patches as attachments make this\nprocess more painful.\n\n> From: Dave Ware <davidw@netvalue.net.nz>\n> Date: Fri, 4 Dec 2015 16:30:03 +1300\n> Subject: [PATCH] Fix bug in git-subtree split.\n\nFor the subject, mention the area you're working on, followed by a\ncolon, followed by a concise description of the problem. If possible,\ntry to say something more specific than \"fix bug\". You might, for\ninstance, say something like:\n\n    contrib/subtree: fix \"subtree split\" skipped-merge bug\n\n> A bug occurs in 'git-subtree split' where a merge is skipped even when\n> both parents act on the subtree, provided the merge results in a tree\n> identical to one of the parents. Fixed by copying the merge if at least\n\nImperative mood: s/Fixed/Fix/\n\n> one parent is non-identical, and the non-identical parent is not an\n> ancestor of the identical parent.\n> \n> Also adding a test case, this checks that a descendant can be pushed to\n> it's ancestor in this case.\n\nYour Signed-off-by: is missing. See Documentation/SubmittingPatches.\n\n> ---\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 9f06571..b837531 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -479,8 +479,16 @@ copy_or_skip()\n>  \t\t\tp=\"$p -p $parent\"\n>  \t\tfi\n>  \tdone\n> -\t\n> -\tif [ -n \"$identical\" ]; then\n> +\n> +\tcopycommit=\n> +\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n> +\t\textras=$(git rev-list --boundary $identical..$nonidentical)\n> +\t\tif [ -n \"$extras\" ]; then\n> +\t\t\t# we need to preserve history along the other branch\n> +\t\t\tcopycommit=1\n> +\t\tfi\n> +\tfi\n> +\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n\nTypically, I'd say something about how this project uses 'test'\nrather than '[' and that 'then' is placed on its own line (with no\nsemicolon), however, in this case, you're sticking to existing style\n(in this script), so I won't mention it.\n\n>  \t\techo $identical\n>  \telse\n>  \t\tcopy_commit $rev $tree \"$p\" || exit $?\n> diff --git a/contrib/subtree/t/t7901-subtree-split.sh b/contrib/subtree/t/t7901-subtree-split.sh\n> new file mode 100755\n> index 0000000..0a1ea56\n> --- /dev/null\n> +++ b/contrib/subtree/t/t7901-subtree-split.sh\n\nIs there a strong reason why this demands a new test script rather\nthan being incorporated into the existing t7900-subtree.sh?\n\n> @@ -0,0 +1,62 @@\n> +#!/bin/bash\n> +\n> +test_description='Test for bug in subtree commit filtering'\n\nA somewhat strange description. Typically, scripts want to verify\ncorrect behavior, rather than buggy behavior.\n\n> +TEST_DIRECTORY=$(pwd)/../../../t\n> +export TEST_DIRECTORY\n> +\n> +. ../../../t/test-lib.sh\n> +\n> +\n> +test_expect_success 'subtree descendent check' '\n> +  mkdir git_subtree_split_check &&\n> +  cd git_subtree_split_check &&\n\nTests don't automatically return to the directory prior to the 'cd',\nso when this test ends, the current directory will still be\n'git_subtree_split_check'. If someone later adds a test following\nthis one, that test will execute within 'git_subtree_split_check',\nwhich might not be expected by the test writer.\n\nTo ensure that the prior working directory is restored at the end of\nthe test (regardless of success or failure), tests typically employ a\nsubshell using this idiom:\n\n    mkdir foo &&\n    (\n        cd foo &&\n        ... &&\n        ...\n    )\n\nIn this case, though, I'm wondering what is the purpose of having the\n'git_subtree_split_check' subdirectory at all? Is there a reason you\ncan't just perform the test in the existing directory created\nautomatically specifically for the test script (which is already the\nscript's current working directory)? If, on the other hand, you\nincorporate this test into t7900-subtree.sh, then the separate\n'git_subtree_split_check' directory may make sense if it needs to be\nisolated from the other gunk in that script's test directory.\n\n> +  git init &&\n> +\n> +  mkdir folder &&\n> +\n> +  echo a > folder/a &&\n\nTypical style is to drop the space after the redirection operator,\nhowever, since you're following existing style in t7900-subtree.sh, I\nwon't mention it.\n\n> +  git add . &&\n> +  git commit -m \"first commit\" &&\n> +\n> +  git branch branch &&\n> +\n> +  echo 0 > folder/0 &&\n> +  git add . &&\n> +  git commit -m \"adding 0 to folder\" &&\n> +\n> +  echo b > folder/b &&\n> +  git add . &&\n> +  git commit -m \"adding b to folder\" &&\n> +  git rev-list HEAD -1 > cherry.rev &&\n\nCan this value instead just be assigned to a shell variable rather\nthan being dumped to a file?\n\n    cherryrev=$(git rev-list HEAD -1) &&\n    ... &&\n    git cherry-pick $cherryrev &&\n\n> +  git checkout branch &&\n> +  echo text > textBranch.txt &&\n> +  git add . &&\n> +  git commit -m \"commit to fiddle with branch: branch\" &&\n> +\n> +  git cherry-pick $(cat cherry.rev) &&\n\nSee above: git cherry-pick $cherryrev &&\n\n> +  git checkout master &&\n> +  git merge -m \"merge\" branch &&\n> +\n> +  git branch noop_branch &&\n> +\n> +  echo d > folder/d &&\n> +  git add . &&\n> +  git commit -m \"adding d to folder\" &&\n> +\n> +  git checkout noop_branch &&\n> +  echo moreText > anotherText.txt &&\n> +  git add . &&\n> +  git commit -m \"irrelevant\" &&\n> +\n> +  git checkout master &&\n> +  git merge -m \"second merge\" noop_branch &&\n> +\n> +  git subtree split --prefix folder/ --branch subtree_tip master &&\n> +  git subtree split --prefix folder/ --branch subtree_branch branch &&\n> +  git push . subtree_tip:subtree_branch\n> +  '\n> +\n> +test_done\n> -- \n> 1.9.1\n"},{"id":"274138","messageId":"1449521452-19043-1-git-send-email-davidw@realtimegenomics.com","threadId":"40946","inReplyTo":"20151207045307.GA624@flurp.local","subject":"[PATCH] contrib/subtree: fix \"subtree split\" skipped-merge bug.","fromName":"Dave Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2015-12-07T20:50:52Z","receivedAt":"2015-12-07T20:50:52Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"A bug occurs in 'git-subtree split' where a merge is skipped even when\nboth parents act on the subtree, provided the merge results in a tree\nidentical to one of the parents. Fix by copying the merge if at least\none parent is non-identical, and the non-identical parent is not an\nancestor of the identical parent.\n\nAlso adding a test case, this checks that a descendant can be pushed to\nit's ancestor in this case.\n\nSigned-off-by: Dave Ware <davidw@realtimegenomics.com>\n---\n contrib/subtree/git-subtree.sh     | 12 +++++++--\n contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 9f06571..b837531 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -479,8 +479,16 @@ copy_or_skip()\n \t\t\tp=\"$p -p $parent\"\n \t\tfi\n \tdone\n-\t\n-\tif [ -n \"$identical\" ]; then\n+\n+\tcopycommit=\n+\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n+\t\textras=$(git rev-list --boundary $identical..$nonidentical)\n+\t\tif [ -n \"$extras\" ]; then\n+\t\t\t# we need to preserve history along the other branch\n+\t\t\tcopycommit=1\n+\t\tfi\n+\tfi\n+\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n \t\techo $identical\n \telse\n \t\tcopy_commit $rev $tree \"$p\" || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 9051982..ea991eb 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n \t))\n '\n \n+test_expect_success 'subtree descendent check' '\n+  mkdir git_subtree_split_check &&\n+  (\n+    cd git_subtree_split_check &&\n+    git init &&\n+\n+    mkdir folder &&\n+\n+    echo a >folder/a &&\n+    git add . &&\n+    git commit -m \"first commit\" &&\n+\n+    git branch branch &&\n+\n+    echo 0 >folder/0 &&\n+    git add . &&\n+    git commit -m \"adding 0 to folder\" &&\n+\n+    echo b >folder/b &&\n+    git add . &&\n+    git commit -m \"adding b to folder\" &&\n+    cherry=$(git rev-list HEAD -1) &&\n+\n+    git checkout branch &&\n+    echo text >textBranch.txt &&\n+    git add . &&\n+    git commit -m \"commit to fiddle with branch: branch\" &&\n+\n+    git cherry-pick $cherry &&\n+    git checkout master &&\n+    git merge -m \"merge\" branch &&\n+\n+    git branch noop_branch &&\n+\n+    echo d >folder/d &&\n+    git add . &&\n+    git commit -m \"adding d to folder\" &&\n+\n+    git checkout noop_branch &&\n+    echo moreText >anotherText.txt &&\n+    git add . &&\n+    git commit -m \"irrelevant\" &&\n+\n+    git checkout master &&\n+    git merge -m \"second merge\" noop_branch &&\n+\n+    git subtree split --prefix folder/ --branch subtree_tip master &&\n+    git subtree split --prefix folder/ --branch subtree_branch branch &&\n+    git push . subtree_tip:subtree_branch\n+  )\n+  '\n+\n test_done\n-- \n1.9.1\n"},{"id":"274140","messageId":"CAET=KiXHoasXv6_y=SLZEAd=CCzj0T3zW6Wb7rF42=8v2LxZ+w@mail.gmail.com","threadId":"40946","inReplyTo":"20151207045307.GA624@flurp.local","subject":"Re: git subtree bug produces divergent descendants","fromName":"David Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2015-12-07T21:01:32Z","receivedAt":"2015-12-07T21:01:32Z","isPatch":false,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"Thanks for taking the time to look over it. I'm not familiar with the\nprocess here.\n\nOn Mon, Dec 7, 2015 at 5:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Tests don't automatically return to the directory prior to the 'cd',\n> so when this test ends, the current directory will still be\n> 'git_subtree_split_check'. If someone later adds a test following\n> this one, that test will execute within 'git_subtree_split_check',\n> which might not be expected by the test writer.\n>\n> To ensure that the prior working directory is restored at the end of\n> the test (regardless of success or failure), tests typically employ a\n> subshell using this idiom:\n>\n>     mkdir foo &&\n>     (\n>         cd foo &&\n>         ... &&\n>         ...\n>     )\n>\n\nI'm not at all familiar with this test harness so I had a few problems\nhere (like this, and the bash variable). Thank you for the advice.\n\nCheers,\nDave Ware\n"},{"id":"274163","messageId":"CAPig+cR36772YDc5RQRwXP3+ucVWumim9HYTXVMuGXN2cnQ7Ow@mail.gmail.com","threadId":"40946","inReplyTo":"1449521452-19043-1-git-send-email-davidw@realtimegenomics.com","subject":"Re: [PATCH] contrib/subtree: fix \"subtree split\" skipped-merge bug.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-08T06:49:02Z","receivedAt":"2015-12-08T06:49:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 7, 2015 at 3:50 PM, Dave Ware <davidw@realtimegenomics.com> wrote:\n> [PATCH] contrib/subtree: fix \"subtree split\" skipped-merge bug.\n\nAs an aid for reviewers, please indicate the version of this patch\nsubmission. For instance, this is the second attempt, so the subject\nwould be decorated as [PATCH v2], and the next one (if submitted) will\nbe v3. The -v option of git-format-patch can help automate this.\n\nStyle: drop the full-stop (period) from the subject line\n\n> A bug occurs in 'git-subtree split' where a merge is skipped even when\n> both parents act on the subtree, provided the merge results in a tree\n> identical to one of the parents. Fix by copying the merge if at least\n> one parent is non-identical, and the non-identical parent is not an\n> ancestor of the identical parent.\n>\n> Also adding a test case, this checks that a descendant can be pushed to\n\ns/Also adding/Also, add/\ns/, this/which/\n\n> it's ancestor in this case.\n\ns/it's/its/\n\n> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n> ---\n\nRight here below the \"---\" line is a good place to describe what\nchanged since the previous version. For instance, in v2, you made\nminor improvements to the commit message, added your sign-off, folded\nthe new test into the existing t7900-subtree.sh, added a subshell\naround 'cd', and assigned the output of git-rev-list to a shell\nvariable rather than dumping it to a file.\n\nIncluding a link to the previous version, like this[1], is also\nreviewer-friendly.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/282065\n\nAs before, I'm not a git-subtree user, so this review is superficial.\nMore below...\n\n>  contrib/subtree/git-subtree.sh     | 12 +++++++--\n>  contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 62 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index 9051982..ea991eb 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n>         ))\n>  '\n>\n> +test_expect_success 'subtree descendent check' '\n> +  mkdir git_subtree_split_check &&\n> +  (\n> +    cd git_subtree_split_check &&\n\nStyle: indent with tabs rather than spaces\n\n> +    git init &&\n> +\n> +    mkdir folder &&\n> +\n> +    echo a >folder/a &&\n> +    git add . &&\n> +    git commit -m \"first commit\" &&\n> +\n> +    git branch branch &&\n> +\n> +    echo 0 >folder/0 &&\n> +    git add . &&\n> +    git commit -m \"adding 0 to folder\" &&\n> +\n> +    echo b >folder/b &&\n> +    git add . &&\n> +    git commit -m \"adding b to folder\" &&\n> +    cherry=$(git rev-list HEAD -1) &&\n\ngit-rev-parse would probably be more idiomatic:\n\n    cherry=$(git rev-parse HEAD)\n\n> +    git checkout branch &&\n> +    echo text >textBranch.txt &&\n> +    git add . &&\n> +    git commit -m \"commit to fiddle with branch: branch\" &&\n> +\n> +    git cherry-pick $cherry &&\n> +    git checkout master &&\n> +    git merge -m \"merge\" branch &&\n> +\n> +    git branch noop_branch &&\n> +\n> +    echo d >folder/d &&\n> +    git add . &&\n> +    git commit -m \"adding d to folder\" &&\n> +\n> +    git checkout noop_branch &&\n> +    echo moreText >anotherText.txt &&\n> +    git add . &&\n> +    git commit -m \"irrelevant\" &&\n> +\n> +    git checkout master &&\n> +    git merge -m \"second merge\" noop_branch &&\n> +\n> +    git subtree split --prefix folder/ --branch subtree_tip master &&\n> +    git subtree split --prefix folder/ --branch subtree_branch branch &&\n> +    git push . subtree_tip:subtree_branch\n> +  )\n> +  '\n> +\n>  test_done\n> --\n> 1.9.1\n"},{"id":"274193","messageId":"1449607160-20608-1-git-send-email-davidw@realtimegenomics.com","threadId":"40946","inReplyTo":"CAPig+cR36772YDc5RQRwXP3+ucVWumim9HYTXVMuGXN2cnQ7Ow@mail.gmail.com","subject":"[PATCH v3] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Dave Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2015-12-08T20:39:20Z","receivedAt":"2015-12-08T20:39:20Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"A bug occurs in 'git-subtree split' where a merge is skipped even when\nboth parents act on the subtree, provided the merge results in a tree\nidentical to one of the parents. Fix by copying the merge if at least\none parent is non-identical, and the non-identical parent is not an\nancestor of the identical parent.\n\nAlso, add a test case which checks that a descendant can be pushed to\nits ancestor in this case.\n\nSigned-off-by: Dave Ware <davidw@realtimegenomics.com>\n---\n\nNotes:\n    Many thanks to Eric Sunshine for his adivce on this patch\n    Changes since v2:\n    - Minor improvements to commit message\n    - Changed space indentation to tab indentation in test case\n    - Changed use of rev-list for obtaining commit id to use rev-parse instead\n    Changes since v1:\n    - Minor improvements to commit message\n    - Added sign off\n    - Moved test case from own file into t7900-subtree.sh\n    - Added subshell to test around 'cd'\n    - Moved record of commit for cherry-pick to variable instead of dumping into file\n    \n    [v2]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282121\n    [v1]: http://thread.gmane.org/gmane.comp.version-control.git/282065\n\n contrib/subtree/git-subtree.sh     | 12 +++++++--\n contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 9f06571..b837531 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -479,8 +479,16 @@ copy_or_skip()\n \t\t\tp=\"$p -p $parent\"\n \t\tfi\n \tdone\n-\t\n-\tif [ -n \"$identical\" ]; then\n+\n+\tcopycommit=\n+\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n+\t\textras=$(git rev-list --boundary $identical..$nonidentical)\n+\t\tif [ -n \"$extras\" ]; then\n+\t\t\t# we need to preserve history along the other branch\n+\t\t\tcopycommit=1\n+\t\tfi\n+\tfi\n+\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n \t\techo $identical\n \telse\n \t\tcopy_commit $rev $tree \"$p\" || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 9051982..710278c 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n \t))\n '\n \n+test_expect_success 'subtree descendent check' '\n+\tmkdir git_subtree_split_check &&\n+\t(\n+\t\tcd git_subtree_split_check &&\n+\t\tgit init &&\n+\n+\t\tmkdir folder &&\n+\n+\t\techo a >folder/a &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"first commit\" &&\n+\n+\t\tgit branch branch &&\n+\n+\t\techo 0 >folder/0 &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding 0 to folder\" &&\n+\n+\t\techo b >folder/b &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding b to folder\" &&\n+\t\tcherry=$(git rev-parse HEAD) &&\n+\n+\t\tgit checkout branch &&\n+\t\techo text >textBranch.txt &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"commit to fiddle with branch: branch\" &&\n+\n+\t\tgit cherry-pick $cherry &&\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"merge\" branch &&\n+\n+\t\tgit branch noop_branch &&\n+\n+\t\techo d >folder/d &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding d to folder\" &&\n+\n+\t\tgit checkout noop_branch &&\n+\t\techo moreText >anotherText.txt &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"irrelevant\" &&\n+\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"second merge\" noop_branch &&\n+\n+\t\tgit subtree split --prefix folder/ --branch subtree_tip master &&\n+\t\tgit subtree split --prefix folder/ --branch subtree_branch branch &&\n+\t\tgit push . subtree_tip:subtree_branch\n+\t)\n+\t'\n+\n test_done\n-- \n1.9.1\n"},{"id":"274194","messageId":"xmqqk2ook52u.fsf@gitster.mtv.corp.google.com","threadId":"40946","inReplyTo":"1449607160-20608-1-git-send-email-davidw@realtimegenomics.com","subject":"Re: [PATCH v3] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-08T21:23:21Z","receivedAt":"2015-12-08T21:23:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Ware <davidw@realtimegenomics.com> writes:\n\n> A bug occurs in 'git-subtree split' where a merge is skipped even when\n> both parents act on the subtree, provided the merge results in a tree\n> identical to one of the parents. Fix by copying the merge if at least\n> one parent is non-identical, and the non-identical parent is not an\n> ancestor of the identical parent.\n>\n> Also, add a test case which checks that a descendant can be pushed to\n> its ancestor in this case.\n>\n> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n> ---\n\nThe first sentence may be made clearer if you rephrased the early\npart of the sentence this way:\n\n\t'git subtree split' can incorrectly skip a merge even when\n        both parents ...\n\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 9f06571..b837531 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -479,8 +479,16 @@ copy_or_skip()\n>  \t\t\tp=\"$p -p $parent\"\n>  \t\tfi\n>  \tdone\n> -\t\n> -\tif [ -n \"$identical\" ]; then\n> +\n> +\tcopycommit=\n> +\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n> +\t\textras=$(git rev-list --boundary $identical..$nonidentical)\n> +\t\tif [ -n \"$extras\" ]; then\n> +\t\t\t# we need to preserve history along the other branch\n> +\t\t\tcopycommit=1\n> +\t\tfi\n\nWhat is the significance of \"--boundary\" here?  I think for the\npurpose of \"is the identical one part of the nonidentical one?\" you\ndo not need it, but there may be something subtle I missed.  I am\nasking this because use of \"rev-list --boundary\" in scripts is\nalmost always a bug.\n\nAlso, depending on how huge the output from the rev-list could be,\nyou might want to use \"rev-list --count $i..$n\" and compare it with\n0 instead--that way, you would not have to be worried about having\nto carry around a huge string that you would otherwise not use, only\nto see if that string is empty.\n\nThanks.\n\n> +\tfi\n> +\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n>  \t\techo $identical\n>  \telse\n>  \t\tcopy_commit $rev $tree \"$p\" || exit $?\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index 9051982..710278c 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n>  \t))\n>  '\n>  \n> +test_expect_success 'subtree descendent check' '\n> +\tmkdir git_subtree_split_check &&\n> +\t(\n> +\t\tcd git_subtree_split_check &&\n> +\t\tgit init &&\n> +\n> +\t\tmkdir folder &&\n> +\n> +\t\techo a >folder/a &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"first commit\" &&\n> +\n> +\t\tgit branch branch &&\n> +\n> +\t\techo 0 >folder/0 &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"adding 0 to folder\" &&\n> +\n> +\t\techo b >folder/b &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"adding b to folder\" &&\n> +\t\tcherry=$(git rev-parse HEAD) &&\n> +\n> +\t\tgit checkout branch &&\n> +\t\techo text >textBranch.txt &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"commit to fiddle with branch: branch\" &&\n> +\n> +\t\tgit cherry-pick $cherry &&\n> +\t\tgit checkout master &&\n> +\t\tgit merge -m \"merge\" branch &&\n> +\n> +\t\tgit branch noop_branch &&\n> +\n> +\t\techo d >folder/d &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"adding d to folder\" &&\n> +\n> +\t\tgit checkout noop_branch &&\n> +\t\techo moreText >anotherText.txt &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"irrelevant\" &&\n> +\n> +\t\tgit checkout master &&\n> +\t\tgit merge -m \"second merge\" noop_branch &&\n> +\n> +\t\tgit subtree split --prefix folder/ --branch subtree_tip master &&\n> +\t\tgit subtree split --prefix folder/ --branch subtree_branch branch &&\n> +\t\tgit push . subtree_tip:subtree_branch\n> +\t)\n> +\t'\n> +\n>  test_done\n"},{"id":"274198","messageId":"CAET=KiWRazHNTT5dJakUFGmRKMnFhv3Lxkm2WXa6bue=BrfU+A@mail.gmail.com","threadId":"40946","inReplyTo":"xmqqk2ook52u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"David Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2015-12-09T00:16:52Z","receivedAt":"2015-12-09T00:16:52Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"On Wed, Dec 9, 2015 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Ware <davidw@realtimegenomics.com> writes:\n>\n>> A bug occurs in 'git-subtree split' where a merge is skipped even when\n>> both parents act on the subtree, provided the merge results in a tree\n>> identical to one of the parents. Fix by copying the merge if at least\n>> one parent is non-identical, and the non-identical parent is not an\n>> ancestor of the identical parent.\n>>\n>> Also, add a test case which checks that a descendant can be pushed to\n>> its ancestor in this case.\n>>\n>> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n>> ---\n>\n> The first sentence may be made clearer if you rephrased the early\n> part of the sentence this way:\n>\n>         'git subtree split' can incorrectly skip a merge even when\n>         both parents ...\n>\n\nNoted.\n\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>> index 9f06571..b837531 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -479,8 +479,16 @@ copy_or_skip()\n>>                       p=\"$p -p $parent\"\n>>               fi\n>>       done\n>> -\n>> -     if [ -n \"$identical\" ]; then\n>> +\n>> +     copycommit=\n>> +     if [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n>> +             extras=$(git rev-list --boundary $identical..$nonidentical)\n>> +             if [ -n \"$extras\" ]; then\n>> +                     # we need to preserve history along the other branch\n>> +                     copycommit=1\n>> +             fi\n>\n> What is the significance of \"--boundary\" here?  I think for the\n> purpose of \"is the identical one part of the nonidentical one?\" you\n> do not need it, but there may be something subtle I missed.  I am\n> asking this because use of \"rev-list --boundary\" in scripts is\n> almost always a bug.\n>\n\nThe other way around actually I'm trying to determine if nonidentical\ncontains any commits\n not in identical.  I'll confess I don't actually know specifically\nwhat the --boundary option\ndoes, this probably came from a stack overflow example while we were\nlooking up how to\nbest do the check. Further experimentation with the option suggests\nthat it does not do what\nI want, so I will remove it. Thank you.\n\n> Also, depending on how huge the output from the rev-list could be,\n> you might want to use \"rev-list --count $i..$n\" and compare it with\n> 0 instead--that way, you would not have to be worried about having\n> to carry around a huge string that you would otherwise not use, only\n> to see if that string is empty.\n\nThanks, I didn't know about that option.\n"},{"id":"274199","messageId":"1449620377-30479-1-git-send-email-davidw@realtimegenomics.com","threadId":"40946","inReplyTo":"xmqqk2ook52u.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v4] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Dave Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2015-12-09T00:19:37Z","receivedAt":"2015-12-09T00:19:37Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"'git subtree split' can incorrectly skip a merge even when both parents\nact on the subtree, provided the merge results in a tree identical to\none of the parents. Fix by copying the merge if at least one parent is\nnon-identical, and the non-identical parent is not an ancestor of the\nidentical parent.\n\nAlso, add a test case which checks that a descendant can be pushed to\nits ancestor in this case.\n\nSigned-off-by: Dave Ware <davidw@realtimegenomics.com>\n---\n\nNotes:\n    Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch\n    \n    Changes since v3:\n    - Improvements to commit message\n    - Removed incorrect use of --boundary on rev-list\n    - Changed use of rev-list to use --count\n    Changes since v2:\n    - Minor improvements to commit message\n    - Changed space indentation to tab indentation in test case\n    - Changed use of rev-list for obtaining commit id to use rev-parse instead\n    Changes since v1:\n    - Minor improvements to commit message\n    - Added sign off\n    - Moved test case from own file into t7900-subtree.sh\n    - Added subshell to test around 'cd'\n    - Moved record of commit for cherry-pick to variable instead of dumping into file\n    \n    [v3]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282176\n    [v2]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282121\n    [v1]: http://thread.gmane.org/gmane.comp.version-control.git/282065\n\n contrib/subtree/git-subtree.sh     | 12 +++++++--\n contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 9f06571..ebf99d9 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -479,8 +479,16 @@ copy_or_skip()\n \t\t\tp=\"$p -p $parent\"\n \t\tfi\n \tdone\n-\t\n-\tif [ -n \"$identical\" ]; then\n+\n+\tcopycommit=\n+\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n+\t\textras=$(git rev-list --count $identical..$nonidentical)\n+\t\tif [ \"$extras\" -ne 0 ]; then\n+\t\t\t# we need to preserve history along the other branch\n+\t\t\tcopycommit=1\n+\t\tfi\n+\tfi\n+\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n \t\techo $identical\n \telse\n \t\tcopy_commit $rev $tree \"$p\" || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 9051982..710278c 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n \t))\n '\n \n+test_expect_success 'subtree descendent check' '\n+\tmkdir git_subtree_split_check &&\n+\t(\n+\t\tcd git_subtree_split_check &&\n+\t\tgit init &&\n+\n+\t\tmkdir folder &&\n+\n+\t\techo a >folder/a &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"first commit\" &&\n+\n+\t\tgit branch branch &&\n+\n+\t\techo 0 >folder/0 &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding 0 to folder\" &&\n+\n+\t\techo b >folder/b &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding b to folder\" &&\n+\t\tcherry=$(git rev-parse HEAD) &&\n+\n+\t\tgit checkout branch &&\n+\t\techo text >textBranch.txt &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"commit to fiddle with branch: branch\" &&\n+\n+\t\tgit cherry-pick $cherry &&\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"merge\" branch &&\n+\n+\t\tgit branch noop_branch &&\n+\n+\t\techo d >folder/d &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding d to folder\" &&\n+\n+\t\tgit checkout noop_branch &&\n+\t\techo moreText >anotherText.txt &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"irrelevant\" &&\n+\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"second merge\" noop_branch &&\n+\n+\t\tgit subtree split --prefix folder/ --branch subtree_tip master &&\n+\t\tgit subtree split --prefix folder/ --branch subtree_branch branch &&\n+\t\tgit push . subtree_tip:subtree_branch\n+\t)\n+\t'\n+\n test_done\n-- \n1.9.1\n"},{"id":"274204","messageId":"CAPig+cSfkz=SNOn+8yP-QN8gJ0ej1wo3HW+y3NO+QvUCOP=+8A@mail.gmail.com","threadId":"40946","inReplyTo":"1449620377-30479-1-git-send-email-davidw@realtimegenomics.com","subject":"Re: [PATCH v4] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-09T07:52:01Z","receivedAt":"2015-12-09T07:52:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 8, 2015 at 7:19 PM, Dave Ware <davidw@realtimegenomics.com> wrote:\n> 'git subtree split' can incorrectly skip a merge even when both parents\n> act on the subtree, provided the merge results in a tree identical to\n> one of the parents. Fix by copying the merge if at least one parent is\n> non-identical, and the non-identical parent is not an ancestor of the\n> identical parent.\n>\n> Also, add a test case which checks that a descendant can be pushed to\n> its ancestor in this case.\n>\n> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n> ---\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n>         ))\n>  '\n>\n> +test_expect_success 'subtree descendent check' '\n\ns/descendent/descendant/\n\n> +       mkdir git_subtree_split_check &&\n> +       (\n> +               cd git_subtree_split_check &&\n> +[...]\n> +               git push . subtree_tip:subtree_branch\n> +       )\n> +       '\n\nStyle nit: don't indent closing quotation mark\n\n>  test_done\n> --\n> 1.9.1\n"},{"id":"274212","messageId":"1449695853-24929-1-git-send-email-davidw@realtimegenomics.com","threadId":"40946","inReplyTo":"CAPig+cSfkz=SNOn+8yP-QN8gJ0ej1wo3HW+y3NO+QvUCOP=+8A@mail.gmail.com","subject":"[PATCH v5] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Dave Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2015-12-09T21:17:33Z","receivedAt":"2015-12-09T21:17:33Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"'git subtree split' can incorrectly skip a merge even when both parents\nact on the subtree, provided the merge results in a tree identical to\none of the parents. Fix by copying the merge if at least one parent is\nnon-identical, and the non-identical parent is not an ancestor of the\nidentical parent.\n\nAlso, add a test case which checks that a descendant can be pushed to\nits ancestor in this case.\n\nSigned-off-by: Dave Ware <davidw@realtimegenomics.com>\n---\n\nNotes:\n    Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch\n    \n    Changes since v4\n    - Minor spelling and style fixes to test case\n    Changes since v3:\n    - Improvements to commit message\n    - Removed incorrect use of --boundary on rev-list\n    - Changed use of rev-list to use --count\n    Changes since v2:\n    - Minor improvements to commit message\n    - Changed space indentation to tab indentation in test case\n    - Changed use of rev-list for obtaining commit id to use rev-parse instead\n    Changes since v1:\n    - Minor improvements to commit message\n    - Added sign off\n    - Moved test case from own file into t7900-subtree.sh\n    - Added subshell to test around 'cd'\n    - Moved record of commit for cherry-pick to variable instead of dumping into file\n    \n    [v4]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282182\n    [v3]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282176\n    [v2]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282121\n    [v1]: http://thread.gmane.org/gmane.comp.version-control.git/282065\n\n contrib/subtree/git-subtree.sh     | 12 +++++++--\n contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 9f06571..ebf99d9 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -479,8 +479,16 @@ copy_or_skip()\n \t\t\tp=\"$p -p $parent\"\n \t\tfi\n \tdone\n-\t\n-\tif [ -n \"$identical\" ]; then\n+\n+\tcopycommit=\n+\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n+\t\textras=$(git rev-list --count $identical..$nonidentical)\n+\t\tif [ \"$extras\" -ne 0 ]; then\n+\t\t\t# we need to preserve history along the other branch\n+\t\t\tcopycommit=1\n+\t\tfi\n+\tfi\n+\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n \t\techo $identical\n \telse\n \t\tcopy_commit $rev $tree \"$p\" || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 9051982..4fe4820 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n \t))\n '\n \n+test_expect_success 'subtree descendant check' '\n+\tmkdir git_subtree_split_check &&\n+\t(\n+\t\tcd git_subtree_split_check &&\n+\t\tgit init &&\n+\n+\t\tmkdir folder &&\n+\n+\t\techo a >folder/a &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"first commit\" &&\n+\n+\t\tgit branch branch &&\n+\n+\t\techo 0 >folder/0 &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding 0 to folder\" &&\n+\n+\t\techo b >folder/b &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding b to folder\" &&\n+\t\tcherry=$(git rev-parse HEAD) &&\n+\n+\t\tgit checkout branch &&\n+\t\techo text >textBranch.txt &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"commit to fiddle with branch: branch\" &&\n+\n+\t\tgit cherry-pick $cherry &&\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"merge\" branch &&\n+\n+\t\tgit branch noop_branch &&\n+\n+\t\techo d >folder/d &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"adding d to folder\" &&\n+\n+\t\tgit checkout noop_branch &&\n+\t\techo moreText >anotherText.txt &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"irrelevant\" &&\n+\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"second merge\" noop_branch &&\n+\n+\t\tgit subtree split --prefix folder/ --branch subtree_tip master &&\n+\t\tgit subtree split --prefix folder/ --branch subtree_branch branch &&\n+\t\tgit push . subtree_tip:subtree_branch\n+\t)\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"275863","messageId":"87y4bunopj.fsf@waller.obbligato.org","threadId":"40946","inReplyTo":"1449695853-24929-1-git-send-email-davidw@realtimegenomics.com","subject":"Re: [PATCH v5] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-13T03:27:36Z","receivedAt":"2016-01-13T03:27:36Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Dave Ware <davidw@realtimegenomics.com> writes:\n\n[ I am sorry I took so long to respond.  This one slipped by me.  Thank\n  you for tracking this problem down and fixing it!  ]\n\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 9f06571..ebf99d9 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -479,8 +479,16 @@ copy_or_skip()\n>  \t\t\tp=\"$p -p $parent\"\n>  \t\tfi\n>  \tdone\n> -\t\n> -\tif [ -n \"$identical\" ]; then\n> +\n> +\tcopycommit=\n> +\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n> +\t\textras=$(git rev-list --count $identical..$nonidentical)\n> +\t\tif [ \"$extras\" -ne 0 ]; then\n> +\t\t\t# we need to preserve history along the other branch\n> +\t\t\tcopycommit=1\n> +\t\tfi\n> +\tfi\n> +\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n>  \t\techo $identical\n>  \telse\n>  \t\tcopy_commit $rev $tree \"$p\" || exit $?\n\nI don't see anything objectionable here.  I am just learning the split\ncode myself.  :)\n\nHowever, when I apply this against master, the test doesn't actually\npass and a gitk --all shows the merge commit still missing.  At least if\nI understand the problem correctly.  Can you verify whether it works for\nyou?\n\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index 9051982..4fe4820 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n>  \t))\n>  '\n>  \n> +test_expect_success 'subtree descendant check' '\n> +\tmkdir git_subtree_split_check &&\n> +\t(\n> +\t\tcd git_subtree_split_check &&\n> +\t\tgit init &&\n\nThis shouldn't be necessary.  If you look at the other tests in\nt7900-subtree.sh, they all start with:\n\n  next_test\n  test_expect_success '<blah>' '\n  subtree_test_create_repo \"$subtree_test_count\"\n\nThe \"subtree_test_create_repo\" takes care of creating a subdirectory and\ninitializing a repository.  Perhaps you didn't (or still don't) have the\ntest script rewrite patch that got merged a month or so ago.  If not,\nplease update to it and reformulate your test to follow the established\nconvention.  It helps *a lot* when debugging regressions.\n\n> +\t\tmkdir folder &&\n> +\n> +\t\techo a >folder/a &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"first commit\" &&\n\nYou can use \"test_create_commit\" to do these \"generate commit\"\noperations.  It's on my TODO list to update the subtree tests to use\nmore of the standard test infrastructure.  For now, just go ahead and\nuse what the other tests use.\n\n> +\t\tgit branch noop_branch &&\n[...]\n> +\t\tgit checkout noop_branch &&\n> +\t\techo moreText >anotherText.txt &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"irrelevant\" &&\n\nThis is unfortunate naming.  Why is the branch a no-op and why is the\ncommit irrelevant?  Does the test test the same thing without them?  I\nnot they should have different names.  If so, why are these needed in\nthe test?\n\n> +\t\tgit checkout master &&\n> +\t\tgit merge -m \"second merge\" noop_branch &&\n> +\n> +\t\tgit subtree split --prefix folder/ --branch subtree_tip master &&\n> +\t\tgit subtree split --prefix folder/ --branch subtree_branch branch &&\n> +\t\tgit push . subtree_tip:subtree_branch\n\nI understand the problem was discovered because of an inability to push\nand it probably makes sense to test that since that's what exposed the\nbug.  However, I wonder if there are some additional checks that should\nbe done.  What do you expect subtree_tip and subtree_branch to look like\nand how do you expect them to relate to each other?  Should\nsubtree_branch be an ancestor of subtree_tip?  If so we should\nexplicitly test that.\n\nAgain, thanks for your work on this!  I think I actually may have hit\nthis bug in my own work but I couldn't be sure I hadn't done something\nwrong.  The sequence of commands and splits is eerily similar to\nsomething I tried a while back.  I'm *very* glad you were able to track\nit down!\n\n                          -David\n"},{"id":"275966","messageId":"CAET=KiVY5g41YgCbGqDqUaDjrd-Do9jNf=1L6xbBPcUoGcM2Kg@mail.gmail.com","threadId":"40946","inReplyTo":"87y4bunopj.fsf@waller.obbligato.org","subject":"Re: [PATCH v5] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"David Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2016-01-13T19:33:53Z","receivedAt":"2016-01-13T19:33:53Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"On Wed, Jan 13, 2016 at 4:27 PM, David A. Greene <greened@obbligato.org> wrote:\n> Dave Ware <davidw@realtimegenomics.com> writes:\n>\n> [ I am sorry I took so long to respond.  This one slipped by me.  Thank\n>   you for tracking this problem down and fixing it!  ]\n>\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>> index 9f06571..ebf99d9 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -479,8 +479,16 @@ copy_or_skip()\n>>                       p=\"$p -p $parent\"\n>>               fi\n>>       done\n>> -\n>> -     if [ -n \"$identical\" ]; then\n>> +\n>> +     copycommit=\n>> +     if [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n>> +             extras=$(git rev-list --count $identical..$nonidentical)\n>> +             if [ \"$extras\" -ne 0 ]; then\n>> +                     # we need to preserve history along the other branch\n>> +                     copycommit=1\n>> +             fi\n>> +     fi\n>> +     if [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n>>               echo $identical\n>>       else\n>>               copy_commit $rev $tree \"$p\" || exit $?\n>\n> I don't see anything objectionable here.  I am just learning the split\n> code myself.  :)\n>\n> However, when I apply this against master, the test doesn't actually\n> pass and a gitk --all shows the merge commit still missing.  At least if\n> I understand the problem correctly.  Can you verify whether it works for\n> you?\n>\n\nThe commit was made against v2.6.3, when I try to apply the patch\nagainst master it fails.\n\nHowever I can verify the test passes for me when applied against\nv2.6.3, and it also passed if I merge my patched copy of v2.6.3 into\nmaster. The process I'm using to run the tests is a little strange\nthough, it seems I have to make git, then make contrib/subtree, then\ncp git-subtree to the root before running the Makefile on the tests.\nLet me know if there's a less strange process for running the subtree\ntests.\n\nThe test case actually began life as a bash script I was running\nmanually and visually inspecting. It covers the 2 cases we needed in\norder to push our release\n1) Merges where one parent is a superset of the changes of the other\nparents regarding changes to the subtree, in this case the merge\ncommit should be copied (represented by \"merge\" in test case)\n2) Merges where only one parent operate on the subtree, and the merge\ncommit should be skipped (represented by \"second merge\" in test case)\n\nI haven't done an in depth look to verify the test checks the second\ncase, since this bit was never actually broken. But in terms of what\nthe test case should be doing only the first merge should be preserved\nin the subtree\n\n>> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n>> index 9051982..4fe4820 100755\n>> --- a/contrib/subtree/t/t7900-subtree.sh\n>> +++ b/contrib/subtree/t/t7900-subtree.sh\n>> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '\n>>       ))\n>>  '\n>>\n>> +test_expect_success 'subtree descendant check' '\n>> +     mkdir git_subtree_split_check &&\n>> +     (\n>> +             cd git_subtree_split_check &&\n>> +             git init &&\n>\n> This shouldn't be necessary.  If you look at the other tests in\n> t7900-subtree.sh, they all start with:\n>\n>   next_test\n>   test_expect_success '<blah>' '\n>   subtree_test_create_repo \"$subtree_test_count\"\n>\n> The \"subtree_test_create_repo\" takes care of creating a subdirectory and\n> initializing a repository.  Perhaps you didn't (or still don't) have the\n> test script rewrite patch that got merged a month or so ago.  If not,\n> please update to it and reformulate your test to follow the established\n> convention.  It helps *a lot* when debugging regressions.\n>\n\nI'm not a regular contributor to git (this is my first). So I'm not\nvery familiar with the test harness.\nAlso as noted I created the patch against v2.6.3, which did not have\nthe changes you mentioned.\n\n>> +             mkdir folder &&\n>> +\n>> +             echo a >folder/a &&\n>> +             git add . &&\n>> +             git commit -m \"first commit\" &&\n>\n> You can use \"test_create_commit\" to do these \"generate commit\"\n> operations.  It's on my TODO list to update the subtree tests to use\n> more of the standard test infrastructure.  For now, just go ahead and\n> use what the other tests use.\n>\n>> +             git branch noop_branch &&\n> [...]\n>> +             git checkout noop_branch &&\n>> +             echo moreText >anotherText.txt &&\n>> +             git add . &&\n>> +             git commit -m \"irrelevant\" &&\n>\n> This is unfortunate naming.  Why is the branch a no-op and why is the\n> commit irrelevant?  Does the test test the same thing without them?  I\n> not they should have different names.  If so, why are these needed in\n> the test?\n>\n\nThis is to create a merge that operates workflow (2) mentioned above,\ni.e. a branch that has absolutely no effect on the subtree and as such\nshould be skipped\n\n>> +             git checkout master &&\n>> +             git merge -m \"second merge\" noop_branch &&\n>> +\n>> +             git subtree split --prefix folder/ --branch subtree_tip master &&\n>> +             git subtree split --prefix folder/ --branch subtree_branch branch &&\n>> +             git push . subtree_tip:subtree_branch\n>\n> I understand the problem was discovered because of an inability to push\n> and it probably makes sense to test that since that's what exposed the\n> bug.  However, I wonder if there are some additional checks that should\n> be done.  What do you expect subtree_tip and subtree_branch to look like\n> and how do you expect them to relate to each other?  Should\n> subtree_branch be an ancestor of subtree_tip?  If so we should\n> explicitly test that.\n>\n\nit should look like this:\n\nR--A1--A2-----M---H\n  \\               /\n   B-------------\n\nWhere H is subtree_tip and B is subtree_branch. So yes subtree_tip is\na descendant of subtree_branch\n\nAgreed, it should probably be checking things more explicitly. And\nideally should also be checking that commit \"irrelevant\" and \"second\nmerge\" are being skipped if possible.\n\n> Again, thanks for your work on this!  I think I actually may have hit\n> this bug in my own work but I couldn't be sure I hadn't done something\n> wrong.  The sequence of commands and splits is eerily similar to\n> something I tried a while back.  I'm *very* glad you were able to track\n> it down!\n>\n>                           -David\n\nAs I noted in my original email this patch is solely designed to fix\nthe issue we ran into whilst trying to make our release (essentially\n(1) and (2) mentioned above) and other cases of this same issue are\nnot addressed.\ni.e.\n- The many parent case. I've made no attempt to handle this situation\nproperly in the presence of greater than 2 parents. In theory it will\nnow sometimes correctly copy the merge where it wouldn't before, and\nsometimes use the old behaviour.\n- This is one I've only realised since submitting the patch: The case\nwhere both parents have an identical tree to the merge commit, they\ndon't necessarily have the same set of commits to achieve this state,\nso this should be being checked as well. Again I don't think this\npatch makes this situation worse, it will simply result in the old\nbehaviour being used.\n\nThanks for taking the time to look at this.\n\nCheers,\nDave Ware\n"},{"id":"276002","messageId":"87bn8o97mh.fsf@waller.obbligato.org","threadId":"40946","inReplyTo":"CAET=KiVY5g41YgCbGqDqUaDjrd-Do9jNf=1L6xbBPcUoGcM2Kg@mail.gmail.com","subject":"Re: [PATCH v5] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-14T03:12:38Z","receivedAt":"2016-01-14T03:12:38Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"David Ware <davidw@realtimegenomics.com> writes:\n\n>> However, when I apply this against master, the test doesn't actually\n>> pass and a gitk --all shows the merge commit still missing.  At least if\n>> I understand the problem correctly.  Can you verify whether it works for\n>> you?\n>>\n>\n> The commit was made against v2.6.3, when I try to apply the patch\n> against master it fails.\n\nAny ideas why?\n\n> However I can verify the test passes for me when applied against\n> v2.6.3, and it also passed if I merge my patched copy of v2.6.3 into\n> master.\n\nI don't think the subtree split code has changed at all in that period\nand the logs bear that out.  So there must be some change in\nv2.6.3..master that confounds your patch.\n\nRe-checking the patch submission guidelines, it looks like bugfixes\nshould be based against maint.  I did that and the test still fails with\nyour changes.  It seems like we ought to rebase to maint and continue\nour investigation there.\n\n> The process I'm using to run the tests is a little strange though, it\n> seems I have to make git, then make contrib/subtree, then cp\n> git-subtree to the root before running the Makefile on the tests.  Let\n> me know if there's a less strange process for running the subtree\n> tests.\n\nI actually have an update that makes this easier but I haven't submitted\nit yet.  But yes, you've got the current process right.\n\n> The test case actually began life as a bash script I was running\n> manually and visually inspecting. It covers the 2 cases we needed in\n> order to push our release\n>\n> 1) Merges where one parent is a superset of the changes of the other\n> parents regarding changes to the subtree, in this case the merge\n> commit should be copied (represented by \"merge\" in test case)\n>\n> 2) Merges where only one parent operate on the subtree, and the merge\n> commit should be skipped (represented by \"second merge\" in test case)\n>\n> I haven't done an in depth look to verify the test checks the second\n> case, since this bit was never actually broken. But in terms of what\n> the test case should be doing only the first merge should be preserved\n> in the subtree\n\nOk, thanks.  More on this below.\n\n>> The \"subtree_test_create_repo\" takes care of creating a subdirectory and\n>> initializing a repository.  Perhaps you didn't (or still don't) have the\n>> test script rewrite patch that got merged a month or so ago.  If not,\n>> please update to it and reformulate your test to follow the established\n>> convention.  It helps *a lot* when debugging regressions.\n>>\n>\n> I'm not a regular contributor to git (this is my first). So I'm not\n> very familiar with the test harness.\n\n:)  Welcome!  I'm just (re-)starting work on git-subtree myself so we're\non the same learning curve.  I inherited the code from the original\nauthor and we're slowly cleaning it up.  The goal is to get it out of\ncontrib and add some useful features.\n\n> Also as noted I created the patch against v2.6.3, which did not have\n> the changes you mentioned.\n\nOk.  Your patch applied cleanly to maint and maint has the latest\nversion of the test file.  It should be just a matter of following what\nthe other tests do.  I'm more than happy to guide you through it.\n\n>>> +             git branch noop_branch &&\n>> [...]\n>>> +             git checkout noop_branch &&\n>>> +             echo moreText >anotherText.txt &&\n>>> +             git add . &&\n>>> +             git commit -m \"irrelevant\" &&\n>>\n>> This is unfortunate naming.  Why is the branch a no-op and why is the\n>> commit irrelevant?  Does the test test the same thing without them?  I\n>> not they should have different names.  If so, why are these needed in\n>> the test?\n>>\n>\n> This is to create a merge that operates workflow (2) mentioned above,\n> i.e. a branch that has absolutely no effect on the subtree and as such\n> should be skipped\n\nOk.  Some comments to that effect would be great.  Something like what\nyour wrote describing (1) and (2) about would help a lot.  I'd still\nlike to see these names cleaned up because they confused me when I\nlooked at it.  Perhaps \"no_subtree_work_branch\" and \"Non-subtree\nchange?\"  Feel free to pick your own names if you think of something\nbetter.\n\n>>> +             git checkout master &&\n>>> +             git merge -m \"second merge\" noop_branch &&\n>>> +\n>>> +             git subtree split --prefix folder/ --branch subtree_tip master &&\n>>> +             git subtree split --prefix folder/ --branch subtree_branch branch &&\n>>> +             git push . subtree_tip:subtree_branch\n>>\n>> I understand the problem was discovered because of an inability to push\n>> and it probably makes sense to test that since that's what exposed the\n>> bug.  However, I wonder if there are some additional checks that should\n>> be done.  What do you expect subtree_tip and subtree_branch to look like\n>> and how do you expect them to relate to each other?  Should\n>> subtree_branch be an ancestor of subtree_tip?  If so we should\n>> explicitly test that.\n>>\n>\n> it should look like this:\n>\n> R--A1--A2-----M---H\n>   \\               /\n>    B-------------\n>\n> Where H is subtree_tip and B is subtree_branch. So yes subtree_tip is\n> a descendant of subtree_branch\n\nOk.\n\n> Agreed, it should probably be checking things more explicitly. And\n> ideally should also be checking that commit \"irrelevant\" and \"second\n> merge\" are being skipped if possible.\n\nRight.  If you want to add those tests, great.  Otherwise, please add a\ncomment describing them so that others can add them later.  I just don't\nwant to forget to test things we know about but I don't want it to hold\nup your patch.\n\n> As I noted in my original email this patch is solely designed to fix\n> the issue we ran into whilst trying to make our release (essentially\n> (1) and (2) mentioned above) and other cases of this same issue are\n> not addressed.\n> i.e.\n\n> - The many parent case. I've made no attempt to handle this situation\n> properly in the presence of greater than 2 parents. In theory it will\n> now sometimes correctly copy the merge where it wouldn't before, and\n> sometimes use the old behaviour.\n\n> - This is one I've only realised since submitting the patch: The case\n> where both parents have an identical tree to the merge commit, they\n> don't necessarily have the same set of commits to achieve this state,\n> so this should be being checked as well. Again I don't think this\n> patch makes this situation worse, it will simply result in the old\n> behaviour being used.\n\nYou certainly don't have to test and/or fix every potential problem with\nthis patch.  Noting them in the test via comments would help guide\nothers to write tests for them and/or fix the problems.  Could you add a\nblock comment before the test that describes scenarios (1) and (2),\ntalks about the status of testing them in the test (i.e. (2) isn't\ntested) and explains the potential problems listed above that are not\nbeing tested?  Thanks!\n\nAgain, thank you for your contributions!\n\n                           -David\n"},{"id":"276077","messageId":"CAET=KiWjVr5h8nfU2DfUHGvzc7Tq7LoDWym7zXPq1Nvf+xHCCg@mail.gmail.com","threadId":"40946","inReplyTo":"87bn8o97mh.fsf@waller.obbligato.org","subject":"Re: [PATCH v5] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"David Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2016-01-14T20:45:43Z","receivedAt":"2016-01-14T20:45:43Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"On Thu, Jan 14, 2016 at 4:12 PM, David A. Greene <greened@obbligato.org> wrote:\n> David Ware <davidw@realtimegenomics.com> writes:\n>> The commit was made against v2.6.3, when I try to apply the patch\n>> against master it fails.\n>\n> Any ideas why?\n\n\"git am\" (a command I have never used before) Fails like so\n\nApplying: contrib/subtree: fix \"subtree split\" skipped-merge bug\nerror: patch failed: contrib/subtree/t/t7900-subtree.sh:468\nerror: contrib/subtree/t/t7900-subtree.sh: patch does not apply\n\nIt doesn't even put any files into a conflict state.\nI guess it's because of the hefty test refactoring you mentioned.\n\n>\n>> However I can verify the test passes for me when applied against\n>> v2.6.3, and it also passed if I merge my patched copy of v2.6.3 into\n>> master.\n>\n> I don't think the subtree split code has changed at all in that period\n> and the logs bear that out.  So there must be some change in\n> v2.6.3..master that confounds your patch.\n>\n> Re-checking the patch submission guidelines, it looks like bugfixes\n> should be based against maint.  I did that and the test still fails with\n> your changes.  It seems like we ought to rebase to maint and continue\n> our investigation there.\n>\n\nHmm, the patch fails to apply for me there also. Same issue with\ncontrib/subtree/t/t7900-subtree.sh\n\nI haven't worked with mailed patches at all before, so it is possible\nI'm not using the correct workflow (I just saved the raw email I\nreceived for the patch as txt and fed it to 'git am').\nCherrypicking the commit onto maint works fine though, and the test\npasses for me in this situation.\n\n>> The process I'm using to run the tests is a little strange though, it\n>> seems I have to make git, then make contrib/subtree, then cp\n>> git-subtree to the root before running the Makefile on the tests.  Let\n>> me know if there's a less strange process for running the subtree\n>> tests.\n>\n> I actually have an update that makes this easier but I haven't submitted\n> it yet.  But yes, you've got the current process right.\n>\n\nThat will be nice.\n\n> Ok.  Your patch applied cleanly to maint and maint has the latest\n> version of the test file.  It should be just a matter of following what\n> the other tests do.  I'm more than happy to guide you through it.\n>\n>>>> +             git branch noop_branch &&\n>>> [...]\n>>>> +             git checkout noop_branch &&\n>>>> +             echo moreText >anotherText.txt &&\n>>>> +             git add . &&\n>>>> +             git commit -m \"irrelevant\" &&\n>>>\n>>> This is unfortunate naming.  Why is the branch a no-op and why is the\n>>> commit irrelevant?  Does the test test the same thing without them?  I\n>>> not they should have different names.  If so, why are these needed in\n>>> the test?\n>>>\n\nAs noted above I can't get the patch to apply cleanly to maint for me,\nbut I suppose it doesn't matter since I'm about to mail in a new\nversion created against maint.\nI've rewritten the test to use the repo/commit creation methods, and\nrenamed that branch. I've also added the comments you requested, and\nchanged the push to an ancestor check.\nI'll be submitting the new version of the patch shortly.\n\nCheers,\nDave Ware\n"},{"id":"276083","messageId":"1452806795-26621-1-git-send-email-davidw@realtimegenomics.com","threadId":"40946","inReplyTo":"87bn8o97mh.fsf@waller.obbligato.org","subject":"[PATCH v6] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Dave Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2016-01-14T21:26:35Z","receivedAt":"2016-01-14T21:26:35Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"'git subtree split' can incorrectly skip a merge even when both parents\nact on the subtree, provided the merge results in a tree identical to\none of the parents. Fix by copying the merge if at least one parent is\nnon-identical, and the non-identical parent is not an ancestor of the\nidentical parent.\n\nAlso, add a test case which checks that a descendant remains a\ndescendent on the subtree in this case.\n\nSigned-off-by: Dave Ware <davidw@realtimegenomics.com>\n---\n contrib/subtree/git-subtree.sh     | 12 ++++++--\n contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex edf36f8..5c83727 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -479,8 +479,16 @@ copy_or_skip()\n \t\t\tp=\"$p -p $parent\"\n \t\tfi\n \tdone\n-\t\n-\tif [ -n \"$identical\" ]; then\n+\n+\tcopycommit=\n+\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n+\t\textras=$(git rev-list --count $identical..$nonidentical)\n+\t\tif [ \"$extras\" -ne 0 ]; then\n+\t\t\t# we need to preserve history along the other branch\n+\t\t\tcopycommit=1\n+\t\tfi\n+\tfi\n+\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n \t\techo $identical\n \telse\n \t\tcopy_commit $rev $tree \"$p\" || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 751aee3..c5089c3 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -1014,4 +1014,64 @@ test_expect_success 'push split to subproj' '\n \t)\n '\n \n+#\n+# This test covers 2 cases in subtree split copy_or_skip code\n+# 1) Merges where one parent is a superset of the changes of the other\n+#    parent regarding changes to the subtree, in this case the merge\n+#    commit should be copied\n+# 2) Merges where only one parent operate on the subtree, and the merge\n+#    commit should be skipped\n+#\n+# (1) is checked by ensuring subtree_tip is a descendent of subtree_branch\n+# (2) should have a check added (not_a_subtree_change shouldn't be present\n+#     on the produced subtree)\n+#\n+# Other related cases which are not tested (or currently handled correctly)\n+# - Case (1) where there are more than 2 parents, it will sometimes correctly copy\n+#   the merge, and sometimes not\n+# - Merge commit where both parents have same tree as the merge, currently\n+#   will always be skipped, even if they reached that state via different\n+#   set of commits.\n+#\n+\n+next_test\n+test_expect_success 'subtree descendant check' '\n+\tsubtree_test_create_repo \"$subtree_test_count\" &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/a &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit branch branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/0 &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/b &&\n+\tcherry=$(cd \"$subtree_test_count\"; git rev-parse HEAD) &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit checkout branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" commit_on_branch &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit cherry-pick $cherry &&\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"merge should be kept on subtree\" branch &&\n+\t\tgit branch no_subtree_work_branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/d &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit checkout no_subtree_work_branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" not_a_subtree_change &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit checkout master\n+\t\tgit merge -m \"merge should be skipped on subtree\" no_subtree_work_branch\n+\n+\t\tgit subtree split --prefix folder_subtree/ --branch subtree_tip master &&\n+\t\tgit subtree split --prefix folder_subtree/ --branch subtree_branch branch\n+\t\tcheck_equal $(git rev-list --count subtree_tip..subtree_branch) 0\n+\t)\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"276113","messageId":"1452818503-21079-1-git-send-email-davidw@realtimegenomics.com","threadId":"40946","inReplyTo":"1452806795-26621-1-git-send-email-davidw@realtimegenomics.com","subject":"[PATCH v7] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Dave Ware","fromEmail":"davidw@realtimegenomics.com","sentAt":"2016-01-15T00:41:43Z","receivedAt":"2016-01-15T00:41:43Z","isPatch":true,"sender":{"key":"davidw@realtimegenomics.com","avatar":"https://avatars.githubusercontent.com/u/16342344?v=4"},"body":"'git subtree split' can incorrectly skip a merge even when both parents\nact on the subtree, provided the merge results in a tree identical to\none of the parents. Fix by copying the merge if at least one parent is\nnon-identical, and the non-identical parent is not an ancestor of the\nidentical parent.\n\nAlso, add a test case which checks that a descendant remains a\ndescendent on the subtree in this case.\n\nSigned-off-by: Dave Ware <davidw@realtimegenomics.com>\n---\n\nNotes:\n    Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch\n    Also many thanks to David A. Greene for help with subtree test style\n    \n    Changes since v6\n    - I forgot the notes when I sumbitted v6. (I have now set notes.rewriteRef,\n      so hopefully this wont happen again).\n    - Fixed some missing && in my test rewrite.\n    Changes since v5\n    - Rewrote test case to use subtree test repo and commit creation methods\n    - Added comments on what the test does and which bits are checked\n    - Added comments to test on related bugs which aren't fixed yet\n    Changes since v4\n    - Minor spelling and style fixes to test case\n    Changes since v3:\n    - Improvements to commit message\n    - Removed incorrect use of --boundary on rev-list\n    - Changed use of rev-list to use --count\n    Changes since v2:\n    - Minor improvements to commit message\n    - Changed space indentation to tab indentation in test case\n    - Changed use of rev-list for obtaining commit id to use rev-parse instead\n    Changes since v1:\n    - Minor improvements to commit message\n    - Added sign off\n    - Moved test case from own file into t7900-subtree.sh\n    - Added subshell to test around 'cd'\n    - Moved record of commit for cherry-pick to variable instead of dumping into file\n    \n    [v6]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=284095\n    [v5]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282197\n    [v4]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282182\n    [v3]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282176\n    [v2]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282121\n    [v1]: http://thread.gmane.org/gmane.comp.version-control.git/282065\n\n contrib/subtree/git-subtree.sh     | 12 ++++++--\n contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex edf36f8..5c83727 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -479,8 +479,16 @@ copy_or_skip()\n \t\t\tp=\"$p -p $parent\"\n \t\tfi\n \tdone\n-\t\n-\tif [ -n \"$identical\" ]; then\n+\n+\tcopycommit=\n+\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n+\t\textras=$(git rev-list --count $identical..$nonidentical)\n+\t\tif [ \"$extras\" -ne 0 ]; then\n+\t\t\t# we need to preserve history along the other branch\n+\t\t\tcopycommit=1\n+\t\tfi\n+\tfi\n+\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n \t\techo $identical\n \telse\n \t\tcopy_commit $rev $tree \"$p\" || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 751aee3..3bf96a9 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -1014,4 +1014,64 @@ test_expect_success 'push split to subproj' '\n \t)\n '\n \n+#\n+# This test covers 2 cases in subtree split copy_or_skip code\n+# 1) Merges where one parent is a superset of the changes of the other\n+#    parent regarding changes to the subtree, in this case the merge\n+#    commit should be copied\n+# 2) Merges where only one parent operate on the subtree, and the merge\n+#    commit should be skipped\n+#\n+# (1) is checked by ensuring subtree_tip is a descendent of subtree_branch\n+# (2) should have a check added (not_a_subtree_change shouldn't be present\n+#     on the produced subtree)\n+#\n+# Other related cases which are not tested (or currently handled correctly)\n+# - Case (1) where there are more than 2 parents, it will sometimes correctly copy\n+#   the merge, and sometimes not\n+# - Merge commit where both parents have same tree as the merge, currently\n+#   will always be skipped, even if they reached that state via different\n+#   set of commits.\n+#\n+\n+next_test\n+test_expect_success 'subtree descendant check' '\n+\tsubtree_test_create_repo \"$subtree_test_count\" &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/a &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit branch branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/0 &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/b &&\n+\tcherry=$(cd \"$subtree_test_count\"; git rev-parse HEAD) &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit checkout branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" commit_on_branch &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit cherry-pick $cherry &&\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"merge should be kept on subtree\" branch &&\n+\t\tgit branch no_subtree_work_branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" folder_subtree/d &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit checkout no_subtree_work_branch\n+\t) &&\n+\ttest_create_commit \"$subtree_test_count\" not_a_subtree_change &&\n+\t(\n+\t\tcd \"$subtree_test_count\" &&\n+\t\tgit checkout master &&\n+\t\tgit merge -m \"merge should be skipped on subtree\" no_subtree_work_branch &&\n+\n+\t\tgit subtree split --prefix folder_subtree/ --branch subtree_tip master &&\n+\t\tgit subtree split --prefix folder_subtree/ --branch subtree_branch branch &&\n+\t\tcheck_equal $(git rev-list --count subtree_tip..subtree_branch) 0\n+\t)\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"276115","messageId":"CAPig+cS3hCjkb7bc6Z8CMFBU81Mp1Nn=xUUu4ZveVzOWAM5Ybw@mail.gmail.com","threadId":"40946","inReplyTo":"1452818503-21079-1-git-send-email-davidw@realtimegenomics.com","subject":"Re: [PATCH v7] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-15T01:06:28Z","receivedAt":"2016-01-15T01:06:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jan 14, 2016 at 7:41 PM, Dave Ware <davidw@realtimegenomics.com> wrote:\n> 'git subtree split' can incorrectly skip a merge even when both parents\n> act on the subtree, provided the merge results in a tree identical to\n> one of the parents. Fix by copying the merge if at least one parent is\n> non-identical, and the non-identical parent is not an ancestor of the\n> identical parent.\n>\n> Also, add a test case which checks that a descendant remains a\n> descendent on the subtree in this case.\n>\n> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n> ---\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> @@ -1014,4 +1014,64 @@ test_expect_success 'push split to subproj' '\n> +# This test covers 2 cases in subtree split copy_or_skip code\n> +# 1) Merges where one parent is a superset of the changes of the other\n> +#    parent regarding changes to the subtree, in this case the merge\n> +#    commit should be copied\n> +# 2) Merges where only one parent operate on the subtree, and the merge\n\ns/operate/operates/\n\n> +#    commit should be skipped\n> +#\n> +next_test\n> +test_expect_success 'subtree descendant check' '\n> +       subtree_test_create_repo \"$subtree_test_count\" &&\n> +       test_create_commit \"$subtree_test_count\" folder_subtree/a &&\n> +       (\n> +               cd \"$subtree_test_count\" &&\n> +               git branch branch\n\nNot worth a re-roll (and probably not worthwhile anyhow since it would\nbe inconsistent with the rest of the script), but for these really\nsimple cases, you can use -C and avoid the subshell altogether:\n\n    git -C \"$subtree_test_count\" branch branch\n\n> +       ) &&\n> +       test_create_commit \"$subtree_test_count\" folder_subtree/0 &&\n> +       test_create_commit \"$subtree_test_count\" folder_subtree/b &&\n> +       cherry=$(cd \"$subtree_test_count\"; git rev-parse HEAD) &&\n> +       (\n> +               cd \"$subtree_test_count\" &&\n> +               git checkout branch\n> +       ) &&\n> +       test_create_commit \"$subtree_test_count\" commit_on_branch &&\n> +       (\n> +               cd \"$subtree_test_count\" &&\n> +               git cherry-pick $cherry &&\n> +               git checkout master &&\n> +               git merge -m \"merge should be kept on subtree\" branch &&\n> +               git branch no_subtree_work_branch\n> +       ) &&\n> +       test_create_commit \"$subtree_test_count\" folder_subtree/d &&\n> +       (\n> +               cd \"$subtree_test_count\" &&\n> +               git checkout no_subtree_work_branch\n> +       ) &&\n> +       test_create_commit \"$subtree_test_count\" not_a_subtree_change &&\n> +       (\n> +               cd \"$subtree_test_count\" &&\n> +               git checkout master &&\n> +               git merge -m \"merge should be skipped on subtree\" no_subtree_work_branch &&\n> +\n> +               git subtree split --prefix folder_subtree/ --branch subtree_tip master &&\n> +               git subtree split --prefix folder_subtree/ --branch subtree_branch branch &&\n> +               check_equal $(git rev-list --count subtree_tip..subtree_branch) 0\n> +       )\n> +'\n> +\n>  test_done\n"},{"id":"276187","messageId":"xmqq4meeisas.fsf@gitster.mtv.corp.google.com","threadId":"40946","inReplyTo":"1452818503-21079-1-git-send-email-davidw@realtimegenomics.com","subject":"Re: [PATCH v7] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-15T18:58:03Z","receivedAt":"2016-01-15T18:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Ware <davidw@realtimegenomics.com> writes:\n\n> 'git subtree split' can incorrectly skip a merge even when both parents\n> act on the subtree, provided the merge results in a tree identical to\n> one of the parents. Fix by copying the merge if at least one parent is\n> non-identical, and the non-identical parent is not an ancestor of the\n> identical parent.\n>\n> Also, add a test case which checks that a descendant remains a\n> descendent on the subtree in this case.\n>\n> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n> ---\n\nDavid, how does this round look?  Can we proceed with your (and Eric's)\nReviewed-by: with this version (with one grammo fix Eric pointed out)?\n\n>\n> Notes:\n>     Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch\n>     Also many thanks to David A. Greene for help with subtree test style\n>     \n>     Changes since v6\n>     - I forgot the notes when I sumbitted v6. (I have now set notes.rewriteRef,\n>       so hopefully this wont happen again).\n>     - Fixed some missing && in my test rewrite.\n>     Changes since v5\n>     - Rewrote test case to use subtree test repo and commit creation methods\n>     - Added comments on what the test does and which bits are checked\n>     - Added comments to test on related bugs which aren't fixed yet\n>     Changes since v4\n>     - Minor spelling and style fixes to test case\n>     Changes since v3:\n>     - Improvements to commit message\n>     - Removed incorrect use of --boundary on rev-list\n>     - Changed use of rev-list to use --count\n>     Changes since v2:\n>     - Minor improvements to commit message\n>     - Changed space indentation to tab indentation in test case\n>     - Changed use of rev-list for obtaining commit id to use rev-parse instead\n>     Changes since v1:\n>     - Minor improvements to commit message\n>     - Added sign off\n>     - Moved test case from own file into t7900-subtree.sh\n>     - Added subshell to test around 'cd'\n>     - Moved record of commit for cherry-pick to variable instead of dumping into file\n>     \n>     [v6]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=284095\n>     [v5]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282197\n>     [v4]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282182\n>     [v3]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282176\n>     [v2]: http://thread.gmane.org/gmane.comp.version-control.git/282065/focus=282121\n>     [v1]: http://thread.gmane.org/gmane.comp.version-control.git/282065\n>\n>  contrib/subtree/git-subtree.sh     | 12 ++++++--\n>  contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 70 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index edf36f8..5c83727 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -479,8 +479,16 @@ copy_or_skip()\n>  \t\t\tp=\"$p -p $parent\"\n>  \t\tfi\n>  \tdone\n> -\t\n> -\tif [ -n \"$identical\" ]; then\n> +\n> +\tcopycommit=\n> +\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n> +\t\textras=$(git rev-list --count $identical..$nonidentical)\n> +\t\tif [ \"$extras\" -ne 0 ]; then\n> +\t\t\t# we need to preserve history along the other branch\n> +\t\t\tcopycommit=1\n> +\t\tfi\n> +\tfi\n> +\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n>  \t\techo $identical\n>  \telse\n>  \t\tcopy_commit $rev $tree \"$p\" || exit $?\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index 751aee3..3bf96a9 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -1014,4 +1014,64 @@ test_expect_success 'push split to subproj' '\n>  \t)\n>  '\n>  \n> +#\n> +# This test covers 2 cases in subtree split copy_or_skip code\n> +# 1) Merges where one parent is a superset of the changes of the other\n> +#    parent regarding changes to the subtree, in this case the merge\n> +#    commit should be copied\n> +# 2) Merges where only one parent operate on the subtree, and the merge\n> +#    commit should be skipped\n> +#\n> +# (1) is checked by ensuring subtree_tip is a descendent of subtree_branch\n> +# (2) should have a check added (not_a_subtree_change shouldn't be present\n> +#     on the produced subtree)\n> +#\n> +# Other related cases which are not tested (or currently handled correctly)\n> +# - Case (1) where there are more than 2 parents, it will sometimes correctly copy\n> +#   the merge, and sometimes not\n> +# - Merge commit where both parents have same tree as the merge, currently\n> +#   will always be skipped, even if they reached that state via different\n> +#   set of commits.\n> +#\n> +\n> +next_test\n> +test_expect_success 'subtree descendant check' '\n> +\tsubtree_test_create_repo \"$subtree_test_count\" &&\n> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/a &&\n> +\t(\n> +\t\tcd \"$subtree_test_count\" &&\n> +\t\tgit branch branch\n> +\t) &&\n> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/0 &&\n> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/b &&\n> +\tcherry=$(cd \"$subtree_test_count\"; git rev-parse HEAD) &&\n> +\t(\n> +\t\tcd \"$subtree_test_count\" &&\n> +\t\tgit checkout branch\n> +\t) &&\n> +\ttest_create_commit \"$subtree_test_count\" commit_on_branch &&\n> +\t(\n> +\t\tcd \"$subtree_test_count\" &&\n> +\t\tgit cherry-pick $cherry &&\n> +\t\tgit checkout master &&\n> +\t\tgit merge -m \"merge should be kept on subtree\" branch &&\n> +\t\tgit branch no_subtree_work_branch\n> +\t) &&\n> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/d &&\n> +\t(\n> +\t\tcd \"$subtree_test_count\" &&\n> +\t\tgit checkout no_subtree_work_branch\n> +\t) &&\n> +\ttest_create_commit \"$subtree_test_count\" not_a_subtree_change &&\n> +\t(\n> +\t\tcd \"$subtree_test_count\" &&\n> +\t\tgit checkout master &&\n> +\t\tgit merge -m \"merge should be skipped on subtree\" no_subtree_work_branch &&\n> +\n> +\t\tgit subtree split --prefix folder_subtree/ --branch subtree_tip master &&\n> +\t\tgit subtree split --prefix folder_subtree/ --branch subtree_branch branch &&\n> +\t\tcheck_equal $(git rev-list --count subtree_tip..subtree_branch) 0\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"276218","messageId":"CAPig+cTfH2JvPpG9mnv0L0oAdXmuHDCQVk_98VnOdiOVS4_Y1Q@mail.gmail.com","threadId":"40946","inReplyTo":"xmqq4meeisas.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-15T23:24:26Z","receivedAt":"2016-01-15T23:24:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 15, 2016 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Dave Ware <davidw@realtimegenomics.com> writes:\n>> 'git subtree split' can incorrectly skip a merge even when both parents\n>> act on the subtree, provided the merge results in a tree identical to\n>> one of the parents. Fix by copying the merge if at least one parent is\n>> non-identical, and the non-identical parent is not an ancestor of the\n>> identical parent.\n>>\n>> Also, add a test case which checks that a descendant remains a\n>> descendent on the subtree in this case.\n>>\n>> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n>> ---\n>\n> David, how does this round look?  Can we proceed with your (and Eric's)\n> Reviewed-by: with this version (with one grammo fix Eric pointed out)?\n\nAs I'm not a subtree user, I'm not qualified to give a Reviewed-by:;\nmy review comments were general, not specific to subtree\nfunctionality. At best, that might qualify for a Helped-by: if my\ncomments had any value.\n"},{"id":"276254","messageId":"877fj7bzjg.fsf@waller.obbligato.org","threadId":"40946","inReplyTo":"CAET=KiWjVr5h8nfU2DfUHGvzc7Tq7LoDWym7zXPq1Nvf+xHCCg@mail.gmail.com","subject":"Re: [PATCH v5] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-17T22:40:19Z","receivedAt":"2016-01-17T22:40:19Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"David Ware <davidw@realtimegenomics.com> writes:\n\n> On Thu, Jan 14, 2016 at 4:12 PM, David A. Greene <greened@obbligato.org> wrote:\n>> David Ware <davidw@realtimegenomics.com> writes:\n>>> The commit was made against v2.6.3, when I try to apply the patch\n>>> against master it fails.\n>>\n>> Any ideas why?\n>\n> \"git am\" (a command I have never used before) Fails like so\n>\n> Applying: contrib/subtree: fix \"subtree split\" skipped-merge bug\n> error: patch failed: contrib/subtree/t/t7900-subtree.sh:468\n> error: contrib/subtree/t/t7900-subtree.sh: patch does not apply\n\nOh I'm sorry, I misunderstood.  I thought you meant that the patch\napplied but the test failed.\n\n> It doesn't even put any files into a conflict state.\n> I guess it's because of the hefty test refactoring you mentioned.\n\nYou should be able to just paste your new test right to the end of the\nupdated test file.  The tests were refactored to make each test\nindependent of the other.  There's no functionality change at all.\n\n>> Re-checking the patch submission guidelines, it looks like bugfixes\n>> should be based against maint.  I did that and the test still fails with\n>> your changes.  It seems like we ought to rebase to maint and continue\n>> our investigation there.\n>>\n>\n> Hmm, the patch fails to apply for me there also. Same issue with\n> contrib/subtree/t/t7900-subtree.sh\n>\n> I haven't worked with mailed patches at all before, so it is possible\n> I'm not using the correct workflow (I just saved the raw email I\n> received for the patch as txt and fed it to 'git am').\n> Cherrypicking the commit onto maint works fine though, and the test\n> passes for me in this situation.\n\nOk, that's probably just fine.  I've not used git-am myself either.\n\n>>> The process I'm using to run the tests is a little strange though, it\n>>> seems I have to make git, then make contrib/subtree, then cp\n>>> git-subtree to the root before running the Makefile on the tests.  Let\n>>> me know if there's a less strange process for running the subtree\n>>> tests.\n>>\n>> I actually have an update that makes this easier but I haven't submitted\n>> it yet.  But yes, you've got the current process right.\n>>\n>\n> That will be nice.\n\nI submitted it yesterday.  Might take another round and then a few days\nto get it in.  I believe it would go into master since it's a new\n\"feature\" in the Makefile.\n\n> I've rewritten the test to use the repo/commit creation methods, and\n> renamed that branch. I've also added the comments you requested, and\n> changed the push to an ancestor check.\n> I'll be submitting the new version of the patch shortly.\n\nThank you!\n\n                    -David\n"},{"id":"276255","messageId":"8737tvbzh7.fsf@waller.obbligato.org","threadId":"40946","inReplyTo":"xmqq4meeisas.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7] contrib/subtree: fix \"subtree split\" skipped-merge bug","fromName":"David A. Greene","fromEmail":"greened@obbligato.org","sentAt":"2016-01-17T22:41:40Z","receivedAt":"2016-01-17T22:41:40Z","isPatch":true,"sender":{"key":"greened@obbligato.org","avatar":"https://avatars.githubusercontent.com/u/5291869?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Dave Ware <davidw@realtimegenomics.com> writes:\n>\n>> 'git subtree split' can incorrectly skip a merge even when both parents\n>> act on the subtree, provided the merge results in a tree identical to\n>> one of the parents. Fix by copying the merge if at least one parent is\n>> non-identical, and the non-identical parent is not an ancestor of the\n>> identical parent.\n>>\n>> Also, add a test case which checks that a descendant remains a\n>> descendent on the subtree in this case.\n>>\n>> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>\n>> ---\n>\n> David, how does this round look?  Can we proceed with your (and Eric's)\n> Reviewed-by: with this version (with one grammo fix Eric pointed out)?\n\nYes, this looks great to me!  Thanks Dave!\n\n                            -David\n\n>>\n>> Notes:\n>>     Many thanks to Eric Sunshine and Junio Hamano for adivce on this patch\n>>     Also many thanks to David A. Greene for help with subtree test style\n>>     \n>>     Changes since v6\n>>     - I forgot the notes when I sumbitted v6. (I have now set notes.rewriteRef,\n>>       so hopefully this wont happen again).\n>>     - Fixed some missing && in my test rewrite.\n>>     Changes since v5\n>>     - Rewrote test case to use subtree test repo and commit creation methods\n>>     - Added comments on what the test does and which bits are checked\n>>     - Added comments to test on related bugs which aren't fixed yet\n>>     Changes since v4\n>>     - Minor spelling and style fixes to test case\n>>     Changes since v3:\n>>     - Improvements to commit message\n>>     - Removed incorrect use of --boundary on rev-list\n>>     - Changed use of rev-list to use --count\n>>     Changes since v2:\n>>     - Minor improvements to commit message\n>>     - Changed space indentation to tab indentation in test case\n>>     - Changed use of rev-list for obtaining commit id to use rev-parse instead\n>>     Changes since v1:\n>>     - Minor improvements to commit message\n>>     - Added sign off\n>>     - Moved test case from own file into t7900-subtree.sh\n>>     - Added subshell to test around 'cd'\n>>     - Moved record of commit for cherry-pick to variable instead of dumping into file\n>>     \n>>     [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>\n>>  contrib/subtree/git-subtree.sh     | 12 ++++++--\n>>  contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++++++++++++++++++++++++++++\n>>  2 files changed, 70 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>> index edf36f8..5c83727 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -479,8 +479,16 @@ copy_or_skip()\n>>  \t\t\tp=\"$p -p $parent\"\n>>  \t\tfi\n>>  \tdone\n>> -\t\n>> -\tif [ -n \"$identical\" ]; then\n>> +\n>> +\tcopycommit=\n>> +\tif [ -n \"$identical\" ] && [ -n \"$nonidentical\" ]; then\n>> +\t\textras=$(git rev-list --count $identical..$nonidentical)\n>> +\t\tif [ \"$extras\" -ne 0 ]; then\n>> +\t\t\t# we need to preserve history along the other branch\n>> +\t\t\tcopycommit=1\n>> +\t\tfi\n>> +\tfi\n>> +\tif [ -n \"$identical\" ] && [ -z \"$copycommit\" ]; then\n>>  \t\techo $identical\n>>  \telse\n>>  \t\tcopy_commit $rev $tree \"$p\" || exit $?\n>> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n>> index 751aee3..3bf96a9 100755\n>> --- a/contrib/subtree/t/t7900-subtree.sh\n>> +++ b/contrib/subtree/t/t7900-subtree.sh\n>> @@ -1014,4 +1014,64 @@ test_expect_success 'push split to subproj' '\n>>  \t)\n>>  '\n>>  \n>> +#\n>> +# This test covers 2 cases in subtree split copy_or_skip code\n>> +# 1) Merges where one parent is a superset of the changes of the other\n>> +#    parent regarding changes to the subtree, in this case the merge\n>> +#    commit should be copied\n>> +# 2) Merges where only one parent operate on the subtree, and the merge\n>> +#    commit should be skipped\n>> +#\n>> +# (1) is checked by ensuring subtree_tip is a descendent of subtree_branch\n>> +# (2) should have a check added (not_a_subtree_change shouldn't be present\n>> +#     on the produced subtree)\n>> +#\n>> +# Other related cases which are not tested (or currently handled correctly)\n>> +# - Case (1) where there are more than 2 parents, it will sometimes correctly copy\n>> +#   the merge, and sometimes not\n>> +# - Merge commit where both parents have same tree as the merge, currently\n>> +#   will always be skipped, even if they reached that state via different\n>> +#   set of commits.\n>> +#\n>> +\n>> +next_test\n>> +test_expect_success 'subtree descendant check' '\n>> +\tsubtree_test_create_repo \"$subtree_test_count\" &&\n>> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/a &&\n>> +\t(\n>> +\t\tcd \"$subtree_test_count\" &&\n>> +\t\tgit branch branch\n>> +\t) &&\n>> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/0 &&\n>> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/b &&\n>> +\tcherry=$(cd \"$subtree_test_count\"; git rev-parse HEAD) &&\n>> +\t(\n>> +\t\tcd \"$subtree_test_count\" &&\n>> +\t\tgit checkout branch\n>> +\t) &&\n>> +\ttest_create_commit \"$subtree_test_count\" commit_on_branch &&\n>> +\t(\n>> +\t\tcd \"$subtree_test_count\" &&\n>> +\t\tgit cherry-pick $cherry &&\n>> +\t\tgit checkout master &&\n>> +\t\tgit merge -m \"merge should be kept on subtree\" branch &&\n>> +\t\tgit branch no_subtree_work_branch\n>> +\t) &&\n>> +\ttest_create_commit \"$subtree_test_count\" folder_subtree/d &&\n>> +\t(\n>> +\t\tcd \"$subtree_test_count\" &&\n>> +\t\tgit checkout no_subtree_work_branch\n>> +\t) &&\n>> +\ttest_create_commit \"$subtree_test_count\" not_a_subtree_change &&\n>> +\t(\n>> +\t\tcd \"$subtree_test_count\" &&\n>> +\t\tgit checkout master &&\n>> +\t\tgit merge -m \"merge should be skipped on subtree\" no_subtree_work_branch &&\n>> +\n>> +\t\tgit subtree split --prefix folder_subtree/ --branch subtree_tip master &&\n>> +\t\tgit subtree split --prefix folder_subtree/ --branch subtree_branch branch &&\n>> +\t\tcheck_equal $(git rev-list --count subtree_tip..subtree_branch) 0\n>> +\t)\n>> +'\n>> +\n>>  test_done\n"}]}