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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 8, 2015, 06:49 UTC
Message-ID: <CAPig+cR36772YDc5RQRwXP3+ucVWumim9HYTXVMuGXN2cnQ7Ow@mail.gmail.com>
In-Reply-To: <1449521452-19043-1-git-send-email-davidw@realtimegenomics.com>

On Mon, Dec 7, 2015 at 3:50 PM, Dave Ware <davidw@realtimegenomics.com> wrote:

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

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

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

Show 7 quoted lines

> A bug occurs in 'git-subtree split' where a merge is skipped even when
> both parents act on the subtree, provided the merge results in a tree
> identical to one of the parents. Fix by copying the merge if at least
> one parent is non-identical, and the non-identical parent is not an
> ancestor of the identical parent.
>
> Also adding a test case, this checks that a descendant can be pushed to

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

> it's ancestor in this case.

s/it's/its/

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

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

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

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

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

Show 16 quoted lines

>  contrib/subtree/git-subtree.sh     | 12 +++++++--
>  contrib/subtree/t/t7900-subtree.sh | 52 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 9051982..ea991eb 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -468,4 +468,56 @@ test_expect_success 'verify one file change per commit' '
>         ))
>  '
>
> +test_expect_success 'subtree descendent check' '
> +  mkdir git_subtree_split_check &&
> +  (
> +    cd git_subtree_split_check &&

Style: indent with tabs rather than spaces

Show 18 quoted lines

> +    git init &&
> +
> +    mkdir folder &&
> +
> +    echo a >folder/a &&
> +    git add . &&
> +    git commit -m "first commit" &&
> +
> +    git branch branch &&
> +
> +    echo 0 >folder/0 &&
> +    git add . &&
> +    git commit -m "adding 0 to folder" &&
> +
> +    echo b >folder/b &&
> +    git add . &&
> +    git commit -m "adding b to folder" &&
> +    cherry=$(git rev-list HEAD -1) &&

git-rev-parse would probably be more idiomatic:

    cherry=$(git rev-parse HEAD)

Show 32 quoted lines

> +    git checkout branch &&
> +    echo text >textBranch.txt &&
> +    git add . &&
> +    git commit -m "commit to fiddle with branch: branch" &&
> +
> +    git cherry-pick $cherry &&
> +    git checkout master &&
> +    git merge -m "merge" branch &&
> +
> +    git branch noop_branch &&
> +
> +    echo d >folder/d &&
> +    git add . &&
> +    git commit -m "adding d to folder" &&
> +
> +    git checkout noop_branch &&
> +    echo moreText >anotherText.txt &&
> +    git add . &&
> +    git commit -m "irrelevant" &&
> +
> +    git checkout master &&
> +    git merge -m "second merge" noop_branch &&
> +
> +    git subtree split --prefix folder/ --branch subtree_tip master &&
> +    git subtree split --prefix folder/ --branch subtree_branch branch &&
> +    git push . subtree_tip:subtree_branch
> +  )
> +  '
> +
>  test_done
> --
> 1.9.1