Hi Philip. I hope you don't mind, I've made a few more review comments and suggestions, in the form of a patch.
[[[
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 1585474)
+++ subversion/include/svn_fs.h (working copy)
@@ -2512,8 +2512,9 @@ svn_fs_set_uuid(svn_fs_t *fs,
* expiration error (depending on the API).
*/
-/** The @a targets hash passed to svn_fs_lock_many() has <tt>const char
- * *</tt> keys and <tt>svn_fs_lock_target_t *</tt> values.
+/** Lock information for use with svn_fs_lock_many() [and svn_repos_fs_...].
+ *
+ * @see svn_fs_lock_target_create().
*
* @since New in 1.9.
*/
@@ -2522,6 +2523,9 @@ typedef struct svn_fs_lock_target_t svn_
/* Create an <tt>svn_fs_lock_target_t</tt> allocated in @a pool. @a
* token can be NULL and @a current_rev can be SVN_INVALID_REVNUM.
*
+ * The @a token is not duplicated and so must have a lifetime at least as
+ * long as the returned target object.
+ *
* @since New in 1.9.
**/
svn_fs_lock_target_t *svn_fs_lock_target_create(const char *token,
@@ -2530,20 +2534,27 @@ svn_fs_lock_target_t *svn_fs_lock_target
/* Update @a target changing the token to @a token, @a token can be NULL.
*
+ * The new @a token is not duplicated and so must have a lifetime at least
+ * as long as @a target.
+ *
* @since New in 1.9.
**/
void svn_fs_lock_target_set_token(svn_fs_lock_target_t *target,
const char *token);
-/** The callback invoked by svn_fs_lock_many() and svn_fs_unlock_many().
+/** ### Document here not what the callers will do but what this function
+ * itself is promised and what it is allowed/required to do.
*
- * @a path and @a lock are allocated in the result_pool passed to
- * svn_fs_lock_many/svn_fs_unlock_many and so will persist beyond the
- * callback invocation. @a fs_err will be cleared after the callback
- * returns, use svn_error_dup() to preserve the error.
- *
- * If the callback returns an error no further callbacks will be made
- * and svn_fs_lock_many/svn_fs_unlock_many will return an error.
+ * ### The callback invoked by svn_fs_lock_many() and svn_fs_unlock_many()
+ * ### [and svn_repos_fs_lock_many() and svn_repos_fs_lock_many() and others].
+ * ###
+ * ### @a path and @a lock are allocated in the result_pool passed to
+ * ### svn_fs_lock_many/svn_fs_unlock_many [and the others?] and so will persist beyond the
+ * ### callback invocation. @a fs_err will be cleared after the callback
+ * ### returns, use svn_error_dup() to preserve the error.
+ * ###
+ * ### If the callback returns an error no further callbacks will be made
+ * ### and svn_fs_lock_many/svn_fs_unlock_many will return an error.
*/
typedef svn_error_t *(*svn_fs_lock_callback_t)(void *baton,
const char *path,
@@ -2553,9 +2564,6 @@ typedef svn_error_t *(*svn_fs_lock_callb
/** Lock the paths in @a targets in @a fs.
*
- * @warning You may prefer to use svn_repos_fs_lock_many() instead,
- * which see.
- *
* @a fs must have a username associated with it (see
* #svn_fs_access_t), else return #SVN_ERR_FS_NO_USER. Set the
* 'owner' field in each new lock to the fs username.
@@ -2593,12 +2601,19 @@ typedef svn_error_t *(*svn_fs_lock_callb
* For each path in @a targets @a lock_callback will be invoked
* passing @a lock_baton and the lock and error that apply to path.
* @a lock_callback can be NULL in which case it is not called.
+ * ### Implementation here requires lock_callback is non-null.
+ *
+ * If the callback returns an error no further callbacks will be made
+ * and svn_fs_lock_many will return that error.
*
* The lock and path passed to @a lock_callback will be allocated in
* @a result_pool. Use @a scratch_pool for temporary allocations.
*
* @note At this time, only files can be locked.
*
+ * @note You probably don't want to use this directly. Take a look at
+ * svn_repos_fs_lock_many() instead.
+ *
* @since New in 1.9.
*/
svn_error_t *
@@ -2665,13 +2680,20 @@ svn_fs_generate_lock_token(const char **
* passed to the callback will be NULL. @a lock_callback can be NULL
* in which case it is not called.
*
+ * If the callback returns an error no further callbacks will be made
+ * and svn_fs_unlock_many will return that error.
+ *
* @note #svn_fs_lock_target_t is used to allow @c NULL tokens to be
* passed (it is not possible to pass @c NULL as a hash value
* directly), #svn_fs_lock_target_t->current_rev is ignored.
+ * ### No, svn_fs_lock_target_t is not used here.
*
* The path passed to lock_callback will be allocated in @a result_pool.
* Use @a scratch_pool for temporary allocations.
*
+ * @note You probably don't want to use this directly. Take a look at
+ * svn_repos_fs_unlock_many() instead.
+ *
* @since New in 1.9.
*/
svn_error_t *
]]]
The hunks in svn_repos.h are making the doc strings clearer by removing partial documentation of stuff that's already documented in the referenced FS-layer function. (The docs strings start with "Like svn_fs_...(), but ...".)
[[[
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 1585474)
+++ subversion/include/svn_repos.h (working copy)
@@ -2191,10 +2191,6 @@ svn_repos_fs_begin_txn_for_update(svn_fs
* which the pre-lock is successful are passed to svn_fs_lock_many and
* the post-lock is run for those that are successfully locked.
*
- * For each path in @a targets @a lock_callback will be invoked
- * passing @a lock_baton and the lock and error that apply to path.
- * @a lock_callback can be NULL in which case it is not called.
- *
* If an error occurs when running the post-lock hook the error is
* returned wrapped with #SVN_ERR_REPOS_POST_LOCK_HOOK_FAILED. If the
* caller sees this error, it knows that some locks succeeded.
@@ -2203,9 +2199,6 @@ svn_repos_fs_begin_txn_for_update(svn_fs
* lock, instead of the token supplied; see the pre-lock-hook
* documentation for more.
*
- * The lock and path passed to @a lock_callback will be allocated in
- * @a result_pool. Use @a scratch_pool for temporary allocations.
- *
* @since New in 1.9.
*/
svn_error_t *
@@ -2245,19 +2238,11 @@ svn_repos_fs_lock(svn_lock_t **lock,
* svn_fs_unlock_many and the post-unlock is run for those that are
* successfully unlocked.
*
- * For each path in @a targets @a lock_callback will be invoked
- * passing @a lock_baton and error that apply to path. The lock
- * passed to the callback will be NULL. @a lock_callback can be NULL
- * in which case it is not called.
- *
* If an error occurs when running the post-unlock hook, return the
* original error wrapped with #SVN_ERR_REPOS_POST_UNLOCK_HOOK_FAILED.
* If the caller sees this error, it knows that some unlocks
* succeeded.
*
- * The path passed to @a lock_callback will be allocated in @a result_pool.
- * Use @a scratch_pool for temporary allocations.
- *
* @since New in 1.9.
*/
svn_error_t *
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 1585474)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -1685,6 +1685,12 @@ svn_fs_lock_many(svn_fs_t *fs,
baton.lock_baton = lock_baton;
baton.cb_err = cb_err;
+ /* ### Why the callback wrapper? If fs->vtable->lock implements the
+ callback semantics as documented (the documentation is on the
+ public API) then when the passed-in callback returns an error
+ it should itself skip any further calls-back and return that
+ error. In which case all this wrapping seems moot/redundant.
+ */
err = fs->vtable->lock(fs, ok_targets, comment, is_dav_comment,
expiration_date, steal_lock,
lock_many_cb, &baton,
Index: subversion/libsvn_fs_base/lock.c
===================================================================
--- subversion/libsvn_fs_base/lock.c (revision 1585474)
+++ subversion/libsvn_fs_base/lock.c (working copy)
@@ -375,6 +375,7 @@ svn_fs_base__unlock(svn_fs_t *fs,
svn_error_clear(err);
}
+ /* ### leaks cb_err */
return SVN_NO_ERROR;
}
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 1585474)
+++ subversion/svnserve/serve.c (working copy)
@@ -2780,7 +2780,7 @@ static svn_error_t *lock_many(svn_ra_svn
an error. */
SVN_ERR(must_have_access(conn, pool, b, svn_authz_write, NULL, TRUE));
- /* Loop through the lock requests. */
+ /* Parse PATH_REVS into TARGETS. */
for (i = 0; i < path_revs->nelts; ++i)
{
const char *path, *full_path;
@@ -2803,7 +2803,10 @@ static svn_error_t *lock_many(svn_ra_svn
pool);
target = svn_fs_lock_target_create(NULL, current_rev, pool);
- /* We could check for duplicate paths and reject the request? */
+ /* If it's a duplicate path, once canonicalized, we simply overwrite
+ the previous entry so we end up ignoring all but one of the
+ duplicate requests. We considered checking for duplicate paths
+ and rejecting the request, but decided not to. */
svn_hash_sets(targets, full_path, target);
}
@@ -2812,6 +2815,8 @@ static svn_error_t *lock_many(svn_ra_svn
/* From here on we need to make sure any errors in authz_results, or
results, are cleared before returning from this function. */
+
+ /* Check authz access for each target. */
for (hi = apr_hash_first(pool, targets); hi; hi = apr_hash_next(hi))
{
const char *full_path = svn__apr_hash_index_key(hi);
@@ -2839,8 +2844,10 @@ static svn_error_t *lock_many(svn_ra_svn
0, /* No expiration time. */
steal_lock, lock_many_cb, &lmb,
pool, subpool);
+ SVN_ERR_ASSERT(! err); /* ### Never triggered: implies test suite doesn't exercise this err condition, last time I checked. Not suggesting to commit this assertion. */
- /* The client expects results in the same order as paths were supplied. */
+ /* Send the results. The client expects results in the same order as
+ paths were supplied. */
for (i = 0; i < path_revs->nelts; ++i)
{
const char *path, *full_path;
]]]
- Julian
Received on 2014-04-07 19:57:55 CEST