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
Received on 2010-08-04 12:21:30 CEST