On Fri, Dec 18, 2009 at 11:41 AM, Philip Martin
<philip.mar...@wandisco.com> wrote:
> Paul Burba <ptbu...@gmail.com> writes:
>
>> "This may all be moot because I fixed the bug in r892085 and nominated
>> this for backport to 1.6.7. I also nominated a test which reproduces
>> the bug Hyrum found. This test fails across the board on trunk,
>> 1.6.x, and 1.6.6 without the the r892085 fix in place."
>
> I don't think your test case is exactly the same bug since your
> testcase affects both 1.6.x and 1.6.6 while the bug on the Subversion
> repository doesn't affect 1.6.6. Merging into the 1.6.x.source:
Hi Philip,
Right, it's not exactly the same because it is only testing the
underlying reintegrate bug, not anything to do with the *separate* rel
pathed mergeinfo handling bug which would obscure the first bug. If
we want the test to conflate the two issues, we could apply this
patch:
[[[
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 892317)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -17194,10 +17194,14 @@
# /A:3 describes A2's natural history, a.k.a. it's implicit mergeinfo, so
# it is self-referential. Same for /A/B:4 and A2/B. Normally this is
# redundant but not harmful.
+ #
+ # Also, make this self-referential mergeinfo use relative merge source
+ # paths to replicate the type of bogus mergeinfo that issue #3547 might
+ # let into the repository.
svntest.actions.run_and_verify_svn(None, None, [],
- 'ps', 'svn:mergeinfo', '/A:3', A2_path)
+ 'ps', 'svn:mergeinfo', 'A:3', A2_path)
svntest.actions.run_and_verify_svn(None, None, [],
- 'ps', 'svn:mergeinfo', '/A/B:4',
A2_B_path)
+ 'ps', 'svn:mergeinfo', 'A/B:4', A2_B_path)
svntest.actions.run_and_verify_svn(
None, None, [], 'ci', '-m',
'copy A to A2 and set some self-referential mergeinfo on the latter.',
@@ -17246,8 +17250,8 @@
})
expected_status.tweak(wc_rev=8)
expected_disk = wc.State('', {
- '' : Item(props={SVN_PROP_MERGEINFO : '/A:3\n/A2.1:7-8'}),
- 'B' : Item(props={SVN_PROP_MERGEINFO : '/A/B:4\n/A2.1/B:7-8'}),
+ '' : Item(props={SVN_PROP_MERGEINFO : '/A2.1:7-8\nA:3'}),
+ 'B' : Item(props={SVN_PROP_MERGEINFO : '/A2.1/B:7-8\nA/B:4'}),
'mu' : Item("New A2.1 stuff"),
'B/E' : Item(),
'B/E/alpha' : Item("This is the file 'alpha'.\n"),
]]]
That does fail on 1.6.6, better replicating the error Hyrum
encountered, but it fails simply because the self-referential
mergeinfo is never considered. As I was saying in IRC this morning
this is a case where one bug (rel pathed mergeinfo not handled
correctly by svn_mergeinfo_* APIs) allows us to avoid a different bug
(reintegrate code doesn't tolerate self-referential mergeinfo in some
cases). In most cases the first bug is going to cause problems, not
solve them.
> svn merge --reintegrate ^/subversion/branches/1.6.x-r40...@891008
>
> the 1.6.x client gives an error:
>
> svn: '/repos/asf/!svn/bc/875961/subversion/branches/1.6.x' path not found
>
> and the 1.6.4 client doesn't:
>
> --- Merging differences between repository URLs into '../src-1.6':
> G ../src-1.6/CHANGES
> G ../src-1.6
That is interesting about 1.6.4, I'll have to look into what changed
from 1.6.4 to 1.6.6 that would account for this.
> As I stated in a mail yesterday, the bug on the Subversion repository
> is triggered by the mergeinfo paths not having a leading '/' in
> addition to the other bogus data. It doesn't happen with the old
> collab repository where the paths do have a leading '/'.
Regarding what you found with the old repos: The revision offset
between our old repos revs and the ASF repos is 840074 right?
So shouldn't the mergeinfo on this path in the old repos,
>svn pg svn:mergeinfo
>https://svn.collab.net/repos/svn/branches/1.6.x-r40452/chan...@40513
...
/trunk/CHANGES:2-1281,35888-40052,40088,40152,40451-40452
differ from from the new repos,
>svn pg svn:mergeinfo
>https://svn.apache.org/repos/asf/subversion/branches/1.6.x-r40452/chan...@880587
...
subversion/trunk/CHANGES:836421-837700,872307-876471,876507,876571,880525-880526
only by the offset? The fact that it doesn't is troubling, to say the
least, and might also explain why you got different results with the
old repos.
> Merging r892085 to 1.6.x produces a client that does the merge, so I
> suppose we could simply approve r892085 and then there is definitely
> no regression.
That is probably the best option at this point.
Paul
