Thanks, Paul!
This is great. You're right, the main thing seems to be that find_symmetric_merge is currently only fetching and analyzing the branch-root mergeinfo. It needs to fetch and analyze all mergeinfo on the branch, finding a merge base for each subtree, and proceed something like this:
* If every base found is on the source branch:
-> choose the oldest base, and
-> pass this base to the "sync merge" code path.
* If the same base is found for all subtrees
and this base is on the target branch:
-> pass this base to the "reintegrate" code path.
* If different bases are found for different subtrees
and any base is on the target branch:
-> complain like "reintegrate" does in 1.7.
The first case (bases all on the source) provides what a 1.7 "sync" merge can currently handle: this should enable a merge that falls under this case to merge all subtree ranges that need to be merged, just like a 1.7 sync merge. Currently the symmetric merge will only end up merging subtree ranges that fall within the overall range that it determined for the branch root, but with this change it will merge all subtree ranges even if some of those fall outside (that is, are older than) the branch root range.
The second case will cover what a "reintegrate" merge can do.
The third case will ensure "symmetric merge" will bail out when the current reintegrate merge would bail out, instead of (as it currently does) going ahead with a merge that will in fact miss out some subtree ranges that need to be merged.
Does that sound right?
Now, I ought to draw up some documentation that explains this -- explains exactly what the merge is supposed to do in the presence of subtree merges. I'm currently writing my SvnDay talk so I may be able to come up with a visual description for presentation (this is likely to be before it's had time to be reviewed properly) and then transcribe it into a more formal document afterwards.
In working through your analysis of merge_reintegrate_tests.py 10, below, I added some diagrams to the test to show at a glance what it's doing. I think the diagrams are immensely helpful; let me know if you have any concern about them.
I'd love to implement that myself but am on leave next week so probably can't do it until after the Hackathon, so if anyone else is keen to get involved, this could be a great place to dive in.
- Julian
Paul Burba wrote:
> On Fri, May 25, 2012 at 12:08 PM, Julian Foad wrote:
>> The following tests fail when merge-cmd.c is patched to call "symmetric
>> merge" for sync and reintegrate merges [4]:
>> FAIL: merge_reintegrate_tests.py 10: merge --reintegrate with subtree
>> mergeinfo
>
> Let's focus on what this test is doing at a high level and ignore some
> unnecessary details. Basically the test case does the following:
> 1) branch 'A_at_1' is copied to 'A_COPY'
>
> 2) changes are made on both 'A' and 'A_COPY'
>
> 3) Sync merge ^/A to 'A_COPY', bringing the latter up to r12
>
> 4) Do a reverse-subtree-cherry-pick merge: svn merge ^/A/D -c-8 A_COPY/D
> This "undoes" part of the sync merge in step 3 creates new subtree
> mergeinfo on A_COPY to represent this reversal (Omitting irrelevant
> mergeinfo actually found in the test) :
>
> Properties on 'A_COPY':
> svn:mergeinfo
> /A:2-12
> Properties on 'A_COPY\D':
> svn:mergeinfo
> /A/D:2-7,9-12
>
> 5) The test then tries to reintegrate ^/A_COPY to A. A
> 1.7/Old-style/Non-symmetric merge notices that the mergeinfo "gap"
> created by the reverse merge, '/A/D:8' represents an operative change,
>
> trunk_at_1342725>svn diff -c8
> Index: A/D/H/omega
> ===================================================================
> --- A/D/H/omega (revision 7)
> +++ A/D/H/omega (revision 8)
> @@ -1 +1 @@
> -This is the file 'omega'.
> +New content
> \ No newline at end of file
>
> and then fails with this error:
>
> trunk_at_1342725>svn merge ^^/A_COPY A --reintegrate
> ..\..\..\subversion\svn\merge-cmd.c:382: (apr_err=195016)
> ..\..\..\subversion\libsvn_client\merge.c:10746: (apr_err=195016)
> ..\..\..\subversion\libsvn_client\merge.c:10715: (apr_err=195016)
> ..\..\..\subversion\libsvn_client\merge.c:10715: (apr_err=195016)
> ..\..\..\subversion\libsvn_client\merge.c:10653: (apr_err=195016)
> svn: E195016: Reintegrate can only be used if revisions 2 through 15
> were previously merged from
> file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_reintegrate_tests-10/A
> to the reintegrate source, but this is not the case:
> A_COPY/D
> Missing ranges: /A/D:8
>
> The symmetric merge on the other hand, blissfully ignores this subtree
> mergeinfo and lets the "reintegrate" succeed...
>
> trunk_at_1342725>svn merge ^^/A_COPY A --reintegrate --symmetric
> DBG: merge.c:11461: base on source:
> file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_reintegrate_tests-10/A_at_1
> DBG: merge.c:11462: base on target:
> file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_reintegrate_tests-10/A_at_12
> DBG: merge.c:11567: yca
> file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_reintegrate_tests-10/A_at_1
> DBG: merge.c:11568: base
> file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_reintegrate_tests-10/A_at_12
> DBG: merge.c:11570: mid
> file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_reintegrate_tests-10/A_at_12
> DBG: merge.c:11571: right
> file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_reintegrate_tests-10/A_COPY_at_15
> --- Merging differences between repository URLs into 'A':
> U A\B\E\alpha
> UU A\mu
> U A\D\H\omega
> U A\D
> --- Recording mergeinfo for merge between repository URLs into 'A':
> U A
> U A\D
> G A\mu
>
> ...which means that r8 is reverse merged from 'A' now too (which is
> quite likely *not* what the user expected):
>
> trunk_at_1342725>svn diff A\D\H\omega
> Index: A/D/H/omega
> ===================================================================
> --- A/D/H/omega (revision 14)
> +++ A/D/H/omega (working copy)
> @@ -1 +1 @@
> -New content
> \ No newline at end of file
> +This is the file 'omega'.
>
> This isn't surprising since AFAICT
> merge.c:svn_client__find_symmetric_merge and
> merge.c:svn_client__do_symmetric_merge completely ignore subtree
> mergeinfo on the source. The "old" non-symmetric code path checks
> that the entire mergeinfo catalog on the source (i.e. both the
> explicit mergeinfo on the source and the source's subtree mergeinfo)
> represent a fully synced source (see merge.c:find_unsynced_ranges).
>
> If this test's particular use-case strikes you as too contrived, see
> the attached patch which adds a new merge_symmetric test that
> demonstrates a more (IMO) plausible use-case (one that uses only
> forward sync merges).
>
>> FAIL: merge_tests.py 88: subtree merges dont cause spurious conflicts
>
>This failure is similar to merge_reintegrate_tests.py 10 in that
>subtree mergeinfo is ignored by the symmetric merge and only the
>mergeinfo on the target itself is considered.
>
>Briefly, right before the failure we have this mergeinfo on the branch target:
>
>trunk_at_1343721>svn pl -vR
>Properties on 'A_COPY':
> svn:mergeinfo
> /A:2-7
>Properties on 'A_COPY\D\H\psi':
> svn:mergeinfo
> /A/D/H/psi:2,4-7
>
>If we, as symmetric merge does, consider only the mergeinfo on the
>root of the merge target, then the only operative revision eligible
>for merging is r8:
>
> trunk_at_1343721>svn mergeinfo --show-revs eligible ^^/A A_COPY
> r8
>
>But if we consider subtree mergeinfo then we see that r3 is also
>eligible for a subtree (a file) of the target:
>
> trunk_at_1343721>svn mergeinfo --show-revs eligible ^^/A A_COPY -R
> r3*
> r8
>
> trunk_at_1343721>svn mergeinfo --show-revs eligible ^^/A/D/H/psi A_COPY/D/H/psi
> r3
>
>The old merge code takes notice of the subtree mergeinfo and drives
>the editor such that r3 is applied only to A_COPY/D/H/psi:
>
>trunk_at_1343721>svn merge ^^/A A_COPY
>--- Merging r3 into 'A_COPY\D\H\psi':
>U A_COPY\D\H\psi
>--- Merging r8 through r9 into 'A_COPY':
>U A_COPY\D\G\rho
>--- Recording mergeinfo for merge of r3 through r9 into 'A_COPY':
>U A_COPY
>--- Recording mergeinfo for merge of r3 through r9 into 'A_COPY\D\H\psi':
>U A_COPY\D\H\psi
>--- Eliding mergeinfo from 'A_COPY\D\H\psi':
>U A_COPY\D\H\psi
>
>trunk_at_1343721>svn st
>M A_COPY
>M A_COPY\D\G\rho
>MM A_COPY\D\H\psi
>
>trunk_at_1343721>svn pl -vR
>Properties on 'A_COPY':
> svn:mergeinfo
> /A:2-9
>
>trunk_at_1343721>svn revert -R . -q
>
>The symmetric merge, ignoring the subtree mergeinfo on A_COPY/D/H/psi,
>thinks only r8 needs to be merged:
>
>trunk_at_1343721>svn merge ^^/A A_COPY --symmetric
>DBG: merge.c:11461: base on source:
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A_at_7
>DBG: merge.c:11462: base on target:
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A_at_1
>DBG: merge.c:11567: yca
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A_at_1
>DBG: merge.c:11568: base
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A_at_7
>DBG: merge.c:11571: right
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A_at_9
>--- Merging r8 through r9 into 'A_COPY':
>U A_COPY\D\G\rho
>--- Recording mergeinfo for merge of r8 through r9 into 'A_COPY':
>U A_COPY
>
>trunk_at_1343721>svn st
>M A_COPY
>M A_COPY\D\G\rho
>
>trunk_at_1343721>svn pl -vR
>Properties on 'A_COPY':
> svn:mergeinfo
> /A:2-9
>Properties on 'A_COPY\D\H\psi':
> svn:mergeinfo
> /A/D/H/psi:2,4-7
>
>So we can sync all day with the symmetric code path and r3 will never
>get merged.
>C:\SVN\src-trunk\Debug\subversion\tests\cmdline\svn-test-work\working_copies\merge_tests-88>
>
>> FAIL: merge_tests.py 89: target and subtrees need nonintersecting revs
>
>I won't go into any details here, this is exactly the same problem as
>merge_tests.py 89.
>
>> FAIL: merge_tests.py 78: dont merge revs into a subtree that predate it
>
>Similar problem here to the other two merge tests, except that the
>problem is obscured a bit by the fact that:
>
>1) The target itself has no mergeinfo, only the subtree does.
>2) The resulting working copy state is identical regardless of whether
>the subtree mergeinfo is considered, so the test only fails because
>the notification headers differ:
>
>Symmetric Merge:
>
>trunk_at_1343721>svn st
>MM H_COPY\nu
>
>trunk_at_1343721>svn pl -vR
>Properties on 'H_COPY\nu':
> svn:mergeinfo
> /A/D/H/nu:7
>
>trunk_at_1343721>svn merge ^^/A/D/H H_COPY --force --symmetric
>DBG: merge.c:11461: base on source:
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-78/A/D/H_at_5
>DBG: merge.c:11462: base on target:
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-78/A/D/H_at_5
>DBG: merge.c:11567: yca
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-78/A/D/H_at_5
>DBG: merge.c:11568: base
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-78/A/D/H_at_5
>DBG: merge.c:11571: right
>file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-78/A/D/H_at_9
>--- Merging r7 through r9 into 'H_COPY':
>U H_COPY\psi
>D H_COPY\nu
>--- Recording mergeinfo for merge of r6 through r9 into 'H_COPY':
>U H_COPY
>
>trunk_at_1343721>svn st
>M H_COPY
>D H_COPY\nu
>M H_COPY\psi
>
>trunk_at_1343721>svn pl -vR
>Properties on 'H_COPY':
> svn:mergeinfo
> /A/D/H:6-9
>
>trunk_at_1343721>svn revert -R .
>Reverted 'H_COPY'
>Reverted 'H_COPY\psi'
>Reverted 'H_COPY\nu'
>
>trunk_at_1343721>cd ..
>
>trunk_at_1343721>cd merge_tests-78
>
>Old merge:
>
>trunk_at_1343721>svn st
>MM H_COPY\nu
>
>trunk_at_1343721>svn pl -vR
>Properties on 'H_COPY\nu':
> svn:mergeinfo
> /A/D/H/nu:7
>
>trunk_at_1343721>svn merge ^^/A/D/H H_COPY --force
>--- Merging r6 through r9 into 'H_COPY':
> ^^^^^
> r6 rather than r7, everything else is the same.
>U H_COPY\psi
>D H_COPY\nu
>--- Recording mergeinfo for merge of r6 through r9 into 'H_COPY':
>U H_COPY
>
>trunk_at_1343721>svn st
>M H_COPY
>D H_COPY\nu
>M H_COPY\psi
>
>trunk_at_1343721>svn pl -vR
>Properties on 'H_COPY':
> svn:mergeinfo
> /A/D/H:6-9
>
>> Clearly there's something up with subtree merges,
>
>Let's jump back to the simple example in merge_tests.py 88 to take a
>closer look at what's going on. Recall prior to the merge we have
>this mergeinfo on our merge target:
>
>trunk_at_1343721>svn pl -vR
>Properties on 'A_COPY':
> svn:mergeinfo
> /A:2-7
>Properties on 'A_COPY\D\H\psi':
> svn:mergeinfo
> /A/D/H/psi:2,4-7
>
>An old-style merge feeds merge.c:do_merge() a single merge_source_t:
>
> source
> loc1
> repos_root_url
>"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88"
> repos_uuid "3c093481-932f-b94f-a0fe-b71a763dd933"
> rev 1
> url "file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A"
> loc2
> repos_root_url
>"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88"
> repos_uuid "3c093481-932f-b94f-a0fe-b71a763dd933"
> rev 9
> url "file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A"
> ancestral 1
>
>Your symmetric merge on the other hand passes this single merge_source_t:
>
> source
> loc1
> repos_root_url
>"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88"
> repos_uuid "3c093481-932f-b94f-a0fe-b71a763dd933"
> rev 7
> url "file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A"
> loc2
> repos_root_url
>"file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88"
> repos_uuid "3c093481-932f-b94f-a0fe-b71a763dd933"
> rev 9
> url "file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-88/A"
> ancestral 1
>
>Notice that source->loc1->rev is r1 in the first case and r7 in the
>second (symmetric) case. Because the symmetric merge doesn't consider
>r1-6, all the subtree mergeinfo logic that exists within do_merge and
>it's helpers never even has the chance to consider /A:1-6 as part of
>the requested merge.
>
>So what to do? Maybe make merge.c:find_symmetric_merge() retrieve the
>subtree mergeinfo for s_t->target->abspath, not just the
>explicit/inherited mergeinfo (i.e. call
>svn_client__get_wc_or_repos_mergeinfo_catalog rather than
>svn_client__get_wc_or_repos_mergeinfo). It could then evaluate the
>totality of mergeinfo on/under the target when determining "the latest
>revision of A synced to B and the latest revision of B synced to A."
>Yes, I'm doing a lot of hand-waving here :-)
>
>--
>Paul T. Burba
>CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
>Skype: ptburba
>
>> but, as I said above, I
>> have reason to believe that it's not fundamentally broken or unsupported.
>>
>> New tests:
>>
>> I've started "merge_symmetric_tests.py".
>>
>> Are any new tests required to ensure existing scenarios aren't broken, that
>> may not be tested yet?
>>
>> - The "keep-alive dance". For completeness, we should check how that will
>> behave, as we can assume some people will have adopted practices that
>> incorporate it. I do NOT think we should continue to support that
>> work-around: if it continues to behave as now, that's fine, and if its
>> behaviour changes, I expect to be able to claim that as an intentional
>> behaviour change for the better. That is, to be plain, if anyone's relying
>> on that, they may need to change their practice.
>>
>> - The tests I have at the moment are pretty small cases. It would be good
>> to create a test that exercises a series of much bigger merges.
>>
>> - Any other scenario?
>>
>>
>> PERFORMANCE
>>
>> - Performance (in terms of network traffic, in particular). After a
>> series of (same direction or to-and-fro) merges, is the cost of the
>> base-finding algorithm proportional to time since the YCA of the branches
>> (i.e., ever increasing), or only proportional to time since last merge?
>>
>> - Any other performance concern?
>>
>>
>> Please let me know any thoughts you have. And if you might be able to take
>> on the investigation of one of the test failures, or writing a new test
>> (whether pseudo-code or actual code), or checking the performance, that
>> would be awesome.
>>
>> - Julian
>>
>>
>> [1] <http://wiki.apache.org/subversion/SymmetricMerge>
>> [2] <http://www.elego.de/svnday2012>
>> [3] Define a "simple" merge as one that does not find any subtree merges,
>> cherry picks, mixed-rev/switched/sparse WC.
>> [4] Attached patch, "use-symmetric-merge-1.patch", makes all (?) sync and
>> reint merge requests use the 'symmetric' code.
>
>
>
Received on 2012-05-30 15:14:02 CEST