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* ...".
> - 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
> 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-16 13:59:03 CET