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

[David Ware <davidw@realtimegenomics.com>]

On Wed, Dec 9, 2015 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Ware <davidw@realtimegenomics.com> writes:
>
>> A bug occurs in 'git-subtree split' where a merge is skipped even when
>> both parents act on the subtree, provided the merge results in a tree
>> identical to one of the parents. Fix by copying the merge if at least
>> one parent is non-identical, and the non-identical parent is not an
>> ancestor of the identical parent.
>>
>> Also, add a test case which checks that a descendant can be pushed to
>> its ancestor in this case.
>>
>> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
>> ---
>
> The first sentence may be made clearer if you rephrased the early
> part of the sentence this way:
>
>         'git subtree split' can incorrectly skip a merge even when
>         both parents ...
>

Noted.

>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 9f06571..b837531 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -479,8 +479,16 @@ copy_or_skip()
>>                       p="$p -p $parent"
>>               fi
>>       done
>> -
>> -     if [ -n "$identical" ]; then
>> +
>> +     copycommit=
>> +     if [ -n "$identical" ] && [ -n "$nonidentical" ]; then
>> +             extras=$(git rev-list --boundary $identical..$nonidentical)
>> +             if [ -n "$extras" ]; then
>> +                     # we need to preserve history along the other branch
>> +                     copycommit=1
>> +             fi
>
> What is the significance of "--boundary" here?  I think for the
> purpose of "is the identical one part of the nonidentical one?" you
> do not need it, but there may be something subtle I missed.  I am
> asking this because use of "rev-list --boundary" in scripts is
> almost always a bug.
>

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

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

Thanks, I didn't know about that option.