[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r921713 - /subversion/trunk/subversion/libsvn_subr/mergeinfo.c

From: Paul Burba <ptburba_at_gmail.com>
Date: Mon, 15 Mar 2010 15:01:09 -0400

On Fri, Mar 12, 2010 at 2:56 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Fri, Mar 12, 2010 at 5:38 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
>> On Thu, 2010-03-11, Greg Stein wrote:
>>> On Thu, Mar 11, 2010 at 02:54,  <julianfoad_at_apache.org> wrote:
>> [...]
>>> > +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Thu Mar 11 07:54:16 2010
>>> >...
>>> > @@ -1988,13 +1963,9 @@ svn_mergeinfo__filter_mergeinfo_by_range
>>> >             {
>>> >               apr_array_header_t *new_rangelist;
>>> >
>>> > -              if (include_range)
>>> > -                SVN_ERR(svn_rangelist_intersect(&new_rangelist, rangelist,
>>> > -                                                filter_rangelist, FALSE,
>>> > -                                                result_pool));
>>> > -              else
>>> > -                SVN_ERR(svn_rangelist_remove(&new_rangelist, filter_rangelist,
>>> > -                                             rangelist, FALSE, result_pool));
>>> > +              SVN_ERR(rangelist_intersect_or_remove(
>>> > +                        &new_rangelist, filter_rangelist, rangelist,
>>> > +                        ! include_range, FALSE, result_pool));
>>>
>>> The intersect call had (rangelist, filter_rangelist), but the remove
>>> is opposite that. Yet the internal function passes along the one,
>>> consistent ordering. Are you sure that is not a problem?
>>>
>>> IOW, they all end up at rangelist_intersect_or_remove(), but you have
>>> changed the order for the include_range==TRUE (intersect) case.
>>
>> I'm confident that's correct: intersection is supposed to be
>> symmetrical.  I've just had another look, although haven't followed
>> completely through the logic of rangelist_intersect_or_remove() to check
>> for symmetry.
>>
>> Paul, could you review this change for me, please?
>
> Hi Julian and Greg,
>
> You are both right...which is a problem!
>
> The order of RANGELIST1 and RANGELIST2 *shouldn't* matter, and
> presently it does not when CONSIDER_INHERITANCE is TRUE.  However, I
> just discovered that it can matter when CONSIDER_INHERITANCE is FALSE:
>
>     WHITEBOARD  ERASER   CONSIDER     DO_REMOVE  *OUTPUT
>                          INHERITANCE
>     ----------  ------   -----------  ---------  -------
> 1)   1-100       90-420*  TRUE         FALSE      Empty Rangelist
> 2)   90-420*     1-100    TRUE         FALSE      Empty Rangelist
>
> 3)   90-420*     1-100*   FALSE        FALSE      90-100*
> 4)   1-100*      90-420*  FALSE        FALSE      90-100*
>
> 5)   90-420      1-100    FALSE        FALSE      90-100
> 6)   1-100       90-420   FALSE        FALSE      90-100
>
> 7)   1-100       90-420*  FALSE        FALSE      90-100
> 8)   90-420*     1-100    FALSE        FALSE      90-100*
>
> The first two result in no intersection because non-inheritable
> revision *N isn't the same as inheritable revision N when considering
> inheritance.
>
> The next four results make sense too, we don't consider inheritance,
> but all the arguments and the resulting intersection have a uniform
> inheritance.
>
> The last two results are where things get sketchy and depend on the
> order of the arguments.  The docstrings for svn_rangelist_intersect()
> both say the intersection should always be inheritable:
>
>  "If @a consider_inheritance is FALSE then
>   the ranges in @a *rangelist are always inheritable."
>
> Obviously that is not happening in 3), 4) or 8).  As to whether 7) is
> wrong or 8) is wrong, well I think we'll all agree the inconsistency
> is bad, so either it should be
>
> 7)   1-100       90-420*  FALSE        FALSE      90-100*
> 8)   90-420*     1-100    FALSE        FALSE      90-100*
>
> or
>
> 7)   1-100       90-420*  FALSE        FALSE      90-100
> 8)   90-420*     1-100    FALSE        FALSE      90-100
>
> I think the latter is correct.  I also think the docstring is wrong
> and that 3) and 4) demonstrate correct behavior.
>
> In all three cases we are saying we don't care about inheritance, so
> we allow a non-inheritable range to intersect with an inheritable
> range, I think the intersection should behave as if we used
> svn_rangelist_merge() to combine the two intersecting parts.  That
> API, and svn_mergeinfo_merge() only result in non-inheritable
> mergeinfo if both parts are non-inheritable:
>
>  * When intersecting rangelists for a path are merged, the inheritability of
>  * the resulting svn_merge_range_t depends on the inheritability of the
>  * operands.  If two non-inheritable ranges are merged the result is always
>  * non-inheritable, in all other cases the resulting range is inheritable.
>  *
>  *  e.g. '/A: 1,3-4'  merged with '/A: 1,3,4*,5' --> '/A: 1,3-5'
>  *       '/A: 1,3-4*' merged with '/A: 1,3,4*,5' --> '/A: 1,3,4*,5'
>
> I'm testing a patch right now that changes svn_rangelist_intersect()
> and svn_mergeinfo_intersect2() to behave this way.  I definitely know
> of no part of the merge tracking logic that depends on the
> inconsistency between 7) and 8) and I don't recall any that relies on
> the intersection always being inheritable.
>
> Paul

In http://svn.apache.org/viewvc?view=revision&revision=923389 I fixed
the bug where the order of the rangelist arguments to
svn_rangelist_intersect() could produce different results. Now the
intersection of two ranges with different inheritance always results
in an inheritable rangelist, regardless of the order in which they are
passed.

I also changed the doc strings for svn_rangelist_intersect() and
svn_mergeinfo_intersect2() to reflect the existing behavior where the
resulting intersection of two non-inheritable ranges is also
non-inheritable, not inheritable as had been claimed.

So now everything is consistent: The intersection of two ranges is
only non-inheritable if both of the ranges are non-inheritable.

If necessary, we can have a discussion about whether this behavior
should be different, but making it consistent was a no-brainer first
step.

Paul
Received on 2010-03-15 20:01:37 CET

This is an archived mail posted to the Subversion Dev mailing list.