On Fri, Mar 12, 2010 at 2:56 PM, Paul Burba <ptbu...@gmail.com> wrote:
> On Fri, Mar 12, 2010 at 5:38 AM, Julian Foad <julian.f...@wandisco.com> wrote:
>> On Thu, 2010-03-11, Greg Stein wrote:
>>> On Thu, Mar 11, 2010 at 02:54, <julianf...@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
