VK Sameer <sameer@collab.net> writes:
> Updated log and patch attached based on reviews by Karl Fogel and Julian
> Foad.
>
> Patch for issue 2264 - svn client and server enhancement to send multiple
> locks over ra_svn. Test #22 in lock_tests.py covers this scenario.
>
> * subversion/libsvn_ra_svn/ra_svn.h
> (svn_ra_svn__handle_failure_status): New function pulled out of
> svn_ra_svn_read_cmd_response to allow lower-level handling of
> cmd response.
> * subversion/libsvn_ra_svn/marshal.c
> (svn_ra_svn__handle_failure_status): New function.
> (svn_ra_svn_read_cmd_response): Refactored to call
> svn_ra_svn__handle_failure_status.
> * subversion/libsvn_ra_svn/client.c:
> (ra_svn_vtable): lock function pointer changed to ra_svn_lock_many and
> unlock function pointer changed to ra_svn_unlock_many.
> (svn_ra_lock_many): New function implementing 'lock-many' verb.
> (svn_ra_unlock_many): New function implementing 'unlock-many' verb
> * subversion/libsvn_ra_svn/serve.c:
> (main_commands): Added "lock-many", "unlock-many" verbs and corresponding
> function pointers.
> (lock_many): New function to implement "lock-many" verb.
> (unlock_many): New function to implement "unlock-many" verb.
>
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 14906)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1396,11 +1396,13 @@
> return SVN_NO_ERROR;
> }
>
> +/* For each path in @a path_revs, send a 'lock' command to the server.
> + See svn_ra_lock comments for interface details. */
> static svn_error_t *ra_svn_lock(svn_ra_session_t *session,
> apr_hash_t *path_revs,
> const char *comment,
> svn_boolean_t force,
> - svn_ra_lock_callback_t lock_func,
> + svn_ra_lock_callback_t lock_func,
> void *lock_baton,
> apr_pool_t *pool)
> {
No big deal, but try to avoid spurious whitespace changes in a
functional patch.
Nice doc string. I was going to complain that it doesn't mention
every parameter, but then I realized it does if one follows the
directions to look at svn_ra_lock() :-).
> @@ -1410,8 +1412,6 @@
> apr_hash_index_t *hi;
> apr_pool_t *iterpool = svn_pool_create (pool);
>
> - /* ### TODO for 1.3: Send all the locks over the wire at once. This
> - loop is just a temporary shim. */
> for (hi = apr_hash_first(pool, path_revs); hi; hi = apr_hash_next(hi))
> {
> svn_lock_t *lock;
> @@ -1459,6 +1459,111 @@
> return SVN_NO_ERROR;
> }
>
Hmmm. You removed the comment saying this loop is just a temporary
shim, but the loop itself remains. So it appears this shim isn't so
temporary after all :-) ?
I think what you want to do is explain in a comment above this
function that this is a compat function, and say why. This is
discussed more below.
In http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=100933,
reviewing an earlier iteration of this patch, I wrote:
> > > Hey, I have an idea! Since the protocol words are "unlock_many" and
> > > "unlock", why don't you make those be the names of the corresponding
> > > internal functions? In other words, "ra_svn_unlock_many()", etc.
> > > Same goes for the locking functions.
> >
> > Keeping the old names was based on Ringstrom's and Fitz's comments on
> > IRC about not revving the interface. The current functions already have
> > the required multi-path interface, so there would be no difference in
> > the parameters of ra_svn_lock and ra_svn_lock_many. A new function name
> > would also need propagation to libsvn_ra code as well.
>
> Oops, I didn't understand this. These are internal static functions.
> They are not part of any published interface, and therefore there are
> no API concerns -- no "revving" would be happening here. You can call
> them anything you want; you might as well call them something that
> matches their protocol commands in an intuitive way.
>
> When you say "the current functions", which ones exactly are you
> referring to?
I don't think that question ever got answered, but now I see the
answer: you were referring to the public functions svn_ra_lock() and
svn_ra_unlock(). In that case, we have two choices, I guess:
1. Make the internal function names match the public APIs.
2. Make the internal function names match the wire protocol words.
If 1, then ra_svn_lock() and ra_svn_unlock() are the way to go. If 2,
then ra_svn_lock_many() and ra_svn_unlock_many() are the way.
Now that I understand what "the current functions" are, I think 1 is
better. Of course, somewhere you can have a comment explaining why,
historically, the 'lock' protocol word is used for locking a single
path, while 'lock-many' is used for multiple paths. (In retrospect,
it's a pity we didn't start out with 'lock-single' or something, so we
could just go with 'lock' for the multipath version. Oh well.)
> +/* @since new in 1.3.
> + Send a 'lock-many' command to the server to lock all paths in
> + @a path_revs. See svn_ra_lock comments for interface details. */
> +static svn_error_t *ra_svn_lock_many(svn_ra_session_t *session,
> + apr_hash_t *path_revs,
> + const char *comment,
> + svn_boolean_t force,
> + svn_ra_lock_callback_t lock_func,
> + void *lock_baton,
> + apr_pool_t *pool)
Internal functions do not get "@since" tags. Actually, they usually
don't get Doxygen markup at all, but there's no harm in using Doxygen
markup either.
So, continuing with the earlier theme, now we have two internal
functions:
ra_svn_lock()
ra_svn_lock_many()
They have exactly the same signature, because they both implement the
back-end of the public svn_ra_lock(). The one we try by default is
ra_svn_lock_many(), and only if it fails (due to pre-1.3 server) do we
fall back to ra_svn_lock(). Therefore, why not do what you said in
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=100905
and use
ra_svn_lock_single() /* currently 'ra_svn_lock()' in your patch */
ra_svn_lock() /* currently 'ra_svn_lock_many()' in your patch */
??
But actually ra_svn_lock_compat() would be even better, since it still
*takes* multiple paths, but just sends them one at a time, for
compatibility purposes.
> +{
> + ra_svn_session_baton_t *sess = session->priv;
> + svn_ra_svn_conn_t* conn = sess->conn;
> + apr_array_header_t *list;
> + apr_hash_index_t *hi;
> + int i;
> + svn_ra_svn_item_t *elt;
> + svn_error_t *err, *callback_err = NULL;
> + apr_pool_t *subpool = svn_pool_create (pool);
> + const char *status;
> + apr_array_header_t *save_paths; /* condensed target paths */
> + save_paths = apr_array_make(pool, 1, sizeof(const char*));
> +
> + /* (lock-many (?c) b ( (c(?r)) ...) ) */
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w((?c)b(!", "lock-many",
> + comment, force));
> +
> + for (hi = apr_hash_first(pool, path_revs); hi; hi = apr_hash_next(hi))
> + {
> + const void *key;
> + const char *path;
> + void *val;
> + svn_revnum_t *revnum;
> + svn_pool_clear (subpool);
> +
> + apr_hash_this(hi, &key, NULL, &val);
> + path = key;
> + APR_ARRAY_PUSH(save_paths, const char*) = path;
> + revnum = val;
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?r)",
> + path, *revnum));
> + }
Excellent pool/subpool technique. But, you might want to move the
blank line to before "svn_pool_clear(subpool)" instead of after.
Right now, the pool clear looks like it's part of the declarations,
which it's really not.
By the way, if you just named "save_paths" "condensed_target_paths",
you wouldn't need that comment by its declaration :-). Then you could
also initialize it in the same statement where you declare it, like
you do for other variables.
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> +
> + err = handle_auth_request(sess, pool);
Huh, so handle_auth_request() is how errors are detected after a
request? Okay. I often wish the ra_svn code had doc strings on all
the functions; but that's nothing to do with your patch.
> + /* Pre-1.3 servers don't support 'lock-many'. If that fails, fall back
> + * to 'lock'. */
> + if (err && err->apr_err == SVN_ERR_RA_SVN_UNKNOWN_CMD &&
> + strcmp (err->message, "Unknown command 'lock-many'") == 0)
> + return ra_svn_lock(session, path_revs, comment, force, lock_func,
> + lock_baton, pool);
> +
> + /* Unknown error */
> + if (err)
> + return err;
> +
> + /* svn_ra_svn_read_cmd_response unusable as it doesn't return
> + * unparsed params. This also means failure status must be handled
> + * explicitly. */
> + err = svn_ra_svn_read_tuple(conn, pool, "wl", &status, &list);
> + if (strcmp (status, "failure") == 0)
> + {
> + return svn_ra_svn__handle_failure_status (pool, list);
> + }
This comment was a little too terse for me to understand, though maybe
if I were more expert in ra_svn it would instantly make sense.
Btw, the pool usage looks excellent so far, nice choices.
> + if (err && !SVN_ERR_IS_LOCK_ERROR (err))
> + return err;
> +
> + for (i = 0; i < list->nelts; ++i)
> + {
> + svn_lock_t *lock;
> + const char *saved_path;
> + svn_pool_clear (subpool);
> +
Same thing about about blank line after rather than before.
> + saved_path = APR_ARRAY_IDX(save_paths, i, const char *);
> + elt = &APR_ARRAY_IDX(list, i, svn_ra_svn_item_t);
> +
> + if (elt->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Lock element not a list"));
> +
> + err = parse_lock(elt->u.list, subpool, &lock);
> + if (err)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, err,
> + _("unable to parse lock data"));
> +
> + if (lock_func) {
> + callback_err = lock_func(lock_baton, saved_path, TRUE,
> + err ? NULL : lock,
> + err, subpool); }
> +
> + if (callback_err)
> + return callback_err;
> + }
> +
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
Everything else looked fine; good pool/subpool choices.
All the comments I made about the lock code apply also to the unlock
code, so I won't repeat them below, I'll only say new things:
> +/* For each path in @path_tokens, send an 'unlock' command to the server.
> + See svn_ra_unlock comments for interface details. */
> static svn_error_t *ra_svn_unlock(svn_ra_session_t *session,
> apr_hash_t *path_tokens,
> svn_boolean_t force,
> @@ -1471,8 +1576,6 @@
> apr_hash_index_t *hi;
> apr_pool_t *iterpool = svn_pool_create (pool);
>
> - /* ### TODO for 1.3: Send all the lock tokens over the wire at once.
> - This loop is just a temporary shim. */
> for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> @@ -1517,6 +1620,87 @@
> return SVN_NO_ERROR;
> }
>
> +/* @since new in 1.3.
> + Send an 'unlock-many' command to the server to unlock all paths in
> + @a path_tokens. See svn_ra_unlock comments for interface details. */
> +static svn_error_t *ra_svn_unlock_many(svn_ra_session_t *session,
> + apr_hash_t *path_tokens,
> + svn_boolean_t force,
> + svn_ra_lock_callback_t lock_func,
> + void *lock_baton,
> + apr_pool_t *pool)
> +{
> + ra_svn_session_baton_t *sess = session->priv;
> + svn_ra_svn_conn_t* conn = sess->conn;
> + apr_hash_index_t *hi;
> + apr_pool_t *subpool = svn_pool_create (pool);
> + svn_error_t *err, *callback_err;
> +
> + /* (unlock-many (b ( (c(?c)) ...) ) ) */
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w(b(!", "unlock-many", force));
> +
> + for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi))
> + {
> + const void *key;
> + const char *path;
> + void *val;
> + const char *token;
> +
> + svn_pool_clear (subpool);
> + apr_hash_this(hi, &key, NULL, &val);
> + path = key;
Heh, here you did the blank line before clear, cool.
> + if (strcmp (val, "") != 0)
> + token = val;
> + else
> + token = NULL;
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?c)",
> + path,token));
> + }
Weird indentation there.
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> +
> + err = handle_auth_request(sess, pool);
> +
> + /* Pre-1.3 servers don't support 'unlock-many'. If unknown, fall back
> + * to 'unlock'.
> + */
> + if (err && err->apr_err == SVN_ERR_RA_SVN_UNKNOWN_CMD &&
> + strcmp (err->message, "Unknown command 'unlock-many'") == 0)
> + return ra_svn_unlock(session, path_tokens, force, lock_func,
> + lock_baton, pool);
> +
> + /* Unknown error */
> + if (err)
> + return err;
> +
> + err = svn_ra_svn_read_cmd_response(conn, pool, "");
> +
> + if (err && !SVN_ERR_IS_UNLOCK_ERROR (err))
> + return err;
> +
> + for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi))
> + {
> + const void *key;
> + const char *path;
> +
> + svn_pool_clear (subpool);
> + apr_hash_this(hi, &key, NULL, NULL);
> + path = key;
> +
> + if (lock_func)
> + callback_err =
> + lock_func(lock_baton, path, FALSE, NULL, err, subpool);
Why the newline? Doesn't this fit within 80 columns?
> + if (callback_err)
> + return callback_err;
> + }
> +
> + svn_error_clear (err);
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *ra_svn_get_lock(svn_ra_session_t *session,
> svn_lock_t **lock,
> const char *path,
> @@ -1558,7 +1742,7 @@
>
> /* Servers before 1.2 doesn't support locking. Check this here. */
> SVN_ERR(handle_unsupported_cmd(handle_auth_request(sess, pool),
> - _("Server doesn't support the get-lock "
> + _("Server doesn't support the get-locks "
> "command")));
Now I have learned that you are correcting a typo, which you pointed
out after I misunderstood it in earlier reviews :-). But, I don't
think you mention this in the log message?
> SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, "l", &list));
> @@ -1604,8 +1788,8 @@
> ra_svn_get_repos_root,
> ra_svn_get_locations,
> ra_svn_get_file_revs,
> - ra_svn_lock,
> - ra_svn_unlock,
> + ra_svn_lock_many,
> + ra_svn_unlock_many,
> ra_svn_get_lock,
> ra_svn_get_locks,
> };
Okay, obviously this part may change depending on what you do about
the function names.
> @@ -1646,3 +1830,4 @@
> #define INITFUNC svn_ra_svn__init
> #define COMPAT_INITFUNC svn_ra_svn_init
> #include "../libsvn_ra/wrapper_template.h"
> +
Is this just a spurious whitespace change?
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 14906)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -741,17 +741,51 @@
>
> /* --- READING AND WRITING COMMANDS AND RESPONSES --- */
>
> +svn_error_t *svn_ra_svn__handle_failure_status(apr_pool_t *pool,
> + apr_array_header_t *params)
> +{
> + const char *message, *file;
> + svn_error_t *err;
> + svn_ra_svn_item_t *elt;
> + int i;
> + apr_uint64_t apr_err, line;
> + apr_pool_t *subpool = svn_pool_create(pool);
> +
> + /* Rebuild the error list from the end, to avoid reversing the order. */
> + if (params->nelts == 0)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Empty error list"));
Shouldn't that comment go before the for-loop to which it applies, not
before this error condition check?
> + err = NULL;
> + for (i = params->nelts - 1; i >= 0; i--)
> + {
> + apr_pool_clear(subpool);
svn_pool_clear(), not apr_pool_clear()
If you're going to initialize 'err' right before the loop, instead of
at its declaration, why not do the same with 'subpool', which is
essentially a loop pool? I'm not sure I prefer one way or the other,
I just favor consistency.
> + elt = &((svn_ra_svn_item_t *) params->elts)[i];
> + if (elt->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Malformed error list"));
> + SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, subpool, "nccn", &apr_err,
> + &message, &file, &line));
> + /* The message field should have been optional, but we can't
> + easily change that, so "" means a nonexistent message. */
> + if (!*message)
> + message = NULL;
> + err = svn_error_create(apr_err, err, message);
> + err->file = apr_pstrdup(err->pool, file);
> + err->line = line;
> + }
> +
> + apr_pool_destroy(subpool);
> + return err;
> +}
> +
> svn_error_t *svn_ra_svn_read_cmd_response(svn_ra_svn_conn_t *conn,
> apr_pool_t *pool,
> const char *fmt, ...)
> {
> va_list ap;
> - const char *status, *message, *file;
> + const char *status;
> apr_array_header_t *params;
> svn_error_t *err;
> - svn_ra_svn_item_t *elt;
> - int i;
> - apr_uint64_t apr_err, line;
>
> SVN_ERR(svn_ra_svn_read_tuple(conn, pool, "wl", &status, ¶ms));
> if (strcmp(status, "success") == 0)
> @@ -763,28 +797,7 @@
> }
> else if (strcmp(status, "failure") == 0)
> {
> - /* Rebuild the error list from the end, to avoid reversing the order. */
> - if (params->nelts == 0)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Empty error list"));
> - err = NULL;
> - for (i = params->nelts - 1; i >= 0; i--)
> - {
> - elt = &((svn_ra_svn_item_t *) params->elts)[i];
> - if (elt->kind != SVN_RA_SVN_LIST)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Malformed error list"));
> - SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, pool, "nccn", &apr_err,
> - &message, &file, &line));
> - /* The message field should have been optional, but we can't
> - easily change that, so "" means a nonexistent message. */
> - if (!*message)
> - message = NULL;
> - err = svn_error_create(apr_err, err, message);
> - err->file = apr_pstrdup(err->pool, file);
> - err->line = line;
> - }
> - return err;
> + return svn_ra_svn__handle_failure_status (pool, params);
> }
Nice to see this factoring, yah.
> return svn_error_createf(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (revision 14906)
> +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> @@ -87,6 +87,13 @@
> const char *user, const char *password,
> const char **message);
>
> +/* Return an error chain based on @a params (which contains a
> + * command response indicating failure). The error chain will be
> + * in the same order as the errors indicated in @a params. Use
> + * @a pool for temporary allocations. */
> +svn_error_t *svn_ra_svn__handle_failure_status(apr_pool_t *pool,
> + apr_array_header_t *params);
> +
I think it's standard to put 'pool' as the last parameter -- or at
least, to put semantically important stuff like 'params' before a pool
argument. (And as long as we're on the subject, return-by-reference
parameters always come first by convention, but that's not an issue
here.)
> --- subversion/svnserve/serve.c (revision 14906)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -1407,6 +1407,74 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *lock_many(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> + apr_array_header_t *params, void *baton)
> +{
> + server_baton_t *b = baton;
> + apr_array_header_t *locks, *lock_cmds;
> + const char *comment;
> + svn_boolean_t force;
> + int i;
> + struct lock_cmd {
> + const char *path;
> + const char *full_path;
> + svn_revnum_t current_rev;
> + svn_lock_t *l;
> + };
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?c)bl", &comment, &force,
> + &locks));
> +
> + lock_cmds = apr_array_make(pool, locks->nelts, sizeof(struct lock_cmd));
> +
> + /* Loop through the lock commands. */
> + for (i = 0; i < locks->nelts; ++i)
> + {
> + struct lock_cmd *cmd = apr_pcalloc(pool, sizeof(struct lock_cmd));
> +
> + svn_ra_svn_item_t *item = &APR_ARRAY_IDX(locks, i, svn_ra_svn_item_t);
> +
> + if (item->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + "Lock commands should be list of lists\n");
> +
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "c(?r)",
> + &cmd->path, &cmd->current_rev));
> +
> + cmd->full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(cmd->path, pool),
> + pool);
> +
> + APR_ARRAY_PUSH(lock_cmds, struct lock_cmd) = *cmd;
> + }
If you had a subpool, you could pass it to svn_ra_svn_parse_tuple().
Since you're canonicalizing cmd->path later anyway, you wouldn't even
need additional code to copy cmd->path from the temporary pool into
the permanent one -- you've already got that code anyway.
> + SVN_ERR(must_have_write_access(conn, pool, b, TRUE));
> +
> + /* Loop through each path to be locked. */
> + for (i = 0; i < lock_cmds->nelts; i++)
> + {
> + struct lock_cmd *cmd = &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> +
> + SVN_CMD_ERR(svn_repos_fs_lock(&cmd->l, b->repos, cmd->full_path,
> + NULL, comment, 0,
> + 0, /* No expiration time. */
> + cmd->current_rev,
> + force, pool));
> + }
> +
> + /* (success( (ccc(?c)c(?c) ... )) */
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w(!", "success"));
> + for (i = 0; i < lock_cmds->nelts; i++)
> + {
> + struct lock_cmd *cmd = &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> + SVN_ERR(write_lock(conn, pool, cmd->l));
> + }
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)"));
> +
> + return SVN_NO_ERROR;
> +}
Again, if you had a subpool, you could pass it to write_lock() in the
for-loop.
> @@ -1430,6 +1498,58 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *unlock_many(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> + apr_array_header_t *params, void *baton)
> +{
> + server_baton_t *b = baton;
> + svn_boolean_t force;
> + apr_array_header_t *unlock_tokens, *unlock_cmds;
> + int i;
> + struct unlock_cmd {
> + const char *path;
> + const char *full_path;
> + const char *token;
> + };
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "bl", &force, &unlock_tokens));
> +
> + unlock_cmds =
> + apr_array_make(pool, unlock_tokens->nelts, sizeof(struct unlock_cmd));
> +
> + /* Loop through the unlock commands. */
> + for (i = 0; i < unlock_tokens->nelts; i++)
> + {
> + struct unlock_cmd *cmd = apr_pcalloc(pool, sizeof(struct unlock_cmd));
> + svn_ra_svn_item_t *item = &APR_ARRAY_IDX(unlock_tokens, i,
> + svn_ra_svn_item_t);
> + if (item->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + "Unlock command should be a list of lists\n");
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "c(?c)",
> + &cmd->path, &cmd->token));
> + cmd->full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(cmd->path, pool),
> + pool);
> + APR_ARRAY_PUSH(unlock_cmds, struct unlock_cmd) = *cmd;
> + }
> +
> + /* Username required unless force was specified. */
> + SVN_ERR(must_have_write_access(conn, pool, b, ! force));
> +
> + /* Loop through each path to be unlocked. */
> + for (i = 0; i < unlock_cmds->nelts; i++)
> + {
> + struct unlock_cmd *cmd = &APR_ARRAY_IDX(unlock_cmds, i,
> + struct unlock_cmd);
> + SVN_CMD_ERR(svn_repos_fs_unlock(b->repos, cmd->full_path,
> + cmd->token, force, pool));
> + }
> +
> + SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> +
> + return SVN_NO_ERROR;
> +}
Same comments apply as for the locking code, so I won't repeat them.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 16 19:24:30 2005