You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2014/03/13 19:47:18 UTC
svn commit: r1577280 [1/3] - in /subversion/trunk: ./ notes/
subversion/include/ subversion/include/private/ subversion/libsvn_client/
subversion/libsvn_fs/ subversion/libsvn_fs_base/ subversion/libsvn_fs_fs/
subversion/libsvn_fs_x/ subversion/libsvn_r...
Author: philip
Date: Thu Mar 13 18:47:17 2014
New Revision: 1577280
URL: http://svn.apache.org/r1577280
Log:
Merge the fsfs-lock-many branch to trunk. This introduce multiple
path lock and unlock at the FS level and connects it up over ra_local
and ra_svn, and also uses it post-commit to release locks. A summary
of the branch changes:
* 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.
* subversion/include/svn_error.h
(SVN_ERR_IS_LOCK_ERROR, SVN_ERR_IS_UNLOCK_ERROR): Add pre- hook errors.
* subversion/libsvn_fs/fs-loader.c
(svn_fs_lock2, svn_fs_unlock2): New.
* subversion/libsvn_fs/fs-loader.h
(fs_vtable_t): Change lock and unlock parameters.
* subversion/libsvn_fs_fs/lock.c
* subversion/libsvn_fs_x/lock.c
(svn_fs_fs__lock, svn_fs_fs__unlock): New multiple path implementation.
* subversion/libsvn_fs_base/lock.c
(svn_fs_base__lock, svn_fs_base__unlock): Iterate over paths.
* subversion/libsvn_fs_fs/lock.h
(svn_fs_fs__lock, svn_fs_fs__unlock): Change parameters.
* subversion/libsvn_fs_x/lock.h
(svn_fs_x__lock, svn_fs_x__unlock): Change parameters.
* subversion/libsvn_fs_base/lock.h
(svn_fs_base__lock, svn_fs_base__unlock): Change parameters.
* subversion/libsvn_repos/fs-wrap.c
(svn_repos_fs_lock2, svn_repos_fs_unlock2): New.
* subversion/libsvn_repos/repos.c
(SVN_REPOS__HOOK_POST_LOCK, SVN_REPOS__HOOK_POST_UNLOCK): Tweak hook
template text.
* subversion/libsvn_ra_local/ra_plugin.c
(deltify_etc, svn_ra_local__lock, svn_ra_local__unlock): Use new API.
* subversion/mod_dav_svn/version.c
(release_locks): Use new API.
* subversion/svnserve/serve.c
(unlock_paths, lock_many, unlock_many): Use new API.
* subversion/libsvn_subr/log.c
* subversion/libsvn_fs_fs/lock.h
(svn_fs_fs__lock, svn_fs_fs__unlock,
svn_log__lock_one_path, svn_log__unlock_one_path): Change parameter.
* subversion/libsvn_client/locking_commands.c
(store_locks_callback): Tweak error handling.
* subversion/tests/libsvn_fs/locks-test.c
(lock_multiple_paths): New test.
* subversion/tests/libsvn_ra/ra-test.c
(lock_test): New test.
* subversion/tests/cmdline/authz_tests.py
(authz_locking): Tweak expectations.
* subversion/tests/cmdline/lock_tests.py
(block_unlock_if_pre_unlock_hook_fails, lock_hook_messages): Tweak
expectations.
(failing_post_hooks): New.
Modified:
subversion/trunk/ (props changed)
subversion/trunk/notes/ (props changed)
subversion/trunk/subversion/include/private/svn_log.h
subversion/trunk/subversion/include/svn_error.h
subversion/trunk/subversion/include/svn_fs.h
subversion/trunk/subversion/include/svn_repos.h
subversion/trunk/subversion/libsvn_client/locking_commands.c
subversion/trunk/subversion/libsvn_fs/fs-loader.c
subversion/trunk/subversion/libsvn_fs/fs-loader.h
subversion/trunk/subversion/libsvn_fs_base/lock.c
subversion/trunk/subversion/libsvn_fs_base/lock.h
subversion/trunk/subversion/libsvn_fs_fs/lock.c
subversion/trunk/subversion/libsvn_fs_fs/lock.h
subversion/trunk/subversion/libsvn_fs_x/ (props changed)
subversion/trunk/subversion/libsvn_fs_x/lock.c
subversion/trunk/subversion/libsvn_fs_x/lock.h
subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
subversion/trunk/subversion/libsvn_repos/fs-wrap.c
subversion/trunk/subversion/libsvn_repos/repos.c
subversion/trunk/subversion/libsvn_subr/log.c
subversion/trunk/subversion/mod_dav_svn/version.c
subversion/trunk/subversion/svnserve/serve.c
subversion/trunk/subversion/tests/cmdline/authz_tests.py
subversion/trunk/subversion/tests/cmdline/lock_tests.py
subversion/trunk/subversion/tests/libsvn_fs/locks-test.c
subversion/trunk/subversion/tests/libsvn_fs_x/ (props changed)
subversion/trunk/subversion/tests/libsvn_ra/ra-test.c
Propchange: subversion/trunk/
------------------------------------------------------------------------------
Merged /subversion/branches/fsfs-lock-many:r1571740-1577217
Propchange: subversion/trunk/notes/
------------------------------------------------------------------------------
--- svn:mergeinfo (added)
+++ svn:mergeinfo Thu Mar 13 18:47:17 2014
@@ -0,0 +1,38 @@
+/subversion/branches/1.5.x-r30215/notes:870312
+/subversion/branches/bdb-reverse-deltas/notes:872050-872529
+/subversion/branches/diff-callbacks3/notes:870059-870761
+/subversion/branches/dont-save-plaintext-passwords-by-default/notes:870728-871118
+/subversion/branches/double-delete/notes:870511-872970
+/subversion/branches/explore-wc/notes:875486,875493,875497,875507,875511,875514,875559,875580-875581,875584,875587,875611,875627,875647,875667-875668,875711-875712,875733-875734,875736,875744-875748,875751,875758,875782,875795-875796,875830,875836,875838,875842,875852,875855,875864,875870,875873,875880,875885-875888,875890,875897-875898,875905,875907-875909,875935,875943-875944,875946,875979,875982-875983,875985-875986,875990,875997
+/subversion/branches/file-externals/notes:871779-873302
+/subversion/branches/fs-rep-sharing/notes:869036-873803
+/subversion/branches/fsfs-lock-many/notes:1571740-1577217
+/subversion/branches/fsfs-pack/notes:873717-874575
+/subversion/branches/gnome-keyring/notes:870558-871410
+/subversion/branches/http-protocol-v2/notes:874395-876041
+/subversion/branches/in-memory-cache/notes:869829-871452
+/subversion/branches/issue-2843-dev/notes:871432-874179
+/subversion/branches/issue-3000/notes:871713,871716-871719,871721-871726,871728,871734
+/subversion/branches/issue-3067-deleted-subtrees/notes:873375-874084
+/subversion/branches/issue-3148-dev/notes:875193-875204
+/subversion/branches/issue-3220-dev/notes:872210-872226
+/subversion/branches/issue-3242-dev/notes:879653-896436
+/subversion/branches/issue-3334-dirs/notes:875156-875867
+/subversion/branches/kwallet/notes:870785-871314
+/subversion/branches/log-g-performance/notes:870941-871032
+/subversion/branches/merge-skips-obstructions/notes:874525-874615
+/subversion/branches/nfc-nfd-aware-client/notes:870276,870376
+/subversion/branches/ra_serf-digest-authn/notes:875693-876404
+/subversion/branches/reintegrate-improvements/notes:873853-874164
+/subversion/branches/subtree-mergeinfo/notes:876734-878766
+/subversion/branches/svn-mergeinfo-enhancements/notes:870119-870195,870197-870288
+/subversion/branches/svn-patch-improvements/notes:918519-934609
+/subversion/branches/svnpatch-diff/notes:865738-876477
+/subversion/branches/svnraisetc/notes:874709-875149
+/subversion/branches/svnserve-logging/notes:869828-870893
+/subversion/branches/tc-issue-3334/notes:874697-874773
+/subversion/branches/tc-merge-notify/notes:874017-874062
+/subversion/branches/tc-resolve/notes:874191-874239
+/subversion/branches/tc_url_rev/notes:874351-874483
+/subversion/branches/tree-conflicts/notes:868291-873154
+/subversion/branches/tree-conflicts-notify/notes:873926-874008
Modified: subversion/trunk/subversion/include/private/svn_log.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_log.h?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_log.h (original)
+++ subversion/trunk/subversion/include/private/svn_log.h Thu Mar 13 18:47:17 2014
@@ -204,7 +204,7 @@ svn_log__get_file_revs(const char *path,
* @since New in 1.6.
*/
const char *
-svn_log__lock(const apr_array_header_t *paths, svn_boolean_t steal,
+svn_log__lock(apr_hash_t *targets, svn_boolean_t steal,
apr_pool_t *pool);
/**
@@ -213,7 +213,7 @@ svn_log__lock(const apr_array_header_t *
* @since New in 1.6.
*/
const char *
-svn_log__unlock(const apr_array_header_t *paths, svn_boolean_t break_lock,
+svn_log__unlock(apr_hash_t *targets, svn_boolean_t break_lock,
apr_pool_t *pool);
/**
Modified: subversion/trunk/subversion/include/svn_error.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_error.h?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_error.h (original)
+++ subversion/trunk/subversion/include/svn_error.h Thu Mar 13 18:47:17 2014
@@ -428,18 +428,23 @@ svn_error_t *svn_error_purge_tracing(svn
* SVN_ERR_FS_OUT_OF_DATE and SVN_ERR_FS_NOT_FOUND are in here because it's a
* non-fatal error that can be thrown when attempting to lock an item.
*
+ * SVN_ERR_REPOS_HOOK_FAILURE refers to the pre-lock hook.
+ *
* @since New in 1.2.
*/
#define SVN_ERR_IS_LOCK_ERROR(err) \
(err->apr_err == SVN_ERR_FS_PATH_ALREADY_LOCKED || \
err->apr_err == SVN_ERR_FS_NOT_FOUND || \
err->apr_err == SVN_ERR_FS_OUT_OF_DATE || \
- err->apr_err == SVN_ERR_FS_BAD_LOCK_TOKEN)
+ err->apr_err == SVN_ERR_FS_BAD_LOCK_TOKEN || \
+ err->apr_err == SVN_ERR_REPOS_HOOK_FAILURE)
/**
* Return TRUE if @a err is an error specifically related to unlocking
* a path in the repository, FALSE otherwise.
*
+ * SVN_ERR_REPOS_HOOK_FAILURE refers to the pre-unlock hook.
+ *
* @since New in 1.2.
*/
#define SVN_ERR_IS_UNLOCK_ERROR(err) \
@@ -448,7 +453,8 @@ svn_error_t *svn_error_purge_tracing(svn
err->apr_err == SVN_ERR_FS_LOCK_OWNER_MISMATCH || \
err->apr_err == SVN_ERR_FS_NO_SUCH_LOCK || \
err->apr_err == SVN_ERR_RA_NOT_LOCKED || \
- err->apr_err == SVN_ERR_FS_LOCK_EXPIRED)
+ err->apr_err == SVN_ERR_FS_LOCK_EXPIRED || \
+ err->apr_err == SVN_ERR_REPOS_HOOK_FAILURE)
/** Evaluates to @c TRUE iff @a apr_err (of type apr_status_t) is in the given
* @a category, which should be one of the @c SVN_ERR_*_CATEGORY_START
Modified: subversion/trunk/subversion/include/svn_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_fs.h?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_fs.h (original)
+++ subversion/trunk/subversion/include/svn_fs.h Thu Mar 13 18:47:17 2014
@@ -2512,11 +2512,30 @@ svn_fs_set_uuid(svn_fs_t *fs,
* expiration error (depending on the API).
*/
+/* The @a targets hash passed to svn_fs_lock2() has <tt>const char
+ *</tt> keys and <tt>svn_fs_lock_target_t *</tt> values. */
+typedef struct svn_fs_lock_target_t
+{
+ const char *token;
+ svn_revnum_t current_rev;
+
+} svn_fs_lock_target_t;
+
+/* The @a results hash returned by svn_fs_lock2() and svn_fs_unlock2()
+ has <tt>const char *</tt> keys and <tt>svn_fs_lock_result_t *</tt>
+ values. */
+typedef struct svn_fs_lock_result_t
+{
+ svn_lock_t *lock;
+ svn_error_t *err;
+
+} svn_fs_lock_result_t;
-/** Lock @a path in @a fs, and set @a *lock to a lock
- * representing the new lock, allocated in @a pool.
+/** Lock the paths in @a targets in @a fs, and set @a *results to the
+ * locks or errors representing each new lock, allocated in @a
+ * result_pool.
*
- * @warning You may prefer to use svn_repos_fs_lock() instead,
+ * @warning You may prefer to use svn_repos_fs_lock2() instead,
* which see.
*
* @a fs must have a username associated with it (see
@@ -2530,28 +2549,56 @@ svn_fs_set_uuid(svn_fs_t *fs,
* generic DAV client; only mod_dav_svn's autoversioning feature needs
* to use it. If in doubt, pass 0.
*
- * If path is already locked, then return #SVN_ERR_FS_PATH_ALREADY_LOCKED,
+ * The paths to be locked are passed as the <tt>const char *<tt> keys
+ * of the @a targets hash. The hash values are
+ * <tt>svn_fs_lock_target_t *</tt> and provide the token and
+ * current_rev for each path. The token is a lock token such as can
+ * be generated using svn_fs_generate_lock_token() (indicating that
+ * the caller wants to dictate the lock token used), or it is @c NULL
+ * (indicating that the caller wishes to have a new token generated by
+ * this function). If the token is not @c NULL, and represents an
+ * existing lock, then the path must match the path associated with
+ * 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.
+ *
+ * If a path is already locked, then return #SVN_ERR_FS_PATH_ALREADY_LOCKED,
* 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 @a path, and
+ * current lock's owner: delete the existing lock on the path, and
* create a new one.
*
- * @a token is a lock token such as can be generated using
- * svn_fs_generate_lock_token() (indicating that the caller wants to
- * dictate the lock token used), or it is @c NULL (indicating that the
- * caller wishes to have a new token generated by this function). If
- * @a token is not @c NULL, and represents an existing lock, then @a
- * path must match the path associated with that existing lock.
- *
* If @a expiration_date is zero, then create a non-expiring lock.
* Else, the lock will expire at @a expiration_date.
*
- * If @a current_rev is a valid revnum, then do an out-of-dateness
- * check. If the revnum is less than the last-changed-revision of @a
- * path (or if @a path doesn't exist in HEAD), return
- * #SVN_ERR_FS_OUT_OF_DATE.
+ * The results are returned in @a *results hash where the keys are
+ * <tt>const char *</tt> paths and the values are
+ * <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,
+ * this will be @c NULL if no lock was created.
*
* @note At this time, only files can be locked.
+ *
+ * @since New in 1.9.
+ */
+svn_error_t *
+svn_fs_lock2(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
+ const char *comment,
+ svn_boolean_t is_dav_comment,
+ apr_time_t expiration_date,
+ svn_boolean_t steal_lock,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/* Similar to svn_fs_lock2() but locks only a single @a path and
+ * returns the lock in @a *lock, allocated in @a pool, or an error.
+ *
+ * @since New in 1.2.
*/
svn_error_t *
svn_fs_lock(svn_lock_t **lock,
@@ -2578,20 +2625,53 @@ svn_fs_generate_lock_token(const char **
apr_pool_t *pool);
-/** Remove the lock on @a path represented by @a token in @a fs.
+/** Remove the locks on the paths in @a targets in @a fs, and return
+ * the results in @a *results allocated in @a result_pool.
*
- * If @a token doesn't point to a lock, return #SVN_ERR_FS_BAD_LOCK_TOKEN.
- * If @a 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.
+ * 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.
*
- * If @a token points to a lock, but the username of @a fs's access
+ * 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
* return error; allow the lock to be "broken" in any case. In the latter
- * case, @a token shall be @c NULL.
+ * case, the token shall be @c NULL.
+ *
+ * The results are returned in @a *results hash where the keys are
+ * <tt>const char *</tt> paths and the values are
+ * <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.
+ *
+ * @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.
+ *
+ * @note #svn_err_fs_lock_result_t is used to allow @c SVN_NO_ERROR to
+ * be returned (it is not possible to return @c SVN_NO_ERROR as a hash
+ * value directly), #svn_err_fs_lock_result_t->lock is always NULL.
*
* Use @a pool for temporary allocations.
+ *
+ * @since New in 1.9.
+ */
+svn_error_t *
+svn_fs_unlock2(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
+ svn_boolean_t break_lock,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/** Similar to svn_fs_unlock2() but only unlocks a single path.
+ *
+ * @since New in 1.2.
*/
svn_error_t *
svn_fs_unlock(svn_fs_t *fs,
Modified: subversion/trunk/subversion/include/svn_repos.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_repos.h?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_repos.h (original)
+++ subversion/trunk/subversion/include/svn_repos.h Thu Mar 13 18:47:17 2014
@@ -2184,20 +2184,41 @@ svn_repos_fs_begin_txn_for_update(svn_fs
* @{
*/
-/** Like svn_fs_lock(), but invoke the @a repos's pre- and
+/** Like svn_fs_lock2(), but invoke the @a repos's pre- and
* post-lock hooks before and after the locking action. Use @a pool
* for any necessary allocations.
*
- * If the pre-lock hook or svn_fs_lock() fails, throw the original
- * error to caller. If an error occurs when running the post-lock
- * hook, return the original error wrapped with
+ * 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
+ * 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 lock succeeded anyway.
+ * error, it knows that the some locks succeeded. In all cases the
+ * caller must handle 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.
*
+ * @since New in 1.9.
+ */
+svn_error_t *
+svn_repos_fs_lock2(apr_hash_t **results,
+ svn_repos_t *repos,
+ apr_hash_t *targets,
+ const char *comment,
+ svn_boolean_t is_dav_comment,
+ apr_time_t expiration_date,
+ svn_boolean_t steal_lock,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/** Similar to svn_repos_fs_lock2() but locks only a single path.
+ *
* @since New in 1.2.
*/
svn_error_t *
@@ -2226,6 +2247,14 @@ svn_repos_fs_lock(svn_lock_t **lock,
* @since New in 1.2.
*/
svn_error_t *
+svn_repos_fs_unlock2(apr_hash_t **results,
+ svn_repos_t *repos,
+ apr_hash_t *targets,
+ svn_boolean_t break_lock,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+svn_error_t *
svn_repos_fs_unlock(svn_repos_t *repos,
const char *path,
const char *token,
Modified: subversion/trunk/subversion/libsvn_client/locking_commands.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/locking_commands.c?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/locking_commands.c (original)
+++ subversion/trunk/subversion/libsvn_client/locking_commands.c Thu Mar 13 18:47:17 2014
@@ -100,7 +100,7 @@ store_locks_callback(void *baton,
if (do_lock)
{
- if (!ra_err)
+ if (!ra_err && lock)
{
SVN_ERR(svn_wc_add_lock2(lb->ctx->wc_ctx, local_abspath, lock,
lb->pool));
@@ -112,12 +112,14 @@ store_locks_callback(void *baton,
else /* unlocking */
{
/* Remove our wc lock token either a) if we got no error, or b) if
- we got any error except for owner mismatch. Note that the only
- errors that are handed to this callback will be locking-related
- errors. */
+ we got any error except for owner mismatch or hook failure (the
+ hook would be pre-unlock rather than post-unlock). Note that the
+ only errors that are handed to this callback will be
+ locking-related errors. */
if (!ra_err ||
- (ra_err && (ra_err->apr_err != SVN_ERR_FS_LOCK_OWNER_MISMATCH)))
+ (ra_err && (ra_err->apr_err != SVN_ERR_FS_LOCK_OWNER_MISMATCH
+ && ra_err->apr_err != SVN_ERR_REPOS_HOOK_FAILURE)))
{
SVN_ERR(svn_wc_remove_lock2(lb->ctx->wc_ctx, local_abspath,
lb->pool));
Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/fs-loader.c?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs/fs-loader.c (original)
+++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Thu Mar 13 18:47:17 2014
@@ -1583,54 +1583,115 @@ svn_fs_set_uuid(svn_fs_t *fs, const char
}
svn_error_t *
-svn_fs_lock(svn_lock_t **lock, svn_fs_t *fs, const char *path,
- const char *token, const char *comment,
- svn_boolean_t is_dav_comment, apr_time_t expiration_date,
- svn_revnum_t current_rev, svn_boolean_t steal_lock,
- apr_pool_t *pool)
+svn_fs_lock2(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
+ const char *comment,
+ svn_boolean_t is_dav_comment,
+ apr_time_t expiration_date,
+ svn_boolean_t steal_lock,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
+ apr_hash_index_t *hi;
+ apr_hash_t *ok_targets = apr_hash_make(scratch_pool), *ok_results;
+ svn_error_t *err;
+
+ *results = apr_hash_make(result_pool);
+
/* Enforce that the comment be xml-escapable. */
if (comment)
- {
- if (! svn_xml_is_xml_safe(comment, strlen(comment)))
- return svn_error_create
- (SVN_ERR_XML_UNESCAPABLE_DATA, NULL,
- _("Lock comment contains illegal characters"));
- }
+ if (! svn_xml_is_xml_safe(comment, strlen(comment)))
+ return svn_error_create(SVN_ERR_XML_UNESCAPABLE_DATA, NULL,
+ _("Lock comment contains illegal characters"));
+
+ if (expiration_date < 0)
+ return svn_error_create(SVN_ERR_INCORRECT_PARAMS, NULL,
+ _("Negative expiration date passed to svn_fs_lock"));
/* Enforce that the token be an XML-safe URI. */
- if (token)
+ for (hi = apr_hash_first(scratch_pool, targets); hi; hi = apr_hash_next(hi))
{
- const char *c;
+ const svn_fs_lock_target_t *target = svn__apr_hash_index_val(hi);
+
+ err = SVN_NO_ERROR;
+ if (target->token)
+ {
+ const char *c;
+
+
+ if (strncmp(target->token, "opaquelocktoken:", 16))
+ err = svn_error_createf(SVN_ERR_FS_BAD_LOCK_TOKEN, NULL,
+ _("Lock token URI '%s' has bad scheme; "
+ "expected '%s'"),
+ target->token, "opaquelocktoken");
+
+ if (!err)
+ for (c = target->token; *c && !err; c++)
+ if (! svn_ctype_isascii(*c) || svn_ctype_iscntrl(*c))
+ err = svn_error_createf(
+ SVN_ERR_FS_BAD_LOCK_TOKEN, NULL,
+ _("Lock token '%s' is not ASCII or is a "
+ "control character at byte %u"),
+ target->token,
+ (unsigned)(c - target->token));
+
+ /* strlen(token) == c - token. */
+ if (!err && !svn_xml_is_xml_safe(target->token, c - target->token))
+ err = svn_error_createf(SVN_ERR_FS_BAD_LOCK_TOKEN, NULL,
+ _("Lock token URI '%s' is not XML-safe"),
+ target->token);
+ }
- if (strncmp(token, "opaquelocktoken:", 16))
- return svn_error_createf(SVN_ERR_FS_BAD_LOCK_TOKEN, NULL,
- _("Lock token URI '%s' has bad scheme; "
- "expected '%s'"),
- token, "opaquelocktoken");
-
- for (c = token; *c; c++)
- if (! svn_ctype_isascii(*c) || svn_ctype_iscntrl(*c))
- return svn_error_createf(SVN_ERR_FS_BAD_LOCK_TOKEN, NULL,
- _("Lock token '%s' is not ASCII or is a "
- "control character at byte %u"),
- token, (unsigned)(c - token));
-
- /* strlen(token) == c - token. */
- if (! svn_xml_is_xml_safe(token, c - token))
- return svn_error_createf(SVN_ERR_FS_BAD_LOCK_TOKEN, NULL,
- _("Lock token URI '%s' is not XML-safe"),
- token);
+ if (err)
+ {
+ svn_fs_lock_result_t *result
+ = apr_palloc(result_pool, sizeof(svn_fs_lock_result_t));
+
+ result->err = err;
+ result->lock = NULL;
+ svn_hash_sets(*results, svn__apr_hash_index_key(hi), result);
+ }
+ else
+ svn_hash_sets(ok_targets, svn__apr_hash_index_key(hi), target);
}
- if (expiration_date < 0)
- return svn_error_create
- (SVN_ERR_INCORRECT_PARAMS, NULL,
- _("Negative expiration date passed to svn_fs_lock"));
-
- return svn_error_trace(fs->vtable->lock(lock, fs, path, token, comment,
- is_dav_comment, expiration_date,
- current_rev, steal_lock, pool));
+ err = fs->vtable->lock(&ok_results, fs, ok_targets, comment,
+ is_dav_comment, expiration_date,
+ steal_lock, result_pool, scratch_pool);
+
+ for (hi = apr_hash_first(scratch_pool, ok_results);
+ hi; hi = apr_hash_next(hi))
+ svn_hash_sets(*results, svn__apr_hash_index_key(hi),
+ svn__apr_hash_index_val(hi));
+
+ return svn_error_trace(err);
+}
+
+svn_error_t *
+svn_fs_lock(svn_lock_t **lock, svn_fs_t *fs, const char *path,
+ const char *token, const char *comment,
+ svn_boolean_t is_dav_comment, apr_time_t expiration_date,
+ svn_revnum_t current_rev, svn_boolean_t steal_lock,
+ apr_pool_t *pool)
+{
+ apr_hash_t *targets = apr_hash_make(pool), *results;
+ svn_fs_lock_target_t target;
+ svn_fs_lock_result_t *result;
+
+ target.token = token;
+ target.current_rev = current_rev;
+ svn_hash_sets(targets, path, &target);
+
+ SVN_ERR(svn_fs_lock2(&results, fs, targets, comment, is_dav_comment,
+ expiration_date, steal_lock, pool, pool));
+
+ SVN_ERR_ASSERT(apr_hash_count(results));
+ result = svn__apr_hash_index_val(apr_hash_first(pool, results));
+ if (result->lock)
+ *lock = result->lock;
+
+ return result->err;
}
svn_error_t *
@@ -1640,11 +1701,34 @@ svn_fs_generate_lock_token(const char **
}
svn_error_t *
+svn_fs_unlock2(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
+ svn_boolean_t break_lock,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ return svn_error_trace(fs->vtable->unlock(results, fs, targets, break_lock,
+ result_pool, scratch_pool));
+}
+
+svn_error_t *
svn_fs_unlock(svn_fs_t *fs, const char *path, const char *token,
svn_boolean_t break_lock, apr_pool_t *pool)
{
- return svn_error_trace(fs->vtable->unlock(fs, path, token, break_lock,
- pool));
+ apr_hash_t *targets = apr_hash_make(pool), *results;
+ svn_fs_lock_result_t *result;
+
+ if (!token)
+ token = "";
+ svn_hash_sets(targets, path, token);
+
+ SVN_ERR(svn_fs_unlock2(&results, fs, targets, break_lock, pool, pool));
+
+ SVN_ERR_ASSERT(apr_hash_count(results));
+ result = svn__apr_hash_index_val(apr_hash_first(pool, results));
+
+ return result->err;
}
svn_error_t *
Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/fs-loader.h?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs/fs-loader.h (original)
+++ subversion/trunk/subversion/libsvn_fs/fs-loader.h Thu Mar 13 18:47:17 2014
@@ -218,16 +218,18 @@ typedef struct fs_vtable_t
svn_error_t *(*list_transactions)(apr_array_header_t **names_p,
svn_fs_t *fs, apr_pool_t *pool);
svn_error_t *(*deltify)(svn_fs_t *fs, svn_revnum_t rev, apr_pool_t *pool);
- svn_error_t *(*lock)(svn_lock_t **lock, svn_fs_t *fs,
- const char *path, const char *token,
+ svn_error_t *(*lock)(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
const char *comment, svn_boolean_t is_dav_comment,
- apr_time_t expiration_date,
- svn_revnum_t current_rev, svn_boolean_t steal_lock,
- apr_pool_t *pool);
+ apr_time_t expiration_date, svn_boolean_t steal_lock,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool);
svn_error_t *(*generate_lock_token)(const char **token, svn_fs_t *fs,
apr_pool_t *pool);
- svn_error_t *(*unlock)(svn_fs_t *fs, const char *path, const char *token,
- svn_boolean_t break_lock, apr_pool_t *pool);
+ svn_error_t *(*unlock)(apr_hash_t ** results,
+ svn_fs_t *fs, apr_hash_t *targets,
+ svn_boolean_t break_lock,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool);
svn_error_t *(*get_lock)(svn_lock_t **lock, svn_fs_t *fs,
const char *path, apr_pool_t *pool);
svn_error_t *(*get_locks)(svn_fs_t *fs, const char *path, svn_depth_t depth,
Modified: subversion/trunk/subversion/libsvn_fs_base/lock.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/lock.c?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/lock.c (original)
+++ subversion/trunk/subversion/libsvn_fs_base/lock.c Thu Mar 13 18:47:17 2014
@@ -91,6 +91,8 @@ txn_body_lock(void *baton, trail_t *trai
svn_lock_t *existing_lock;
svn_lock_t *lock;
+ *args->lock_p = NULL;
+
SVN_ERR(svn_fs_base__get_path_kind(&kind, args->path, trail, trail->pool));
/* Until we implement directory locks someday, we only allow locks
@@ -215,31 +217,47 @@ txn_body_lock(void *baton, trail_t *trai
svn_error_t *
-svn_fs_base__lock(svn_lock_t **lock,
+svn_fs_base__lock(apr_hash_t **results,
svn_fs_t *fs,
- const char *path,
- const char *token,
+ apr_hash_t *targets,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
- svn_revnum_t current_rev,
svn_boolean_t steal_lock,
- apr_pool_t *pool)
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
- struct lock_args args;
+ apr_hash_index_t *hi;
+
+ *results = apr_hash_make(result_pool);
SVN_ERR(svn_fs__check_fs(fs, TRUE));
- args.lock_p = lock;
- args.path = svn_fs__canonicalize_abspath(path, pool);
- args.token = token;
- args.comment = comment;
- args.is_dav_comment = is_dav_comment;
- args.steal_lock = steal_lock;
- args.expiration_date = expiration_date;
- args.current_rev = current_rev;
+ for (hi = apr_hash_first(scratch_pool, targets); hi; hi = apr_hash_next(hi))
+ {
+ struct lock_args args;
+ const char *path = svn__apr_hash_index_key(hi);
+ const svn_fs_lock_target_t *target = svn__apr_hash_index_val(hi);
+ svn_lock_t *lock;
+ svn_fs_lock_result_t *result = apr_palloc(result_pool,
+ sizeof(svn_fs_lock_result_t));
+
+ args.lock_p = &lock;
+ args.path = svn_fs__canonicalize_abspath(path, result_pool);
+ args.token = target->token;
+ args.comment = comment;
+ args.is_dav_comment = is_dav_comment;
+ args.steal_lock = steal_lock;
+ args.expiration_date = expiration_date;
+ args.current_rev = target->current_rev;
+
+ result->err = svn_fs_base__retry_txn(fs, txn_body_lock, &args, FALSE,
+ scratch_pool);
+ result->lock = lock;
+ svn_hash_sets(*results, args.path, result);
+ }
- return svn_fs_base__retry_txn(fs, txn_body_lock, &args, FALSE, pool);
+ return SVN_NO_ERROR;
}
@@ -307,20 +325,37 @@ txn_body_unlock(void *baton, trail_t *tr
svn_error_t *
-svn_fs_base__unlock(svn_fs_t *fs,
- const char *path,
- const char *token,
+svn_fs_base__unlock(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
svn_boolean_t break_lock,
- apr_pool_t *pool)
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
- struct unlock_args args;
+ apr_hash_index_t *hi;
+
+ *results = apr_hash_make(result_pool);
SVN_ERR(svn_fs__check_fs(fs, TRUE));
- args.path = svn_fs__canonicalize_abspath(path, pool);
- args.token = token;
- args.break_lock = break_lock;
- return svn_fs_base__retry_txn(fs, txn_body_unlock, &args, TRUE, pool);
+ for (hi = apr_hash_first(scratch_pool, targets); hi; hi = apr_hash_next(hi))
+ {
+ struct unlock_args args;
+ const char *path = svn__apr_hash_index_key(hi);
+ const char *token = svn__apr_hash_index_val(hi);
+ svn_fs_lock_result_t *result = apr_palloc(result_pool,
+ sizeof(svn_fs_lock_result_t));
+
+ args.path = svn_fs__canonicalize_abspath(path, result_pool);
+ args.token = token;
+ args.break_lock = break_lock;
+ result->err = svn_fs_base__retry_txn(fs, txn_body_unlock, &args, TRUE,
+ scratch_pool);
+ result->lock = NULL;
+ svn_hash_sets(*results, args.path, result);
+ }
+
+ return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_fs_base/lock.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/lock.h?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/lock.h (original)
+++ subversion/trunk/subversion/libsvn_fs_base/lock.h Thu Mar 13 18:47:17 2014
@@ -35,26 +35,26 @@ extern "C" {
/* These functions implement part of the FS loader library's fs
vtables. See the public svn_fs.h for docstrings.*/
-svn_error_t *svn_fs_base__lock(svn_lock_t **lock,
+svn_error_t *svn_fs_base__lock(apr_hash_t **results,
svn_fs_t *fs,
- const char *path,
- const char *token,
+ apr_hash_t *targets,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
- svn_revnum_t current_rev,
svn_boolean_t steal_lock,
- apr_pool_t *pool);
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
svn_error_t *svn_fs_base__generate_lock_token(const char **token,
svn_fs_t *fs,
apr_pool_t *pool);
-svn_error_t *svn_fs_base__unlock(svn_fs_t *fs,
- const char *path,
- const char *token,
+svn_error_t *svn_fs_base__unlock(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
svn_boolean_t break_lock,
- apr_pool_t *pool);
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
svn_error_t *svn_fs_base__get_lock(svn_lock_t **lock,
svn_fs_t *fs,
Modified: subversion/trunk/subversion/libsvn_fs_fs/lock.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/lock.c?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/lock.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/lock.c Thu Mar 13 18:47:17 2014
@@ -20,7 +20,6 @@
* ====================================================================
*/
-
#include "svn_pools.h"
#include "svn_error.h"
#include "svn_dirent_uri.h"
@@ -42,6 +41,7 @@
#include "private/svn_fs_util.h"
#include "private/svn_fspath.h"
+#include "private/svn_sorts_private.h"
#include "svn_private_config.h"
/* Names of hash keys used to store a lock for writing to disk. */
@@ -340,8 +340,6 @@ read_digest_file(apr_hash_t **children_p
/* Write LOCK in FS to the actual OS filesystem.
Use PERMS_REFERENCE for the permissions of any digest files.
-
- Note: this takes an FS_PATH because it's called from the hotcopy logic.
*/
static svn_error_t *
set_lock(const char *fs_path,
@@ -349,129 +347,110 @@ set_lock(const char *fs_path,
const char *perms_reference,
apr_pool_t *pool)
{
- svn_stringbuf_t *this_path = svn_stringbuf_create(lock->path, pool);
- const char *lock_digest_path = NULL;
- apr_pool_t *subpool;
+ const char *digest_path;
+ apr_hash_t *children;
- SVN_ERR_ASSERT(lock);
+ SVN_ERR(digest_path_from_path(&digest_path, fs_path, lock->path, pool));
- /* Iterate in reverse, creating the lock for LOCK->path, and then
- just adding entries for its parent, until we reach a parent
- that's already listed in *its* parent. */
- subpool = svn_pool_create(pool);
- while (1729)
- {
- const char *digest_path, *digest_file;
- apr_hash_t *this_children;
- svn_lock_t *this_lock;
+ /* We could get away without reading the file as children should
+ always come back empty. */
+ SVN_ERR(read_digest_file(&children, NULL, fs_path, digest_path, pool));
- svn_pool_clear(subpool);
+ SVN_ERR(write_digest_file(children, lock, fs_path, digest_path,
+ perms_reference, pool));
- /* Calculate the DIGEST_PATH for the currently FS path, and then
- get its DIGEST_FILE basename. */
- SVN_ERR(digest_path_from_path(&digest_path, fs_path, this_path->data,
- subpool));
- digest_file = svn_dirent_basename(digest_path, subpool);
+ return SVN_NO_ERROR;
+}
- SVN_ERR(read_digest_file(&this_children, &this_lock, fs_path,
- digest_path, subpool));
+static svn_error_t *
+delete_lock(const char *fs_path,
+ const char *path,
+ apr_pool_t *pool)
+{
+ const char *digest_path;
- /* We're either writing a new lock (first time through only) or
- a new entry (every time but the first). */
- if (lock)
- {
- this_lock = lock;
- lock = NULL;
- lock_digest_path = apr_pstrdup(pool, digest_file);
- }
- else
- {
- /* If we already have an entry for this path, we're done. */
- if (svn_hash_gets(this_children, lock_digest_path))
- break;
- svn_hash_sets(this_children, lock_digest_path, (void *)1);
- }
- SVN_ERR(write_digest_file(this_children, this_lock, fs_path,
- digest_path, perms_reference, subpool));
+ SVN_ERR(digest_path_from_path(&digest_path, fs_path, path, pool));
- /* Prep for next iteration, or bail if we're done. */
- if (svn_fspath__is_root(this_path->data, this_path->len))
- break;
- svn_stringbuf_set(this_path,
- svn_fspath__dirname(this_path->data, subpool));
- }
+ SVN_ERR(svn_io_remove_file2(digest_path, TRUE, pool));
- svn_pool_destroy(subpool);
return SVN_NO_ERROR;
}
-/* Delete LOCK from FS in the actual OS filesystem. */
static svn_error_t *
-delete_lock(svn_fs_t *fs,
- svn_lock_t *lock,
- apr_pool_t *pool)
+add_to_digest(const char *fs_path,
+ apr_array_header_t *paths,
+ const char *index_path,
+ const char *perms_reference,
+ apr_pool_t *pool)
{
- svn_stringbuf_t *this_path = svn_stringbuf_create(lock->path, pool);
- const char *child_to_kill = NULL;
- apr_pool_t *subpool;
+ const char *index_digest_path;
+ apr_hash_t *children;
+ svn_lock_t *lock;
+ int i, original_count;
- SVN_ERR_ASSERT(lock);
+ SVN_ERR(digest_path_from_path(&index_digest_path, fs_path, index_path, pool));
- /* Iterate in reverse, deleting the lock for LOCK->path, and then
- deleting its entry as it appears in each of its parents. */
- subpool = svn_pool_create(pool);
- while (1729)
+ SVN_ERR(read_digest_file(&children, &lock, fs_path, index_digest_path, pool));
+
+ original_count = apr_hash_count(children);
+
+ for (i = 0; i < paths->nelts; ++i)
{
+ const char *path = APR_ARRAY_IDX(paths, i, const char *);
const char *digest_path, *digest_file;
- apr_hash_t *this_children;
- svn_lock_t *this_lock;
- svn_pool_clear(subpool);
+ SVN_ERR(digest_path_from_path(&digest_path, fs_path, path, pool));
+ digest_file = svn_dirent_basename(digest_path, NULL);
+ svn_hash_sets(children, digest_file, (void *)1);
+ }
- /* Calculate the DIGEST_PATH for the currently FS path, and then
- get its DIGEST_FILE basename. */
- SVN_ERR(digest_path_from_path(&digest_path, fs->path, this_path->data,
- subpool));
- digest_file = svn_dirent_basename(digest_path, subpool);
+ if (apr_hash_count(children) != original_count)
+ SVN_ERR(write_digest_file(children, lock, fs_path, index_digest_path,
+ perms_reference, pool));
- SVN_ERR(read_digest_file(&this_children, &this_lock, fs->path,
- digest_path, subpool));
+ return SVN_NO_ERROR;
+}
- /* Delete the lock (first time through only). */
- if (lock)
- {
- this_lock = NULL;
- lock = NULL;
- child_to_kill = apr_pstrdup(pool, digest_file);
- }
+static svn_error_t *
+delete_from_digest(const char *fs_path,
+ apr_array_header_t *paths,
+ const char *index_path,
+ const char *perms_reference,
+ apr_pool_t *pool)
+{
+ const char *index_digest_path;
+ apr_hash_t *children;
+ svn_lock_t *lock;
+ int i;
- if (child_to_kill)
- svn_hash_sets(this_children, child_to_kill, NULL);
+ SVN_ERR(digest_path_from_path(&index_digest_path, fs_path, index_path, pool));
- if (! (this_lock || apr_hash_count(this_children) != 0))
- {
- /* Special case: no goodz, no file. And remember to nix
- the entry for it in its parent. */
- SVN_ERR(svn_io_remove_file2(digest_path, FALSE, subpool));
- }
- else
- {
- const char *rev_0_path = svn_fs_fs__path_rev_absolute(fs, 0, pool);
- SVN_ERR(write_digest_file(this_children, this_lock, fs->path,
- digest_path, rev_0_path, subpool));
- }
+ SVN_ERR(read_digest_file(&children, &lock, fs_path, index_digest_path, pool));
- /* Prep for next iteration, or bail if we're done. */
- if (svn_fspath__is_root(this_path->data, this_path->len))
- break;
- svn_stringbuf_set(this_path,
- svn_fspath__dirname(this_path->data, subpool));
+ for (i = 0; i < paths->nelts; ++i)
+ {
+ const char *path = APR_ARRAY_IDX(paths, i, const char *);
+ const char *digest_path, *digest_file;
+
+ SVN_ERR(digest_path_from_path(&digest_path, fs_path, path, pool));
+ digest_file = svn_dirent_basename(digest_path, NULL);
+ svn_hash_sets(children, digest_file, NULL);
}
- svn_pool_destroy(subpool);
+ if (apr_hash_count(children) || lock)
+ SVN_ERR(write_digest_file(children, lock, fs_path, index_digest_path,
+ perms_reference, pool));
+ else
+ SVN_ERR(svn_io_remove_file2(index_digest_path, TRUE, pool));
+
return SVN_NO_ERROR;
}
+static svn_error_t *
+unlock_single(svn_fs_t *fs,
+ svn_lock_t *lock,
+ apr_pool_t *pool);
+
/* Set *LOCK_P to the lock for PATH in FS. HAVE_WRITE_LOCK should be
TRUE if the caller (or one of its callers) has taken out the
repository-wide write lock, FALSE otherwise. If MUST_EXIST is
@@ -506,7 +485,7 @@ get_lock(svn_lock_t **lock_p,
/* Only remove the lock if we have the write lock.
Read operations shouldn't change the filesystem. */
if (have_write_lock)
- SVN_ERR(delete_lock(fs, lock, pool));
+ SVN_ERR(unlock_single(fs, lock, pool));
return SVN_FS__ERR_LOCK_EXPIRED(fs, lock->token);
}
@@ -581,7 +560,7 @@ locks_walker(void *baton,
/* Only remove the lock if we have the write lock.
Read operations shouldn't change the filesystem. */
if (have_write_lock)
- SVN_ERR(delete_lock(wlb->fs, lock, pool));
+ SVN_ERR(unlock_single(wlb->fs, lock, pool));
}
}
@@ -738,68 +717,59 @@ svn_fs_fs__allow_locked_operation(const
/* Baton used for lock_body below. */
struct lock_baton {
- svn_lock_t **lock_p;
svn_fs_t *fs;
- const char *path;
- const char *token;
+ apr_array_header_t *targets;
+ apr_array_header_t *infos;
const char *comment;
svn_boolean_t is_dav_comment;
apr_time_t expiration_date;
- svn_revnum_t current_rev;
svn_boolean_t steal_lock;
- apr_pool_t *pool;
+ apr_pool_t *result_pool;
};
-
-/* This implements the svn_fs_fs__with_write_lock() 'body' callback
- type, and assumes that the write lock is held.
- BATON is a 'struct lock_baton *'. */
static svn_error_t *
-lock_body(void *baton, apr_pool_t *pool)
+check_lock(svn_error_t **fs_err,
+ const char *path,
+ const svn_fs_lock_target_t *target,
+ struct lock_baton *lb,
+ svn_fs_root_t *root,
+ apr_pool_t *pool)
{
- struct lock_baton *lb = baton;
svn_node_kind_t kind;
svn_lock_t *existing_lock;
- svn_lock_t *lock;
- svn_fs_root_t *root;
- svn_revnum_t youngest;
- const char *rev_0_path;
- /* Until we implement directory locks someday, we only allow locks
- on files or non-existent paths. */
- /* Use fs->vtable->foo instead of svn_fs_foo to avoid circular
- library dependencies, which are not portable. */
- SVN_ERR(lb->fs->vtable->youngest_rev(&youngest, lb->fs, pool));
- SVN_ERR(lb->fs->vtable->revision_root(&root, lb->fs, youngest, pool));
- SVN_ERR(svn_fs_fs__check_path(&kind, root, lb->path, pool));
+ *fs_err = SVN_NO_ERROR;
+
+ SVN_ERR(svn_fs_fs__check_path(&kind, root, path, pool));
if (kind == svn_node_dir)
- return SVN_FS__ERR_NOT_FILE(lb->fs, lb->path);
+ {
+ *fs_err = SVN_FS__ERR_NOT_FILE(lb->fs, path);
+ return SVN_NO_ERROR;
+ }
/* While our locking implementation easily supports the locking of
nonexistent paths, we deliberately choose not to allow such madness. */
if (kind == svn_node_none)
{
- if (SVN_IS_VALID_REVNUM(lb->current_rev))
- return svn_error_createf(
+ if (SVN_IS_VALID_REVNUM(target->current_rev))
+ *fs_err = svn_error_createf(
SVN_ERR_FS_OUT_OF_DATE, NULL,
_("Path '%s' doesn't exist in HEAD revision"),
- lb->path);
+ path);
else
- return svn_error_createf(
+ *fs_err = svn_error_createf(
SVN_ERR_FS_NOT_FOUND, NULL,
_("Path '%s' doesn't exist in HEAD revision"),
- lb->path);
- }
+ path);
- /* We need to have a username attached to the fs. */
- if (!lb->fs->access_ctx || !lb->fs->access_ctx->username)
- return SVN_FS__ERR_NO_USER(lb->fs);
+ return SVN_NO_ERROR;
+ }
/* Is the caller attempting to lock an out-of-date working file? */
- if (SVN_IS_VALID_REVNUM(lb->current_rev))
+ if (SVN_IS_VALID_REVNUM(target->current_rev))
{
svn_revnum_t created_rev;
- SVN_ERR(svn_fs_fs__node_created_rev(&created_rev, root, lb->path,
+ SVN_ERR(svn_fs_fs__node_created_rev(&created_rev, root, path,
pool));
/* SVN_INVALID_REVNUM means the path doesn't exist. So
@@ -807,14 +777,22 @@ lock_body(void *baton, apr_pool_t *pool)
working copy, but somebody else has deleted the thing
from HEAD. That counts as being 'out of date'. */
if (! SVN_IS_VALID_REVNUM(created_rev))
- return svn_error_createf
- (SVN_ERR_FS_OUT_OF_DATE, NULL,
- _("Path '%s' doesn't exist in HEAD revision"), lb->path);
-
- if (lb->current_rev < created_rev)
- return svn_error_createf
- (SVN_ERR_FS_OUT_OF_DATE, NULL,
- _("Lock failed: newer version of '%s' exists"), lb->path);
+ {
+ *fs_err = svn_error_createf
+ (SVN_ERR_FS_OUT_OF_DATE, NULL,
+ _("Path '%s' doesn't exist in HEAD revision"), path);
+
+ return SVN_NO_ERROR;
+ }
+
+ if (target->current_rev < created_rev)
+ {
+ *fs_err = svn_error_createf
+ (SVN_ERR_FS_OUT_OF_DATE, NULL,
+ _("Lock failed: newer version of '%s' exists"), path);
+
+ return SVN_NO_ERROR;
+ }
}
/* If the caller provided a TOKEN, we *really* need to see
@@ -833,117 +811,429 @@ lock_body(void *baton, apr_pool_t *pool)
acceptable to ignore; it means that the path is now free and
clear for locking, because the fsfs funcs just cleared out both
of the tables for us. */
- SVN_ERR(get_lock_helper(lb->fs, &existing_lock, lb->path, TRUE, pool));
+ SVN_ERR(get_lock_helper(lb->fs, &existing_lock, path, TRUE, pool));
if (existing_lock)
{
if (! lb->steal_lock)
{
/* Sorry, the path is already locked. */
- return SVN_FS__ERR_PATH_ALREADY_LOCKED(lb->fs, existing_lock);
- }
- else
- {
- /* STEAL_LOCK was passed, so fs_username is "stealing" the
- lock from lock->owner. Destroy the existing lock. */
- SVN_ERR(delete_lock(lb->fs, existing_lock, pool));
+ *fs_err = SVN_FS__ERR_PATH_ALREADY_LOCKED(lb->fs, existing_lock);
+ return SVN_NO_ERROR;
}
}
- /* Create our new lock, and add it to the tables.
- Ensure that the lock is created in the correct pool. */
- lock = svn_lock_create(lb->pool);
- if (lb->token)
- lock->token = apr_pstrdup(lb->pool, lb->token);
- else
- SVN_ERR(svn_fs_fs__generate_lock_token(&(lock->token), lb->fs,
- lb->pool));
- lock->path = apr_pstrdup(lb->pool, lb->path);
- lock->owner = apr_pstrdup(lb->pool, lb->fs->access_ctx->username);
- lock->comment = apr_pstrdup(lb->pool, lb->comment);
- lock->is_dav_comment = lb->is_dav_comment;
- lock->creation_date = apr_time_now();
- lock->expiration_date = lb->expiration_date;
+ return SVN_NO_ERROR;
+}
+
+struct lock_info_t {
+ const char *path;
+ const char *component;
+ svn_lock_t *lock;
+ svn_error_t *fs_err;
+};
+
+/* This implements the svn_fs_fs__with_write_lock() 'body' callback
+ type, and assumes that the write lock is held.
+ BATON is a 'struct lock_baton *'. */
+static svn_error_t *
+lock_body(void *baton, apr_pool_t *pool)
+{
+ struct lock_baton *lb = baton;
+ svn_fs_root_t *root;
+ svn_revnum_t youngest;
+ const char *rev_0_path;
+ int i, outstanding = 0;
+ apr_pool_t *iterpool = svn_pool_create(pool);
+
+ lb->infos = apr_array_make(lb->result_pool, lb->targets->nelts,
+ sizeof(struct lock_info_t));
+
+ /* Until we implement directory locks someday, we only allow locks
+ on files or non-existent paths. */
+ /* Use fs->vtable->foo instead of svn_fs_foo to avoid circular
+ library dependencies, which are not portable. */
+ SVN_ERR(lb->fs->vtable->youngest_rev(&youngest, lb->fs, pool));
+ SVN_ERR(lb->fs->vtable->revision_root(&root, lb->fs, youngest, pool));
+
+ for (i = 0; i < lb->targets->nelts; ++i)
+ {
+ const svn_sort__item_t *item = &APR_ARRAY_IDX(lb->targets, i,
+ svn_sort__item_t);
+ const svn_fs_lock_target_t *target = item->value;
+ struct lock_info_t info;
+
+ svn_pool_clear(iterpool);
+
+ info.path = item->key;
+ SVN_ERR(check_lock(&info.fs_err, info.path, target, lb, root, iterpool));
+ info.lock = NULL;
+ info.component = NULL;
+ APR_ARRAY_PUSH(lb->infos, struct lock_info_t) = info;
+ if (!info.fs_err)
+ ++outstanding;
+ }
rev_0_path = svn_fs_fs__path_rev_absolute(lb->fs, 0, pool);
- SVN_ERR(set_lock(lb->fs->path, lock, rev_0_path, pool));
- *lb->lock_p = lock;
+ /* Given the paths:
+
+ /foo/bar/f
+ /foo/bar/g
+ /zig/x
+
+ we loop through repeatedly. The first pass sees '/' on all paths
+ and writes the '/' index. The second pass sees '/foo' twice and
+ writes that index followed by '/zig' and that index. The third
+ pass sees '/foo/bar' twice and writes that index, and then writes
+ the lock for '/zig/x'. The fourth pass writes the locks for
+ '/foo/bar/f' and '/foo/bar/g'.
+
+ Writing indices before locks is correct: if interrupted it leaves
+ indices without locks rather than locks without indices. An
+ index without a lock is consistent in that it always shows up as
+ unlocked in svn_fs_fs__allow_locked_operation. A lock without an
+ index is inconsistent, svn_fs_fs__allow_locked_operation will
+ show locked on the file but unlocked on the parent. */
+
+
+ while (outstanding)
+ {
+ const char *last_path = NULL;
+ apr_array_header_t *paths;
+
+ svn_pool_clear(iterpool);
+ paths = apr_array_make(iterpool, 1, sizeof(const char *));
+
+ for (i = 0; i < lb->infos->nelts; ++i)
+ {
+ struct lock_info_t *info = &APR_ARRAY_IDX(lb->infos, i,
+ struct lock_info_t);
+ const svn_sort__item_t *item = &APR_ARRAY_IDX(lb->targets, i,
+ svn_sort__item_t);
+ const svn_fs_lock_target_t *target = item->value;
+
+ if (!info->fs_err && !info->lock)
+ {
+ if (!info->component)
+ {
+ info->component = info->path;
+ APR_ARRAY_PUSH(paths, const char *) = info->path;
+ last_path = "/";
+ }
+ else
+ {
+ info->component = strchr(info->component + 1, '/');
+ if (!info->component)
+ {
+ /* The component is a path to lock, this cannot
+ match a previous path that need to be indexed. */
+ if (paths->nelts)
+ {
+ SVN_ERR(add_to_digest(lb->fs->path, paths, last_path,
+ rev_0_path, iterpool));
+ apr_array_clear(paths);
+ last_path = NULL;
+ }
+
+ info->lock = svn_lock_create(lb->result_pool);
+ if (target->token)
+ info->lock->token = target->token;
+ else
+ SVN_ERR(svn_fs_fs__generate_lock_token(
+ &(info->lock->token), lb->fs,
+ lb->result_pool));
+ info->lock->path = info->path;
+ info->lock->owner = lb->fs->access_ctx->username;
+ info->lock->comment = lb->comment;
+ info->lock->is_dav_comment = lb->is_dav_comment;
+ info->lock->creation_date = apr_time_now();
+ info->lock->expiration_date = lb->expiration_date;
+
+ info->fs_err = set_lock(lb->fs->path, info->lock,
+ rev_0_path, iterpool);
+ --outstanding;
+ }
+ else
+ {
+ /* The component is a path to an index. */
+ apr_size_t len = info->component - info->path;
+
+ if (last_path
+ && (strncmp(last_path, info->path, len)
+ || strlen(last_path) != len))
+ {
+ /* No match to the previous paths to index. */
+ SVN_ERR(add_to_digest(lb->fs->path, paths, last_path,
+ rev_0_path, iterpool));
+ apr_array_clear(paths);
+ last_path = NULL;
+ }
+ APR_ARRAY_PUSH(paths, const char *) = info->path;
+ if (!last_path)
+ last_path = apr_pstrndup(iterpool, info->path, len);
+ }
+ }
+ }
+
+ if (last_path && i == lb->infos->nelts - 1)
+ SVN_ERR(add_to_digest(lb->fs->path, paths, last_path,
+ rev_0_path, iterpool));
+ }
+ }
+
return SVN_NO_ERROR;
}
-/* Baton used for unlock_body below. */
struct unlock_baton {
svn_fs_t *fs;
- const char *path;
- const char *token;
+ apr_array_header_t *targets;
+ apr_array_header_t *infos;
+ svn_boolean_t skip_check;
svn_boolean_t break_lock;
+ apr_pool_t *result_pool;
+};
+
+static svn_error_t *
+check_unlock(svn_error_t **fs_err,
+ const char *path,
+ const char *token,
+ struct unlock_baton *ub,
+ svn_fs_root_t *root,
+ apr_pool_t *pool)
+{
+ svn_lock_t *lock;
+
+ *fs_err = get_lock(&lock, ub->fs, path, TRUE, TRUE, pool);
+ if (!*fs_err && !ub->break_lock)
+ {
+ if (strcmp(token, lock->token) != 0)
+ *fs_err = SVN_FS__ERR_NO_SUCH_LOCK(ub->fs, path);
+ else if (strcmp(ub->fs->access_ctx->username, lock->owner) != 0)
+ *fs_err = SVN_FS__ERR_LOCK_OWNER_MISMATCH(ub->fs,
+ ub->fs->access_ctx->username,
+ lock->owner);
+ }
+
+ return SVN_NO_ERROR;
+}
+
+struct unlock_info_t {
+ const char *path;
+ const char *component;
+ svn_error_t *fs_err;
+ int components;
};
-/* This implements the svn_fs_fs__with_write_lock() 'body' callback
- type, and assumes that the write lock is held.
- BATON is a 'struct unlock_baton *'. */
static svn_error_t *
unlock_body(void *baton, apr_pool_t *pool)
{
struct unlock_baton *ub = baton;
- svn_lock_t *lock;
+ svn_fs_root_t *root;
+ svn_revnum_t youngest;
+ const char *rev_0_path;
+ int i, max_components = 0, outstanding = 0;
+ apr_pool_t *iterpool = svn_pool_create(pool);
+
+ ub->infos = apr_array_make(ub->result_pool, ub->targets->nelts,
+ sizeof(struct unlock_info_t));
+
+ SVN_ERR(ub->fs->vtable->youngest_rev(&youngest, ub->fs, pool));
+ SVN_ERR(ub->fs->vtable->revision_root(&root, ub->fs, youngest, pool));
+
+ for (i = 0; i < ub->targets->nelts; ++i)
+ {
+ const svn_sort__item_t *item = &APR_ARRAY_IDX(ub->targets, i,
+ svn_sort__item_t);
+ const char *token = item->value;
+ struct unlock_info_t info;
+
+ svn_pool_clear(iterpool);
+
+ info.path = item->key;
+ if (!ub->skip_check)
+ SVN_ERR(check_unlock(&info.fs_err, info.path, token, ub, root,
+ iterpool));
+ if (!info.fs_err)
+ {
+ const char *s;
+
+ info.components = 1;
+ info.component = info.path;
+ while((s = strchr(info.component + 1, '/')))
+ {
+ info.component = s;
+ ++info.components;
+ }
+
+ if (info.components > max_components)
+ max_components = info.components;
+
+ ++outstanding;
+ }
+ APR_ARRAY_PUSH(ub->infos, struct unlock_info_t) = info;
+ }
- /* This could return SVN_ERR_FS_BAD_LOCK_TOKEN or SVN_ERR_FS_LOCK_EXPIRED. */
- SVN_ERR(get_lock(&lock, ub->fs, ub->path, TRUE, TRUE, pool));
+ rev_0_path = svn_fs_fs__path_rev_absolute(ub->fs, 0, pool);
- /* Unless breaking the lock, we do some checks. */
- if (! ub->break_lock)
+ for (i = max_components; i >= 0; --i)
{
- /* Sanity check: the incoming token should match lock->token. */
- if (strcmp(ub->token, lock->token) != 0)
- return SVN_FS__ERR_NO_SUCH_LOCK(ub->fs, lock->path);
+ const char *last_path = NULL;
+ apr_array_header_t *paths;
+ int j;
- /* There better be a username attached to the fs. */
- if (! (ub->fs->access_ctx && ub->fs->access_ctx->username))
- return SVN_FS__ERR_NO_USER(ub->fs);
+ svn_pool_clear(iterpool);
+ paths = apr_array_make(pool, 1, sizeof(const char *));
- /* And that username better be the same as the lock's owner. */
- if (strcmp(ub->fs->access_ctx->username, lock->owner) != 0)
- return SVN_FS__ERR_LOCK_OWNER_MISMATCH(
- ub->fs, ub->fs->access_ctx->username, lock->owner);
+ for (j = 0; j < ub->infos->nelts; ++j)
+ {
+ struct unlock_info_t *info = &APR_ARRAY_IDX(ub->infos, j,
+ struct unlock_info_t);
+
+ if (!info->fs_err && info->path)
+ {
+
+ if (info->components == i)
+ {
+ SVN_ERR(delete_lock(ub->fs->path, info->path, iterpool));
+ }
+ else if (info->components > i)
+ {
+ apr_size_t len = info->component - info->path;
+
+ if (last_path
+ && strcmp(last_path, "/")
+ && (strncmp(last_path, info->path, len)
+ || strlen(last_path) != len))
+ {
+ SVN_ERR(delete_from_digest(ub->fs->path, paths, last_path,
+ rev_0_path, iterpool));
+ apr_array_clear(paths);
+ last_path = NULL;
+ }
+ APR_ARRAY_PUSH(paths, const char *) = info->path;
+ if (!last_path)
+ {
+ if (info->component > info->path)
+ last_path = apr_pstrndup(pool, info->path, len);
+ else
+ last_path = "/";
+ }
+
+ if (info->component > info->path)
+ {
+ --info->component;
+ while(info->component[0] != '/')
+ --info->component;
+ }
+ }
+ }
+
+ if (last_path && j == ub->infos->nelts - 1)
+ SVN_ERR(delete_from_digest(ub->fs->path, paths, last_path,
+ rev_0_path, iterpool));
+ }
}
- /* Remove lock and lock token files. */
- return delete_lock(ub->fs, lock, pool);
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
+unlock_single(svn_fs_t *fs,
+ svn_lock_t *lock,
+ apr_pool_t *pool)
+{
+ struct unlock_baton ub;
+ svn_sort__item_t item;
+ apr_array_header_t *targets = apr_array_make(pool, 1,
+ sizeof(svn_sort__item_t));
+ item.key = lock->path;
+ item.klen = strlen(item.key);
+ item.value = (char*)lock->token;
+ APR_ARRAY_PUSH(targets, svn_sort__item_t) = item;
+
+ ub.fs = fs;
+ ub.targets = targets;
+ ub.skip_check = TRUE;
+ ub.result_pool = pool;
+
+ SVN_ERR(unlock_body(&ub, pool));
+
+ return SVN_NO_ERROR;
}
/*** Public API implementations ***/
svn_error_t *
-svn_fs_fs__lock(svn_lock_t **lock_p,
+svn_fs_fs__lock(apr_hash_t **results,
svn_fs_t *fs,
- const char *path,
- const char *token,
+ apr_hash_t *targets,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
- svn_revnum_t current_rev,
svn_boolean_t steal_lock,
- apr_pool_t *pool)
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
struct lock_baton lb;
+ apr_array_header_t *sorted_targets;
+ apr_hash_t *cannonical_targets = apr_hash_make(scratch_pool);
+ apr_hash_index_t *hi;
+ svn_error_t *err;
+ int i;
+
+ *results = apr_hash_make(result_pool);
SVN_ERR(svn_fs__check_fs(fs, TRUE));
- path = svn_fs__canonicalize_abspath(path, pool);
- lb.lock_p = lock_p;
+ /* We need to have a username attached to the fs. */
+ if (!fs->access_ctx || !fs->access_ctx->username)
+ return SVN_FS__ERR_NO_USER(fs);
+
+ /* The FS locking API allows both cannonical and non-cannonical
+ paths which means that the same cannonical path could be
+ represented more than once in the TARGETS hash. We just keep
+ one, choosing one with a token if possible. */
+ for (hi = apr_hash_first(scratch_pool, targets); hi; hi = apr_hash_next(hi))
+ {
+ const char *path = svn__apr_hash_index_key(hi);
+ const svn_fs_lock_target_t *target = svn__apr_hash_index_val(hi);
+ const svn_fs_lock_target_t *other;
+
+ path = svn_fspath__canonicalize(path, result_pool);
+ other = svn_hash_gets(cannonical_targets, path);
+
+ if (!other || (!other->token && target->token))
+ svn_hash_sets(cannonical_targets, path, target);
+ }
+
+ sorted_targets = svn_sort__hash(cannonical_targets,
+ svn_sort_compare_items_as_paths,
+ scratch_pool);
+
lb.fs = fs;
- lb.path = path;
- lb.token = token;
+ lb.targets = sorted_targets;
lb.comment = comment;
lb.is_dav_comment = is_dav_comment;
lb.expiration_date = expiration_date;
- lb.current_rev = current_rev;
lb.steal_lock = steal_lock;
- lb.pool = pool;
+ lb.result_pool = result_pool;
+
+ err = svn_fs_fs__with_write_lock(fs, lock_body, &lb, scratch_pool);
+ for (i = 0; i < lb.infos->nelts; ++i)
+ {
+ struct lock_info_t *info = &APR_ARRAY_IDX(lb.infos, i,
+ struct lock_info_t);
+ svn_fs_lock_result_t *result
+ = apr_palloc(result_pool, sizeof(svn_fs_lock_result_t));
+
+ result->lock = info->lock;
+ result->err = info->fs_err;
+
+ svn_hash_sets(*results, info->path, result);
+ }
- return svn_fs_fs__with_write_lock(fs, lock_body, &lb, pool);
+ return err;
}
@@ -963,25 +1253,67 @@ svn_fs_fs__generate_lock_token(const cha
return SVN_NO_ERROR;
}
-
svn_error_t *
-svn_fs_fs__unlock(svn_fs_t *fs,
- const char *path,
- const char *token,
+svn_fs_fs__unlock(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
svn_boolean_t break_lock,
- apr_pool_t *pool)
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
struct unlock_baton ub;
+ apr_array_header_t *sorted_targets;
+ apr_hash_t *cannonical_targets = apr_hash_make(scratch_pool);
+ apr_hash_index_t *hi;
+ svn_error_t *err;
+ int i;
+
+ *results = apr_hash_make(result_pool);
SVN_ERR(svn_fs__check_fs(fs, TRUE));
- path = svn_fs__canonicalize_abspath(path, pool);
+
+ /* We need to have a username attached to the fs. */
+ if (!fs->access_ctx || !fs->access_ctx->username)
+ return SVN_FS__ERR_NO_USER(fs);
+
+ for (hi = apr_hash_first(scratch_pool, targets); hi; hi = apr_hash_next(hi))
+ {
+ const char *path = svn__apr_hash_index_key(hi);
+ const char *token = svn__apr_hash_index_val(hi);
+ const char *other;
+
+ path = svn_fspath__canonicalize(path, result_pool);
+ other = svn_hash_gets(cannonical_targets, path);
+
+ if (!other)
+ svn_hash_sets(cannonical_targets, path, token);
+ }
+
+ sorted_targets = svn_sort__hash(cannonical_targets,
+ svn_sort_compare_items_as_paths,
+ scratch_pool);
ub.fs = fs;
- ub.path = path;
- ub.token = token;
+ ub.targets = sorted_targets;
+ ub.skip_check = FALSE;
ub.break_lock = break_lock;
+ ub.result_pool = result_pool;
+
+ err = svn_fs_fs__with_write_lock(fs, unlock_body, &ub, scratch_pool);
+ for (i = 0; i < ub.infos->nelts; ++i)
+ {
+ struct unlock_info_t *info = &APR_ARRAY_IDX(ub.infos, i,
+ struct unlock_info_t);
+ svn_fs_lock_result_t *result
+ = apr_palloc(result_pool, sizeof(svn_fs_lock_result_t));
+
+ result->lock = NULL;
+ result->err = info->fs_err;
+
+ svn_hash_sets(*results, info->path, result);
+ }
- return svn_fs_fs__with_write_lock(fs, unlock_body, &ub, pool);
+ return err;
}
Modified: subversion/trunk/subversion/libsvn_fs_fs/lock.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/lock.h?rev=1577280&r1=1577279&r2=1577280&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/lock.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/lock.h Thu Mar 13 18:47:17 2014
@@ -32,26 +32,26 @@ extern "C" {
/* These functions implement some of the calls in the FS loader
library's fs vtables. */
-svn_error_t *svn_fs_fs__lock(svn_lock_t **lock,
+svn_error_t *svn_fs_fs__lock(apr_hash_t **results,
svn_fs_t *fs,
- const char *path,
- const char *token,
+ apr_hash_t *targets,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
- svn_revnum_t current_rev,
svn_boolean_t steal_lock,
- apr_pool_t *pool);
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
svn_error_t *svn_fs_fs__generate_lock_token(const char **token,
svn_fs_t *fs,
apr_pool_t *pool);
-svn_error_t *svn_fs_fs__unlock(svn_fs_t *fs,
- const char *path,
- const char *token,
+svn_error_t *svn_fs_fs__unlock(apr_hash_t **results,
+ svn_fs_t *fs,
+ apr_hash_t *targets,
svn_boolean_t break_lock,
- apr_pool_t *pool);
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
svn_error_t *svn_fs_fs__get_lock(svn_lock_t **lock,
svn_fs_t *fs,
Propchange: subversion/trunk/subversion/libsvn_fs_x/
------------------------------------------------------------------------------
Merged /subversion/branches/fsfs-lock-many/subversion/libsvn_fs_x:r1571740-1577217
Re: Review of lock-many API [was: svn commit: r1577280 [1/3] ...]
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> In principle, the 'is_dav_comment' and 'expiration_date'
> parameters should similarly be per target,
Not "should", but rather "could be argued, at some pedantic level".
> 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 [...]
[...]
> 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.
In fact, I strongly feel we should not return error objects within the results structure. A per-target result is not, in some abstract sense, an error -- the caller didn't try to do something it is forbidden to try, and the server didn't encounter an unexpected condition or anything broken in its execution. A result such as "the lock you're trying to remove is not present" is merely one of the possible and expected outcomes.
The problem existed before this lock-many work. Wrapping the existing design in a multi-target wrapper just exacerbates the issue.
So, rather, we should return flags or other information that specifically states the result for that target. (If a higher level caller wants to consider it an error and construct an error object, that's it's business. For backwards compatibility that will obviously have to be done in some places.)
A different, practical objection to error objects constructed on the server is they are not localized to the client's human language.
- Julian
Re: Review of lock-many API
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Philip.
Philip Martin wrote:
> Julian Foad writes:
>> Index: subversion/svnserve/serve.c
>> ===================================================================
>> - /* 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?
>
> It could run out of disk space half way through.
AFAICT, if I'm reading this right, then a valid reason for not finding a result here is if svn_repos_fs_lock_many returned an error overall, having locked some but not all of the paths. Doesn't really matter why.
Breaking here seems wrong. We're processing the targets in this loop in a different order from the hash order that svn_repos_fs_lock_many() was using to lock them. Therefore we are potentially stopping before we have given responses to all the paths that were successfully locked. We would stop the response list at this point, and terminate it with a successful "done" keyword, but the response list would have too few elements. This seems arbitrary and broken.
I can think of two alternatives. We could just not generate a response list in the case where svn_repos_fs_lock_many returns an error, but simply report the error. That's effectively no worse than the current behaviour. Or we could insert an error response or some sort of place-holder entry in the list for this target, and then continue to the next target, to give a full response.
Does that make sense and would either of those options work?
- Julian
>> 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;
Re: Review of lock-many API
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>> Imagine the caller is an svn GUI that has an offline mode in which the
>> user can queue up lock requests, and the software will send them to
>> the server when it's back on line. Does the GUI's back end need to run
>> through the queued lock requests and re-group them into batches such
>> that all the lock requests in a batch have the same comment?
>>
>> Or imagine we want to teach 'svnsync' to synchronize locks: it reads
>> all the locks in the source repo and writes them to the target
>> repo. Do we want to force this software to re-group the locks into
>> batches or write them one at a time? Wouldn't it be better if it can
>> just say "Here is a bunch of locks; please write them"?
>>
>> The principle that makes sense to me is, if the semantics of the data
>> objects being written is one comment per lock, then the API should
>> match that, and not restrict the caller unnecessarily.
>
> Those examples rely on a new RA API that we don't have. Making the FS
> API support more than the RA API might future-proof the FS API but it
> might also be a dead end that is never used. We don't know exactly how
> lock replication will work and whether it will require changes to the
> storage backend, but if it does require backend changes then any FS API
> we create today may not be sufficient.
>
> Lock usernames are another problem. At present the username is passed
> via an svn_fs_access_t and stored in each lock but there is no mechanism
> to pass per-lock usernames and no mechanism to modify usernames on
> locks. Should we pass per-lock usernames with the rest of the lock
> data, or should we introduce per-lock usernames in svn_fs_access_t? The
> first is contrary to all other FS APIs, the second is a totally new API
> that nobody yet needs. I don't know the answer and since we don't need
> this feature yet I prefer not to make a decision at this time.
Ack. Okay.
> I have made svn_fs_lock_target_t opaque in the public API which should
> make it easier to add new fields in future.
OK.
Thanks.
- Julian
Re: Review of lock-many API
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> Imagine the caller is an svn GUI that has an offline mode in which the
> user can queue up lock requests, and the software will send them to
> the server when it's back on line. Does the GUI's back end need to run
> through the queued lock requests and re-group them into batches such
> that all the lock requests in a batch have the same comment?
>
> Or imagine we want to teach 'svnsync' to synchronize locks: it reads
> all the locks in the source repo and writes them to the target
> repo. Do we want to force this software to re-group the locks into
> batches or write them one at a time? Wouldn't it be better if it can
> just say "Here is a bunch of locks; please write them"?
>
> The principle that makes sense to me is, if the semantics of the data
> objects being written is one comment per lock, then the API should
> match that, and not restrict the caller unnecessarily.
Those examples rely on a new RA API that we don't have. Making the FS
API support more than the RA API might future-proof the FS API but it
might also be a dead end that is never used. We don't know exactly how
lock replication will work and whether it will require changes to the
storage backend, but if it does require backend changes then any FS API
we create today may not be sufficient.
Lock usernames are another problem. At present the username is passed
via an svn_fs_access_t and stored in each lock but there is no mechanism
to pass per-lock usernames and no mechanism to modify usernames on
locks. Should we pass per-lock usernames with the rest of the lock
data, or should we introduce per-lock usernames in svn_fs_access_t? The
first is contrary to all other FS APIs, the second is a totally new API
that nobody yet needs. I don't know the answer and since we don't need
this feature yet I prefer not to make a decision at this time.
I have made svn_fs_lock_target_t opaque in the public API which should
make it easier to add new fields in future.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Review of lock-many API
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Philip. Thanks for the responses.
(I see you've changed the API implementation to a callback pattern now.)
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>> 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.
>
> I think per-lock comments in a multi-path lock are unneccesary. I don't
> think anybody locking dozens/hundreds of files will bother defining
> different comments. Anyone who wants two or three locks with distinct
> comments can call the function multiple times.
For a high level client API, I agree with you. But the same argument doesn't make sense for the low level APIs. Here, it's not a matter of what *people* are likely to do, but another layer of *software*.
Imagine the caller is an svn GUI that has an offline mode in which the user can queue up lock requests, and the software will send them to the server when it's back on line. Does the GUI's back end need to run through the queued lock requests and re-group them into batches such that all the lock requests in a batch have the same comment?
Or imagine we want to teach 'svnsync' to synchronize locks: it reads all the locks in the source repo and writes them to the target repo. Do we want to force this software to re-group the locks into batches or write them one at a time? Wouldn't it be better if it can just say "Here is a bunch of locks; please write them"?
The principle that makes sense to me is, if the semantics of the data objects being written is one comment per lock, then the API should match that, and not restrict the caller unnecessarily.
>> 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.
>
> Eventually we will define a new request and mod_dav_svn will start
> calling the multiple path version. It will still handle the current
> LOCK request but may use the multi-path function to do it.
Eventually we will define a new request and mod_dav_svn MAY start calling the multiple path version of the API, but it will never need to set multiple locks at once, because mod_dav_svn talks to DAV via the dav_hooks_locks vtable which only deals with one path at a time. Look at the calls to svn_repos_fs_lock() in mod_dav_svn/lock.c. (A slight confusion is that the vtable methods support multiple locks per path; but that's irrelevant as our implementation doesn't.)
So it could still just as efficiently use the old single-path version of the API, if we support that, couldn't it?
[...]
>> 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? */
>
> I just copied that comment from the old code. I probably should have
> removed it.
Well, I find it very useful to have a comment at the top of a non-trivial block of code summarizing what it's supposed to be doing. How about
/* Parse PATH_REVS into TARGETS. */
here?
>> 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. */
>
> I brought up the handling of canonical paths on the dev list, it wasn't
> conclusive from my point of view so I just implemented something. Given
> that FS generally accepts non-canonical paths it is hard to know what to
> do.
Agreed. Let's just update the comment here to say we decided not to?
Please could you mark comments with '###' or similar when they are questions to reviewers about the code or design being unfinished or questionable, etc.
- Julian
Re: Review of lock-many API
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> 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'l look at doing that.
> 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.
I think per-lock comments in a multi-path lock are unneccesary. I don't
think anybody locking dozens/hundreds of files will bother defining
different comments. Anyone who wants two or three locks with distinct
comments can call the function multiple times.
> 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.
Eventually we will define a new request and mod_dav_svn will start
calling the multiple path version. It will still handle the current
LOCK request but may use the multi-path function to do it.
> 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.
I have a look at that.
> 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.
>
That's an error in libsvn_fs, libsvn_repos does it right. Fixed in
r1582845.
> + /* [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);
It's almost always possible for fs functions to return arbitrary errors
due to filesystem permissions, lack of disk space etc. This could
happen part way through creating locks. I'm not sure whether we should
attempt to run the post-hook.
> 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? */
I just copied that comment from the old code. I probably should have
removed it.
> 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. */
I brought up the handling of canonical paths on the dev list, it wasn't
conclusive from my point of view so I just implemented something. Given
that FS generally accepts non-canonical paths it is hard to know what to
do.
> 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? */
That's the way servers/repos functions work.
> 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?
It could run out of disk space half way through.
> 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)
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Review of lock-many API [was: svn commit: r1577280 [1/3] ...]
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: svn commit: r1577280 [1/3] - in /subversion/trunk: ./ notes/ subversion/include/ subversion/include/private/ subversion/libsvn_client/ subversion/libsvn_fs/ subversion/libsvn_fs_base/ subversion/libsvn_fs_fs/ subversion/libsvn_fs_x/ subversion/libsvn_r...
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
> Hi Philip...
>
>> URL: http://svn.apache.org/r1577280
>> Log:
>> Merge the fsfs-lock-many branch to trunk. [...]
>
>> * 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.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1577280 [1/3] - in /subversion/trunk: ./ notes/ subversion/include/ subversion/include/private/ subversion/libsvn_client/ subversion/libsvn_fs/ subversion/libsvn_fs_base/ subversion/libsvn_fs_fs/ subversion/libsvn_fs_x/ subversion/libsvn_r...
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Philip...
> URL: http://svn.apache.org/r1577280
> Log:
> Merge the fsfs-lock-many branch to trunk. [...]
> * 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().
And...
> Modified: subversion/trunk/subversion/include/svn_repos.h
> ==============================================================================
> -/** Like svn_fs_lock(), but invoke the @a repos's pre- and
> +/** Like svn_fs_lock2(), but invoke the @a repos's pre- and
> * post-lock hooks before and after the locking action. Use @a pool
> * for any necessary allocations.
> *
> - * If the pre-lock hook or svn_fs_lock() fails, throw the original
> - * error to caller. If an error occurs when running the post-lock
> - * hook, return the original error wrapped with
> + * 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
> + * 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 lock succeeded anyway.
> + * error, it knows that the some locks succeeded. In all cases the
> + * caller must handle 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.
> *
> + * @since New in 1.9.
> + */
> +svn_error_t *
> +svn_repos_fs_lock2(apr_hash_t **results,
> + svn_repos_t *repos,
> + apr_hash_t *targets,
> + const char *comment,
> + svn_boolean_t is_dav_comment,
> + apr_time_t expiration_date,
> + svn_boolean_t steal_lock,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
> +/** Similar to svn_repos_fs_lock2() but locks only a single path.
> + *
> * @since New in 1.2.
> */
> svn_error_t *
> @@ -2226,6 +2247,14 @@ svn_repos_fs_lock(svn_lock_t **lock,
> * @since New in 1.2.
> */
The doc strings for the old and new unlock functions need updating here.
> svn_error_t *
> +svn_repos_fs_unlock2(apr_hash_t **results,
> + svn_repos_t *repos,
> + apr_hash_t *targets,
> + svn_boolean_t break_lock,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
> +svn_error_t *
> svn_repos_fs_unlock(svn_repos_t *repos,
> const char *path,
> const char *token,
Re: r1577280 [1/3] - fsfs-lock-many
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@btopenworld.com> writes:
>> + ub.skip_check = TRUE;
>> + ub.result_pool = pool;
>> +
>> + SVN_ERR(unlock_body(&ub, pool));
>
> This leaks the error that's potentially returned in UB->fs_err, doesn't it?
No, it doesn't because skip_check is TRUE. I've added 1584706 to
explain.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: r1577280 [1/3] - fsfs-lock-many
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Philip.
> Modified: subversion/trunk/subversion/libsvn_fs_fs/lock.c
> ==============================================================================
> +static svn_error_t *
> +unlock_single(svn_fs_t *fs,
> + svn_lock_t *lock,
> + apr_pool_t *pool)
> +{
> + struct unlock_baton ub;
> + svn_sort__item_t item;
> + apr_array_header_t *targets = apr_array_make(pool, 1,
> + sizeof(svn_sort__item_t));
> + item.key = lock->path;
> + item.klen = strlen(item.key);
> + item.value = (char*)lock->token;
> + APR_ARRAY_PUSH(targets, svn_sort__item_t) = item;
> +
> + ub.fs = fs;
> + ub.targets = targets;
> + ub.skip_check = TRUE;
> + ub.result_pool = pool;
> +
> + SVN_ERR(unlock_body(&ub, pool));
This leaks the error that's potentially returned in UB->fs_err, doesn't it?
(And the same in the equivalent FSX function.)
> +
> + return SVN_NO_ERROR;
> }
- Julian