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.
-- Stefan^2.
