Stefan Fuhrmann wrote on Sun, Dec 05, 2010 at 23:45:50 +0100:
> On 01.12.2010 14:16, Daniel Shahaf wrote:
>> Julian Foad wrote on Wed, Dec 01, 2010 at 12:32:45 +0000:
>>> On Wed, 2010-12-01, stef...@apache.org wrote:
>>>> Port (not merge) a fix for a FSFS packing race condition from the
>>>> performance branch to /trunk: There is a slight time window
>>>> between finding the name of a rev file and actually open it. If
>>>> the revision in question gets packed just within this window,
>>>> we will find the new (and final) file name with just one retry.
>>>>
>>>> * subversion/libsvn_fs_fs/fs_fs.c
>>>> (open_pack_or_rev_file): retry once upon "missing file" error
>>> Hi Stefan.
>>>
>>> (1) I think the doc string of svn_fs_fs__path_rev_absolute() should note
>>> that the returned path may no longer be valid by the time you come to
>>> use it, and the reason for that. Does my patch below sound right?
>>>
>> Well, yes, and Stefan already added such a comment at the definition at
>> my suggestion. But +1 to move that to the declaration.
>>
>
>>> Patch for (1) above:
>>> [[[
>>> Index: subversion/libsvn_fs_fs/fs_fs.h
>>> ===================================================================
>>> --- subversion/libsvn_fs_fs/fs_fs.h (revision 1040662)
>>> +++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
>>> @@ -404,13 +404,16 @@ svn_error_t *svn_fs_fs__txn_changes_fetc
>>> PERMS_REFERENCE. Temporary allocations are from POOL. */
>>> svn_error_t *svn_fs_fs__move_into_place(const char *old_filename,
>>> const char *new_filename,
>>> const char *perms_reference,
>>> apr_pool_t *pool);
>>>
>>> -/* Sets *PATH to the path of REV in FS, whether in a pack file or not.
>>> +/* Set *PATH to the path of REV in FS, whether in a pack file or not.
>>> + The path is not guaranteed to remain correct after the function returns,
>>> + because it is possible that the revision will become packed just after
>>> + this call, so the caller should re-try once if the path is not found.
>>> Allocate in POOL. */
>> Sounds right.
>>
>> Bordering on bikeshed, I would suggest some changes:
>>
>> * Separate describing what the function does ("Sets *PATH to FOO and
>> allocates in POOL") from describing the conditions the caller should
>> beware of / check for ("sometimes the return value will be out-of-date
>> by the time you receive it").
>>
>> * Mention that the non-guarantee is not in effect if the caller has the
>> write lock.
>>
> I took the comment from the performance branch and added the
> info about guarantees in presence of a write lock in r1042479.
>
Thanks.
However, Julian already fixed this in fs_fs.h. Normally we keep the doc
strings only at the declaration of a function, and have no comments (or
only comments aimed at people patching the function itself --- but not
at people who call the function) at the definition.
> -- Stefan^2.
