On Tue, Mar 16, 2010 at 8:58 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> On Mon, 2010-03-15, Paul Burba wrote:
>> 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.
>
> Thank you, Paul. That's great! Review...
>
>
>> +/* If DO_REMOVE is true, then remove any overlapping ranges described by
>> + RANGELIST1 from RANGELIST2 and place the results in *OUTPUT. When
>> + DO_REMOVE is true, RANGELIST1 is effectively the "eraser" and RANGELIST2
>> + the "whiteboard".
>> +
>> + If DO_REMOVE is false, then capture the intersection between RANGELIST1
>> + and RANGELIST2 and place the results in *OUTPUT. The ordering of
>> + RANGELIST1 and RANGELIST2 doesn't matter when DO_REMOVE is false.
>> +
>> + If CONSIDER_INHERITANCE is true, then take the inheritance of the
>> + ranges in RANGELIST1 and RANGELIST2 into account when comparing them
>> + for intersection, see the doc string for svn_rangelist_intersection().
>> +
>> + If CONSIDER_INHERITANCE is true, then ranges with differing inheritance
>> + may intersect, but the resulting intersection is non-inheritable only
>> + if both ranges were non-inheritable, e.g.:
>
> I think the last para means to say "If CONSIDER_INHERITANCE is
> *false* ...".
Indeed I did!
>> - const apr_array_header_t *eraser,
>> - const apr_array_header_t *whiteboard,
>> + const apr_array_header_t *rangelist1,
>> + const apr_array_header_t *rangelist2,
>
> I see where you were going with that rename, but actually the names
> "eraser" and "whiteboard" were rather helpful when thinking about the
> "remove" case. "1" and "2" would be OK if we keep reminding ourselves
> that we're "removing 1 from 2" (rather than calculating "1 - 2"), but
> unfortunately the correspondence of names is now slightly more confusing
> than it already was:
>
> rangelist1 - elt2 - j
> rangelist2 - elt1 - i - lasti - wboardelt
>
> :-) Could you clean up the names so they all correspond, like:
>
> rangelist1 - elt1 - i1
> rangelist2 - elt2 - i2 - lasti2 - working_elt2
>
> ?
>
> I'm suggesting "working_elt2" because it looks like "wboardelt" is a
> "working copy with local mods" of the last newly visited element from
> rangelist2. I couldn't quite grasp this from the comment "/* Instead of
> making a copy of the entire array of rangelist2 elements, we just keep a
> copy of the current rangelist2 element that needs to be used, and modify
> our copy if necessary. */"
>
> - Julian
Made those changes http://svn.apache.org/viewvc?view=revision&revision=925356
Paul
Received on 2010-03-19 19:02:03 CET