Hi Mike.
I reviewed r939375,r939376 proposed for back-port to 1.6.x, to fix issue
#3623 "Files added via merge from foreign repositories lose properties"
<http://subversion.tigris.org/issues/show_bug.cgi?id=3623>.
The fix looks fine and works properly, and I'll approve it.
Just a small concern about r939375 which extends the existing test
function (merge_tests 88 on 1.6.x, merge_tests 72 on current trunk). It
looks like it unintentionally removes a check. See the "-" line in the
diff below?
[[[
$ command svn log --diff ^/subversion/trunk -c939375
------------------------------------------------------------------------
r939375 | cmpilato | 2010-04-29 17:47:55 +0100 (Thu, 29 Apr 2010) | 7 lines
Extend a test to address the concerns of issue #3623.
* subversion/tests/cmdline/merge_tests.py
(foreign_repos): Extend this test a bit to really verify that what
was merged from a foreign repos, and committed, is *really* what we
expected.
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
[...]
# repository. Not only should the merge succeed, but the results on
# disk should match those in our first working copy.
### TODO: Use run_and_verify_merge() ###
svntest.main.run_svn(None, 'merge', '-c2', sbox.repo_url, wc_dir2)
svntest.main.run_svn(None, 'ci', '-m', 'Merge from foreign repos', wc_dir2)
- svntest.actions.verify_disk(wc_dir2, expected_disk, True)
+
+ # Now, let's make a third checkout -- our second from the original
+ # repository -- and make sure that all the data there is correct.
+ # It should look just like the original EXPECTED_DISK.
+ wc_dir3 = sbox.add_wc_path('wc3')
+ svntest.actions.run_and_verify_svn(None, None, [], 'checkout',
+ sbox2.repo_url, wc_dir3)
+ svntest.actions.verify_disk(wc_dir3, expected_disk, True)
]]]
Was that intended?
While I'm there, I would suggest that when we test a pair of files (or
dirs), we should test one with props and one without, rather than both
with props. So, instead of expected_disk containing:
'Q' : Item(props={'foo':'bar'}),
'A/D/G/Z' : Item(props={'foo':'bar'}),
'A/D/G/Z/zeta' : Item(contents=zeta_contents,props={'foo':'bar'}),
'A/C/fred' : Item(contents=fred_contents,props={'foo':'bar'}),
it would contain
'Q' : Item(),
'A/D/G/Z' : Item(props={'foo':'bar'}),
'A/D/G/Z/zeta' : Item(contents=zeta_contents),
'A/C/fred' : Item(contents=fred_contents,props={'foo':'bar'}),
If that all sounds OK, I'll re-instate the "-" line above and remove the
props from one file and one dir, like this, in the trunk version of the
test. I don't think we need to back-port these two tweaks.
(I tested 1.6.x locally with those tweaks and it passed.)
- Julian
Received on 2010-06-18 12:31:00 CEST