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

Issue #3623, foreign-repo merge loses props on added file

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Fri, 18 Jun 2010 11:30:15 +0100

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

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