On Tue, Mar 16, 2010 at 8:58 AM, Julian Foad <julian.f...@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
