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

[Julian Foad <ju...@wandisco.com>]

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

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

[Julian Foad <ju...@wandisco.com>]

C. Michael Pilato wrote:
> Julian Foad wrote:
> > 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?
> 
> That check was removed intentionally as a minor optimization.  While it's
> fine to verify the disk tree at the point the test, the whole reason I
> needed to extend the test was that successful verification at that stage
> wasn't enough to verify that the bug was fixed.  So I had the test do more
> work and delayed disk verification until later.  But it's fine with me if
> you want to reinstate it.

Seems worth keeping, as the test is not just for testing the issue #3623
problem, so committed in r955999.

- Julian

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

["C. Michael Pilato" <cm...@collab.net>]

Julian Foad wrote:
> 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?

That check was removed intentionally as a minor optimization.  While it's
fine to verify the disk tree at the point the test, the whole reason I
needed to extend the test was that successful verification at that stage
wasn't enough to verify that the bug was fixed.  So I had the test do more
work and delayed disk verification until later.  But it's fine with me if
you want to reinstate it.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand