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. - Julian
