My vote for r879093 (was r39019) [was: svn commit: r40041 - branches/1.6.x]

[Julian Foad <ju...@btopenworld.com>]

On Mon, 2009-12-14, Paul Burba wrote:
> On Wed, Oct 14, 2009 at 8:23 PM, Julian Foad <ju...@btopenworld.com> wrote:
> > * STATUS: Update my review status on r39019.
[...]
> >      ^/branches/1.6.x-r39109

(The branch is now correctly named "...-r39019" so I've corrected that
reference to it.)

> >    Votes:
> >      +1: pburba
> > -     -0: julianfoad (tried to review but got stuck trying to understand what
> > -           combine_with_lastrange() is meant to do,
> > +     -0: julianfoad (reviewed the changes in mergeinfo.c only;
> >            and also it seems to sneak in another bug fix in
> >            fix_deleted_subtree_ranges(). I suggest splitting up this patch.)
> 
> Hi Julian,
> 
> Here is why that change is included:
[...]

Hi Paul. Thank you for the explanation. I've had another look and given
it a +1.

- Julian


> svn_rangelist_* APIs have always required forward rangelists:
> 
>  * (b) A "rangelist".  An array (@c apr_array_header_t *) of non-overlapping
>  *     merge ranges (@c svn_merge_range_t *), sorted as said by
>  *     @c svn_sort_compare_ranges().  An empty range list is represented by
>  *     an empty array.  Unless specifically noted otherwise, all APIs require
>  *     rangelists that describe only forward ranges, i.e. the range's start
>  *     revision is less than its end revision.
> 
> Prior to r879093(r40041)
> libsvn_client/merge.c:fix_deleted_subtree_ranges() called
> svn_rangelist_diff() with reversed ranges.  We simply lucked out that
> this has never caused any *known* problems.  AFAICT this abuse of the
> svn_rangelist_diff API in fix_deleted_subtree_ranges() could never
> cause an incorrect merge/error/assert in practice(1) prior to
> r879093...
> 
> ...But r879093 added the helper function
> libsvn_subr/mergeinfo.c:get_type_of_intersection() which is ultimately
> used by all the svn_rangelist_* and svn_mergeinfo_* APIs and is
> written assuming only forward merges are passed in; the function
> asserts if reverse ranges are passed to it.  So if I were to create a
> backport branch of r879093 minus the change to
> fix_deleted_subtree_ranges(), this assert would get triggered in
> previously working merges.  In our test suite this can be seen in
> merge_tests.py 65 'child having different rev ranges to merge'.  I'm
> pretty certain nobody would approve a backport which breaks the test
> suite.
> 
> I endeavor to keep each commit a logically discrete change, but in
> this case I think the fix of fix_deleted_subtree_ranges()'s API abuse
> is so interdependent with the core fix of r879093 that it made little
> sense to commit them separately.  Even if I had, they would both be
> nominated as a group for backport.
> 
> Paul
> 
> (1) I hesitate to get into to much detail why this is (it hardly seems
> relevant), but briefly this seems never to have bitten us due to the
> call to filter_merged_revisions() immediately preceding
> svn_rangelist_diff() in
> fix_deleted_subtree_ranges(calculate_remaining_ranges on the 1.6.x
> branch).  This removes any requested reverse merges which are not
> represented in explicit/implicit mergeinfo.  In combination with the
> temporary inheritable mergeinfo created by a merge under any paths
> with non-inheritable mergeinfo in the merge target, I can't find any
> combination or merges that would create a situation where a reverse
> merge would break, though this is quite dependent on the current
> implementation of get_type_of_intersection().  Assuming there were no
> asserts, an equally valid implementation of get_type_of_intersection
> could easily produce incorrect merges.