On 04.08.2010 12:14, Julian Foad wrote: > On Tue, 2010-08-03 at 20:08 +0300, Daniel Shahaf wrote: > >> Julian Foad wrote on Tue, Aug 03, 2010 at 17:36:00 +0100: >> >>> On Tue, 2010-08-03 at 18:19 +0300, Daniel Shahaf wrote: >>> >>>> Daniel Shahaf wrote on Tue, Aug 03, 2010 at 11:58:32 +0300: >>>> >>>>> There was yesterday on IRC [1] some discussion around whether revprop >>>>> editing can corrupt rev files. >>>>> >>>>> [[[ >>>>> 0:% rm -rf r >>>>> 0:% ./subversion/svnadmin/svnadmin create r >>>>> 0:% ls -l r/db/revs/0/0 >>>>> -rw-r--r-- 1 daniel daniel 115 2010-08-03 11:56 r/db/revs/0/0 >>>>> ]]] >>>>> >>>>> Shouldn't we create rev files with the write bit disabled? (i.e., 0222 >>>>> umask) >>>>> >>>> r981921 + nominated for backport. >>>> >>> The idea looks sane. >>> >>> There's perhaps a problem with revPROPS files: fs_fs.c copies the >>> permissions of a revprops file from the corresponding rev file, which >>> now means all revprops files will be read-only, which might cause a >>> problem when trying to edit a revprop. >>> >>> >> Good catch. >> >> As far as I can tell, 'svnadmin setrevprop' and 'svn propset --revprop' >> still work and fs-test and fs-pack-test (which test svn_fs_change_rev_prop()) >> still pass (even though the revprop files are mode 0444), so I'm tempted to >> leave the code as-is. >> >> Does anyone see a problem here, or see an error when trying to edit revprops >> with this patch applied? >> > I didn't encounter a problem on Ubuntu, in a simple manual test (not > trying this within the test suite, this time). > > "svn propset --revprop -r <REV> ...": for a packed rev it modified > 'revprops.db', leaving that file read-write, and for a not-yet-packed > revision (r30) it modified the file 'revprops/3/30', leaving that file > read-only. > > "svnadmin pack" worked, and the pack files and manifests are read-only. >
I'm assuming "svnadmin pack" deletes obsolete revprop files. Read-only files can be deleted on Unix as long as the containing directory is read-write. This needs to be tested on Windows, where that is not the case. -- Brane
