Re: [PATCH v3] contrib/subtree: fix "subtree split" skipped-merge bug
On Wed, Dec 9, 2015 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> Dave Ware <davidw@realtimegenomics.com> writes:
>
>> A bug occurs in 'git-subtree split' where a merge is skipped even when
>> both parents act on the subtree, provided the merge results in a tree
>> identical to one of the parents. Fix by copying the merge if at least
>> one parent is non-identical, and the non-identical parent is not an
>> ancestor of the identical parent.
>>
>> Also, add a test case which checks that a descendant can be pushed to
>> its ancestor in this case.
>>
>> Signed-off-by: Dave Ware <davidw@realtimegenomics.com>
>> ---
>
> The first sentence may be made clearer if you rephrased the early
> part of the sentence this way:
>
> 'git subtree split' can incorrectly skip a merge even when
> both parents ...
>
Show 25 quoted lines
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 9f06571..b837531 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -479,8 +479,16 @@ copy_or_skip()
>> p="$p -p $parent"
>> fi
>> done
>> -
>> - if [ -n "$identical" ]; then
>> +
>> + copycommit=
>> + if [ -n "$identical" ] && [ -n "$nonidentical" ]; then
>> + extras=$(git rev-list --boundary $identical..$nonidentical)
>> + if [ -n "$extras" ]; then
>> + # we need to preserve history along the other branch
>> + copycommit=1
>> + fi
>
> What is the significance of "--boundary" here? I think for the
> purpose of "is the identical one part of the nonidentical one?" you
> do not need it, but there may be something subtle I missed. I am
> asking this because use of "rev-list --boundary" in scripts is
> almost always a bug.
>
The other way around actually I'm trying to determine if nonidentical
contains any commits
not in identical. I'll confess I don't actually know specifically
what the --boundary option
does, this probably came from a stack overflow example while we were
looking up how to
best do the check. Further experimentation with the option suggests
that it does not do what
I want, so I will remove it. Thank you.
Show 5 quoted lines
> Also, depending on how huge the output from the rev-list could be,
> you might want to use "rev-list --count $i..$n" and compare it with
> 0 instead--that way, you would not have to be worried about having
> to carry around a huge string that you would otherwise not use, only
> to see if that string is empty.
Thanks, I didn't know about that option.
David Ware <davidw@realtimegenomics.com>