On Tue, Nov 27, 2012 at 6:53 PM, Julian Foad <julianf...@btopenworld.com> wrote:
> Johan Corveleyn wrote:
>> On Sun, Nov 11, 2012 at 1:59 PM, Stefan Fuhrmann wrote:
>>> On Thu, Nov 8, 2012 at 6:04 PM, Julian Foad wrote:
>>>> Stefan Fuhrmann wrote:
>>>>> But I did run across an assertion in mergeinfo.c, :line 1174
>>>>>> during merge_tests.py 125:
>>>>>>>
>>>>>>> SVN_ERR_ASSERT_NO_RETURN(IS_VALID_FORWARD_RANGE(first));
>
> I debugged this assertion failure and came up with this fix:
>
> [[[
> Fix issue #4132 "merge of replaced source asserts", fixing merge_tests 125.
>
> * subversion/libsvn_client/merge.c
> (find_gaps_in_merge_source_history): Fix an off-by-1 bug. Assert that the
> function's result constitutes a valid non-empty range.
>
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 1414469)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -4310,7 +4310,7 @@ find_gaps_in_merge_source_history(svn_re
> /* Get SOURCE as mergeinfo. */
> SVN_ERR(svn_client__get_history_as_mergeinfo(&implicit_src_mergeinfo, NULL,
> primary_src,
> - primary_src->rev, old_rev,
> + primary_src->rev, old_rev + 1,
> ra_session,
> ctx, scratch_pool));
>
> @@ -4384,6 +4384,9 @@ find_gaps_in_merge_source_history(svn_re
> SVN_ERR_ASSERT(*gap_start == MIN(source->loc1->rev, source->loc2->rev)
> || (*gap_start == SVN_INVALID_REVNUM
> && *gap_end == SVN_INVALID_REVNUM));
> + SVN_ERR_ASSERT(*gap_end > *gap_start
> + || (*gap_start == SVN_INVALID_REVNUM
> + && *gap_end == SVN_INVALID_REVNUM));
> return SVN_NO_ERROR;
> }
> ]]]
>
> Trouble is, this fix makes merge_tests.py 100 fail.
>
> I haven't yet investigated further.
Hi Julian,
(Let me say I'm thrilled this doesn't involve subtree mergeinfo :-)
merge_tests.py 100 demonstrates the problem with what you propose
above, but we can simplify things a bit since the underlying problem
doesn't have anything to do with reverse merges or a merge target with
uncommitted modifications (which are present in the test failure).
Let's start where merge_tests.py 100 fails, but first let's revert the
local changes. This leaves us with this WC based on this:
Rez 'A'
from 'A@2'
r1-------r2---r3---r4---r5----r6 r7--------r9---------->
Add 'A' edit edit edit edit Delete ^ | edit
psi rho beta omega 'A' | | gamma
\ | |
\___________copy____________/ |
|
|
|
r8---------------->
Copy 'A@7'
to 'A_COPY'
Let's try a cherry pick merge from A to A_COPY that spans the deletion
and resurrection of 'A':
Rez 'A'
from 'A@2'
r1-------r2---r3---r4---r5----r6 r7--------r9---------->
Add 'A' edit edit edit edit Delete ^ | edit |
psi rho beta omega 'A' | | gamma |
\ | | |
\___________copy____________/ | merge
| ^/A -r0:9
| |
| V
r8---------------->
Copy 'A@7'
to 'A_COPY'
Without your preceeding patch the merge does what I think is the right thing:
>svn merge -r0:9 ^^/A A_COPY
--- Merging r8 through r9 into 'A_COPY':
U A_COPY\D\gamma
--- Recording mergeinfo for merge of r8 through r9 into 'A_COPY':
U A_COPY
Ok, so what happens with your patch. First merge.c:merge_peg_locked()
calls normalize_merge_sources(), which notices the copy of 'A' at r7
and returns two merge_source_t to reflect this:
loc1
+ repos_root_url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100"
+ repos_uuid "6156da17-bf42-b040-a6df-5ea1a8d7d9d7"
rev 1
+ url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100/A"
loc2
+ repos_root_url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100"
+ repos_uuid "6156da17-bf42-b040-a6df-5ea1a8d7d9d7"
rev 2
+ url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100/A"
ancestral 1
loc1
+ repos_root_url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100"
+ repos_uuid "6156da17-bf42-b040-a6df-5ea1a8d7d9d7"
rev 2
+ url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100/A"
loc2
+ repos_root_url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100"
+ repos_uuid "6156da17-bf42-b040-a6df-5ea1a8d7d9d7"
rev 9
+ url
"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-100/A"
ancestral 1
The merge of -r0:1 is obviously a no-op.
Picking up the second merge_source_t in merge.c:populate_remaining_ranges()
/* If, in the merge source's history, there was a copy from an older
revision, then SOURCE->loc2->url won't exist at some range M:N, where
SOURCE->loc1->rev < M < N < SOURCE->loc2->rev. The rules of 'MERGEINFO
MERGE SOURCE NORMALIZATION' allow this, but we must ignore these gaps
when calculating what ranges remain to be merged from SOURCE. If we
don't and try to merge any part of SOURCE->loc2->url@M:N we would
break the editor since no part of that actually exists. See
http://svn.haxx.se/dev/archive-2008-11/0618.shtml.
Find the gaps in the merge target's history, if any. Eventually
we will adjust CHILD->REMAINING_RANGES such that we don't describe
non-existent paths to the editor. */
SVN_ERR(find_gaps_in_merge_source_history(&gap_start, &gap_end,
source,
ra_session, merge_b->ctx,
iterpool));
You might want to take a look at the referenced thread,
http://svn.haxx.se/dev/archive-2008-11/0618.shtml, because the problem
I solved there is exactly the one your patch above recreates.
Once in find_gaps_in_merge_source_history(), we call
svn_client__get_history_as_mergeinfo(), which without your patch
returns this mergeinfo:
/A:2,7-9
But with your patch, which adds 1 to the RANGE_OLDEST arg of
svn_client__get_history_as_mergeinfo(), the following mergeinfo is
returned:
/A:7-9
This is a problem because we don't detect the copy and thus don't
detect the gap:
/* A gap in natural history can result from either a copy or
a rename. If from a copy then history as mergeinfo will look
something like this:
'/trunk:X,Y-Z'
If from a rename it will look like this:
'/trunk_old_name:X'
'/trunk_new_name:Y-Z'
In both cases the gap, if it exists, is M-N, where M = X + 1 and
N = Y - 1.
Note that per the rules of 'MERGEINFO MERGE SOURCE NORMALIZATION' we
should never have multiple gaps, e.g. if we see anything like the
following then something is quite wrong:
'/trunk_old_name:A,B-C'
'/trunk_new_name:D-E'
*/
### rangelist->nelts == 1 with your patch
### so we never enter this block
### VVV
if (rangelist->nelts > 1) /* Copy */
{
/* As mentioned above, multiple gaps *shouldn't* be possible. */
SVN_ERR_ASSERT(apr_hash_count(implicit_src_mergeinfo) == 1);
*gap_start = MIN(source->loc1->rev, source->loc2->rev);
*gap_end = (APR_ARRAY_IDX(rangelist,
rangelist->nelts - 1,
svn_merge_range_t *))->start;
}
Back in merge.c:populate_remaining_ranges() we never know about the
gap so we don't stash it in the merge cmd baton:
/* Stash any gap in the merge command baton, we'll need it later when
recording mergeinfo describing this merge. */
if (SVN_IS_VALID_REVNUM(gap_start) && SVN_IS_VALID_REVNUM(gap_end))
merge_b->implicit_src_gap = svn_rangelist__initialize(gap_start, gap_end,
TRUE, result_pool);
Then later in populate_remaining_ranges(), we don't adjust our
remaining ranges to merge:
/* Deal with any gap in SOURCE's natural history.
If the gap is a proper subset of CHILD->REMAINING_RANGES then we can
safely ignore it since we won't describe this path/rev pair.
If the gap exactly matches or is a superset of a range in
CHILD->REMAINING_RANGES then we must remove that range so we don't
attempt to describe non-existent paths via the reporter, this will
break the editor and our merge.
If the gap adjoins or overlaps a range in CHILD->REMAINING_RANGES
then we must *add* the gap so we span the missing revisions. */
if (child->remaining_ranges->nelts
&& merge_b->implicit_src_gap)
...
Ok, I'm rambling, so let me move to the summing up part :-)
The above leaves the merge code thinking that the requested merge is:
/A:2-9 (The naive merge request)
Less what's already been merged per the target's implicit mergeinfo:
/A:2,7
/A_COPY:8-9
Less what's already been merged per the target's explict mergefino:
"" (None in this case)
Which leaves:
/A:3-6,8-9
To be merged. We should have removed the non-existent gap /A:3-6, but
as we saw above we didn't. Skipping ahead to
drive_merge_report_editor(), we try to merge this gap, calling
svn_client__get_diff_editor(revision=2) and then
svn_ra_do_diff3(revision=6). Since A@6 doesn't exist, the editor
breaks:
>svn merge -r0:9 ^^/A A_COPY
..\..\..\subversion\svn\util.c:913: (apr_err=160005)
..\..\..\subversion\libsvn_client\merge.c:11074: (apr_err=160005)
..\..\..\subversion\libsvn_client\merge.c:11040: (apr_err=160005)
..\..\..\subversion\libsvn_client\merge.c:9219: (apr_err=160005)
..\..\..\subversion\libsvn_client\merge.c:8925: (apr_err=160005)
..\..\..\subversion\libsvn_client\merge.c:8787: (apr_err=160005)
..\..\..\subversion\libsvn_client\merge.c:5243: (apr_err=160005)
..\..\..\subversion\libsvn_repos\reporter.c:1543: (apr_err=160005)
..\..\..\subversion\libsvn_repos\reporter.c:1458: (apr_err=160005)
..\..\..\subversion\libsvn_repos\reporter.c:1370: (apr_err=160005)
svn: E160005: Target path '/A' does not exist
Anyway, I hope that helps explain what is happening here.
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
