On Wed, Oct 14, 2009 at 8:23 PM, Julian Foad <[email protected]> wrote: > Author: julianfoad > Date: Wed Oct 14 18:23:05 2009 > New Revision: 40041 > > Log: > * STATUS: Update my review status on r39019. > > Modified: > branches/1.6.x/STATUS > > Modified: branches/1.6.x/STATUS > URL: > http://svn.collab.net/viewvc/svn/branches/1.6.x/STATUS?pathrev=40041&r1=40040&r2=40041 > ============================================================================== > --- branches/1.6.x/STATUS Wed Oct 14 13:36:15 2009 (r40040) > +++ branches/1.6.x/STATUS Wed Oct 14 18:23:05 2009 (r40041) > @@ -114,8 +114,7 @@ Candidate changes: > ^/branches/1.6.x-r39109 > 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: 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.
