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
