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

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
Received on 2010-03-12 19:56:38 CET