Stefan Sperling wrote on Sat, Oct 16, 2010 at 15:40:46 +0200:
> On Fri, Oct 15, 2010 at 10:13:48PM +0200, Paul Maier wrote:
> > svn cp should always make the target file read-write in the
> > working copy.
>
> No. A plain copy should carry file permissions of its source along,
> just like a normal (operating system) copy command does.
>
+1
> Only in the special case where an uncommitted file has an svn:needs-lock
> property it should be read-write, because:
> 1) There is no way to get the lock to make the file read-write,
> so there's no point in having the file read-only
> 2) Noone else can see the file yet, anyway
>
+1
> > Sounds like one command somewhere near the end of svn cp, isn't it?
>
> I'd poke around in the code to see where the svn:needs-lock property
> is handled, and try to add a special case there for files which are
> locally added or copied.
I looked at svn_wc__maybe_set_read_only(), discovered it wasn't even
called during 'svn cp', and came up with the following patch:
[[[
Index: subversion/tests/cmdline/lock_tests.py
===================================================================
--- subversion/tests/cmdline/lock_tests.py (revision 1023400)
+++ subversion/tests/cmdline/lock_tests.py (working copy)
@@ -37,6 +37,23 @@ XFail = svntest.testcase.XFail
Item = svntest.wc.StateItem
######################################################################
+# Helpers
+
+def check_writability(path, writable)
+ "Raise if PATH is not writable."
+ bits = stat.S_IWGRP | stat.S_IWOTH | stat.S_IWRITE
+ mode = os.stat(path)[0]
+ if bool(mode & bits) != writable:
+ raise svntest.Failure("path '%s' is unexpectedly %s (mode %o)"
+ % (path, ["writable", "read-only"][writable], mode))
+
+def is_writable(path):
+ check_writability(path, True)
+
+def is_readonly(path):
+ check_writability(path, False)
+
+######################################################################
# Tests
#----------------------------------------------------------------------
@@ -1573,6 +1590,39 @@ def replace_and_propset_locked_path(sbox):
'commit', '-m', '', G_path)
+#----------------------------------------------------------------------
+def cp_isnt_ro(sbox):
+ "uncommitted svn:needs-lock add/cp not read-only"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ mu_path = os.path.join(wc_dir, 'A', 'mu')
+ mu2_path = os.path.join(wc_dir, 'A', 'mu2')
+ kappa_path = os.path.join(wc_dir, 'kappa')
+ open(kappa_path, 'w').write("This is the file 'kappa'.\n")
+
+ # added file
+ sbox.simple_add(kappa_path)
+ svntest.actions.set_prop('svn:needs-lock', 'yes', kappa_path)
+ is_writable(kappa_path)
+ sbox.simple_commit(kappa_path)
+ is_readonly(kappa_path)
+
+ # versioned file
+ svntest.actions.set_prop('svn:needs-lock', 'yes', mu_path)
+ is_writable(mu_path)
+ sbox.simple_commit(mu_path)
+ is_readonly(mu_path)
+
+ # added-with-history file
+ svntest.actions.set_prop('svn:needs-lock', 'yes', mu_path)
+ svntest.main.run_svn(None, 'copy', mu_path, mu2_path)
+ is_writable(mu2_path)
+ sbox.simple_commit(mu2_path)
+ is_readonly(mu2_path)
+
+
########################################################################
# Run the tests
@@ -1619,6 +1669,7 @@ test_list = [ None,
verify_path_escaping,
XFail(replace_and_propset_locked_path,
svntest.main.is_ra_type_dav),
+ cp_isnt_ro,
]
if __name__ == '__main__':
Index: subversion/libsvn_wc/copy.c
===================================================================
--- subversion/libsvn_wc/copy.c (revision 1023400)
+++ subversion/libsvn_wc/copy.c (working copy)
@@ -238,6 +238,17 @@ copy_versioned_file(svn_wc__db_t *db,
tmpdir_abspath,
TRUE, /* recursive */
cancel_func, cancel_baton, scratch_pool));
+
+ /* Remove 'read-only' from the copied file. */
+ {
+ const svn_string_t *needs_lock;
+ SVN_ERR(svn_wc__internal_propget(&needs_lock, db, src_abspath,
+ SVN_PROP_NEEDS_LOCK, scratch_pool,
+ scratch_pool));
+ if (needs_lock)
+ svn_io_set_file_read_write(tmp_dst_abspath, FALSE, scratch_pool);
+ }
+
if (tmp_dst_abspath)
{
svn_skel_t *work_item;
]]]
Please review the regression test :-)
Thanks,
Daniel
