Philip Martin wrote:
> Julian Foad writes:
>>> URL: http://svn.apache.org/r1577280
>>> * subversion/include/svn_fs.h
>>> (svn_fs_lock_target_t, svn_fs_lock_result_t,
>>> svn_fs_lock2, svn_fs_unlock2): new.
>>>
>>> * subversion/include/svn_repos.h
>>> (svn_repos_fs_lock2, svn_repos_fs_unlock2): new.
>>
>> Do we intend to deprecate the old versions? If not, then it would be
>> better to name the new functions something like ...lock_many() instead
>> of ...lock2().
>
> I'm unsure. In some ways it is convenient to have a single path
> function as it avoids the need to construct hashes and the error
> handling is simpler and less prone to leak. On the other hand when
> svn_client_copy4 introduced multi-path copy source the single path
> version was deprecated.
Thoughts...
A low level API such as libsvn_fs should aim to be clean and efficient, more so than to provide functions that are convenient for the casual user. (A high level API such as libsvn_client or the bindings is a more appropriate place to provide convenience functions.) We do have a number of APIs already, in several libraries, where the caller is expected to pass in a list of targets even if they only want to operate on one. Maybe in general the thing to aim for is to make it easy for a caller to do so.
We could provide a singleton constructor for the hash-of-svn_fs_lock_target_t argument. Should we? Probably not at the libsvn_fs level; maybe at a high level.
I started reading further through the new code. Here are some more review comments.
The 'comment' parameter to svn_fs_lock2 should be a member of the 'target' struct, since it is inherently a per-target attribute. Of course there are common scenarios where a client wants to lock a bunch of paths all with the same comment, but the benefits of a lock-many API should also be made available to clients which supply different comments. This applies all the way up the call stack. However, I note that from the RA layer upwards we have had lock-many APIs since v1.2 which take a single comment for all the paths, so changing to per-target comments throughout the stack is perhaps beyond the scope of this issue. We could still do it at the FS and repos layers now in preparation; I don't see that it would add significant overhead in run-time or in maintenance.
In principle, the 'is_dav_comment' and 'expiration_date' parameters should similarly be per target, but that makes less sense in practice as they're only used for generic DAV clients via mod_dav_svn. As an alternative, we might consider dropping them from this API and keeping the original single-lock API (not deprecated) to cater for such locks. RA-local and svnserve and 'svnadmin lock' don't support expiration and always pass is_dav_comment=FALSE. Only mod_dav_svn supplies these two options, and it currently only uses the old one-lock-at-a-time API anyway.
We should provide a destructor for the hash-of-svn_fs_lock_result_t results, which contains errors that need to be cleared. svnserve's lock_many() already contains such code twice. It's simple code -- around 5 lines -- but if we're demanding that callers ensure they do it, it's nice to provide a ready-made way for them to do it. This would also help in future-proofing against us revising the API in later releases.
The semantics relating to returning one error per path and an overall error needs to be fully documented and/or changed. For example, svn_repos_fs_lock() assumes that svn_repos_fs_lock2() has set *results to a valid hash if it returns any overall error, while svn_fs_lock() assumes that svn_fs_lock2() WON'T have put any error in *results if it returns an error overall. These look at least surprising.
And some more suggestions and comments in the form of a diff:
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 1582241)
+++ subversion/include/svn_fs.h (working copy)
@@ -2545,7 +2545,7 @@
*
* @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 the new lock to the fs username.
+ * 'owner' field in each new lock to the fs username.
*
* @a comment is optional: it's either an xml-escapable UTF8 string
* which describes the lock, or it is @c NULL.
@@ -2566,9 +2566,10 @@
* that existing lock. If current_rev is a valid revnum, then do an
* out-of-dateness check. If the revnum is less than the
* last-changed-revision of the path (or if the path doesn't exist in
- * HEAD), return * #SVN_ERR_FS_OUT_OF_DATE.
+ * HEAD), yield an #SVN_ERR_FS_OUT_OF_DATE error for this path.
*
- * If a path is already locked, then return #SVN_ERR_FS_PATH_ALREADY_LOCKED,
+ * If a path is already locked, then yield an
+ * #SVN_ERR_FS_PATH_ALREADY_LOCKED error for this path,
* unless @a steal_lock is TRUE, in which case "steal" the existing
* lock, even if the FS access-context's username does not match the
* current lock's owner: delete the existing lock on the path, and
@@ -2582,7 +2583,7 @@
* <tt>svn_fs_lock_result_t *</tt>. The error associated with each
* path is returned as #svn_fs_lock_result_t->err. The caller must
* ensure that all such errors are handled to avoid leaks. The lock
- * associated with each path is returned as #svn_fs_lock_result_t->lock,
+ * associated with each path is returned as #svn_fs_lock_result_t->lock;
* this will be @c NULL if no lock was created.
*
* Allocate @a *results in @a result_pool. Use @a scratch_pool for
@@ -2637,17 +2638,19 @@
* the results in @a *results.
*
* The paths to be unlocked are passed as <tt>const char *</tt> keys
- * of the @a targets hash with <tt>svn_fs_lock_target_t *</tt> values.
- * #svn_fs_lock_target_t->token provides the token to be unlocked for
- * each path. If the the token doesn't point to a lock, return
- * #SVN_ERR_FS_BAD_LOCK_TOKEN. If the token points to an expired
- * lock, return #SVN_ERR_FS_LOCK_EXPIRED. If @a fs has no username
- * associated with it, return #SVN_ERR_FS_NO_USER unless @a break_lock
- * is specified.
+ * of the @a targets hash with the corresponding lock tokens as the
+ * <tt>const char *</tt> values. If the the token doesn't point to a
+ * lock, yield an #SVN_ERR_FS_BAD_LOCK_TOKEN error for this path. If
+ * the token points to an expired lock, yield an #SVN_ERR_FS_LOCK_EXPIRED
+ * error for this path.
+ *
+ * If @a fs has no username associated with it, return #SVN_ERR_FS_NO_USER
+ * unless @a break_lock is specified.
+ * ### Or: "yield an #SVN_ERR_FS_NO_USER error for this path unless..."?
*
* If the token points to a lock, but the username of @a fs's access
- * context doesn't match the lock's owner, return
- * #SVN_ERR_FS_LOCK_OWNER_MISMATCH. If @a break_lock is TRUE, however, don't
+ * context doesn't match the lock's owner, yield an
+ * #SVN_ERR_FS_LOCK_OWNER_MISMATCH error for this path. If @a break_lock is TRUE, however, don't
* return error; allow the lock to be "broken" in any case. In the latter
* case, the token shall be @c NULL.
*
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 1582241)
+++ subversion/include/svn_repos.h (working copy)
@@ -2187,21 +2187,20 @@
/** Like svn_fs_lock2(), but invoke the @a repos's pre- and
* post-lock hooks before and after the locking action.
*
- * The pre-lock is run for every path in @a targets. Those entries in
- * @a targets for which the pre-lock is successful are passed to
- * svn_fs_lock2 and the post-lock is run for those that are
+ * The pre-lock hook is run for every path in @a targets. Those
+ * targets for which the pre-lock hook is successful are passed to
+ * svn_fs_lock2() and the post-lock hook is run for those that are
* successfully locked.
*
- * @a results contains the result of running the pre-lock and
- * svn_fs_lock2 if the pre-lock was successful. 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 the some locks succeeded. In all cases the
- * caller must handle all errors in @a results to avoid leaks.
+ * @a *results contains the result for each path. If an error occurs
+ * when running the post-lock hook, the error is wrapped with
+ * #SVN_ERR_REPOS_POST_LOCK_HOOK_FAILED. If the caller sees this
+ * error, it knows that this lock succeeded. In all cases the caller
+ * must handle or clear all errors in @a *results to avoid leaks.
*
* The pre-lock hook may cause a different token to be used for the
- * lock, instead of @a token; see the pre-lock-hook documentation for
- * more.
+ * lock, instead of the token supplied here; see the pre-lock-hook
+ * documentation for more.
*
* Allocate @a *results in @a result_pool. Use @a scratch_pool for
* temporary allocations.
@@ -2236,21 +2235,19 @@
apr_pool_t *pool);
-/** Like svn_fs_unlock(), but invoke the @a repos's pre- and
+/** Like svn_fs_unlock2(), but invoke the @a repos's pre- and
* post-unlock hooks before and after the unlocking action.
*
* The pre-unlock hook is run for every path in @a targets. Those
- * entries in @a targets for which the pre-unlock is successful are
- * passed to svn_fs_unlock2 and the post-unlock is run for those that
- * are successfully unlocked.
- *
- * @a results contains the result of of running the pre-unlock and
- * svn_fs_unlock2 if the pre-unlock was successful. 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. In all
- * cases the caller must handle all error in @a results to avoid
- * leaks.
+ * targets for which the pre-unlock hook is successful are passed to
+ * svn_fs_unlock2() and the post-unlock hook is run for those that are
+ * successfully unlocked.
+ *
+ * @a *results contains the result for each path. If an error occurs
+ * when running the post-unlock hook, the error is wrapped with
+ * #SVN_ERR_REPOS_POST_UNLOCK_HOOK_FAILED. If the caller sees this
+ * error, it knows that this unlock succeeded. In all cases the caller
+ * must handle or clear all errors in @a *results to avoid leaks.
*
* Allocate @a *results in @a result_pool. Use @a scratch_pool for
* temporary allocations.
@@ -2265,7 +2262,7 @@
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-/* Similar to svn_repos_fs_unlock2 but only unlocks a single path.
+/* Similar to svn_repos_fs_unlock2() but only unlocks a single path.
*
* @since New in 1.2.
*/
Index: subversion/libsvn_repos/fs-wrap.c
===================================================================
--- subversion/libsvn_repos/fs-wrap.c (revision 1582225)
+++ subversion/libsvn_repos/fs-wrap.c (working copy)
@@ -570,6 +570,10 @@ svn_repos_fs_lock2(apr_hash_t **results,
svn__apr_hash_index_val(hi));
/* If there are locks and an error should we return or run the post-lock? */
+ /* [JAF] Is that a question to reviewers? It depends. What errors can
+ svn_fs_lock2 return, and do any of them justify not running the
+ post-lock hook? Can it even return an error and also create
+ locks? Its doc string needs to say. */
if (err)
return svn_error_trace(err);
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 1582225)
+++ subversion/svnserve/serve.c (working copy)
@@ -2733,7 +2733,8 @@ 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. */
+ /* Loop through the lock requests
+ ### and do what? */
for (i = 0; i < path_revs->nelts; ++i)
{
const char *path, *full_path;
@@ -2759,6 +2760,8 @@ static svn_error_t *lock_many(svn_ra_svn
target->current_rev = current_rev;
/* We could check for duplicate paths and reject the request? */
+ /* ### [JAF] Is that a question to reviewers? We could, but I
+ don't think it's useful to do so. */
svn_hash_sets(targets, full_path, target);
}
@@ -2767,6 +2770,11 @@ 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, because ...
+ ### Why? We don't want svn_repos_fs_lock2() to do this for us ...?
+ Or, we want to log such errors and it's easier to do so before
+ than afterwards? */
for (hi = apr_hash_first(pool, targets); hi; hi = apr_hash_next(hi))
{
const char *full_path = svn__apr_hash_index_key(hi);
@@ -2791,7 +2799,8 @@ static svn_error_t *lock_many(svn_ra_svn
0, /* No expiration time. */
steal_lock, pool, subpool);
- /* 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;
@@ -2816,6 +2825,16 @@ static svn_error_t *lock_many(svn_ra_svn
result = svn_hash_gets(authz_results, full_path);
if (!result)
/* No result? Should we return some sort of placeholder error? */
+ /* ### [JAF] Is that a question to reviewers? Certainly something
+ has gone wrong. Maybe svn_repos_fs_lock2 returned an error
+ overall, having processed none or only some paths -- is it
+ allowed to do so? Breaking here would signal to the client that
+ something went wrong, because they'll see a too-short response
+ list, but would also potentially hide information about further
+ targets that were processed, some of which perhaps were locked
+ successfully. (Note: we're scanning them here in a different
+ order from the order in which svn_repos_fs_lock2() processed
+ them.) */
break;
if (result->err)
- Julian
Received on 2014-03-27 14:17:45 CET