[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r40041 - branches/1.6.x

From: Paul Burba <ptburba_at_gmail.com>
Date: Mon, 14 Dec 2009 14:36:38 -0500

On Wed, Oct 14, 2009 at 8:23 PM, Julian Foad <julianfoad_at_btopenworld.com> 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.
Received on 2009-12-14 20:37:15 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.