[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4

From: <kfogel_at_collab.net>
Date: 2005-07-05 20:40:08 CEST

VK Sameer <sameer@collab.net> writes:
> Added the protocol document, removed an incorrectly commented-out
> svn_pool_clear, and did a little reformatting for ease in reading the
> patch.

There are now two mails entitled

  "Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4"

There's this one, and there's another from a few days earlier. The
other one started with this paragraph:

> Updated patch, accepts all Karl's comments. The current implementation
> of ra_svn_lock and ra_svn_unlock have been changed to ra_svn_lock_compat
> and ra_svn_unlock_compat.

I'll just review this one, under the assumption that it's really v5.
If you post another one, please call it v6 just to avoid any possible
confusion :-).

> 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_compat): Old svn_ra_lock, fallback for 1.2.x servers.
> (svn_ra_unlock_compat): Old svn_ra_unlock, fallback for 1.2.x servers.
> (svn_ra_lock): Modified to implement 'lock-many' verb.
> (svn_ra_unlock): Modified to implement 'unlock-many' verb
> * subversion/libsvn_ra_svn/protocol:
> Added grammar for 'lock-many', 'unlock-many'
> * 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.

Your prose summary implied that you renamed "ra_svn_lock" to
"ra_svn_lock_compat". The code change indicates this too. But the log
message contains no mention of "ra_svn_lock". It talks about
"svn_ra_lock" and "svn_ra_lock_compat", and same with the "_unlock"
counterparts. These are just typos, right?

Minor nits:

Capitalize start of sentence "lock function pointer changed to...".
No big deal, but hey, this is a review, so I guess I'm supposed to
mention it, right?

Also, I usually put "#" in front of issue numbers, but I'm not sure
others do that consistently. I have a fantasy that someday we'll have
a presentation filter that converts it into a link to the relevant
issue, and having a consistent syntax would make that easier. On the
other hand, it is still just vaporous fantasy.

> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 15189)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1396,13 +1396,17 @@
> return SVN_NO_ERROR;
> }
>
> -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,
> - void *lock_baton,
> - apr_pool_t *pool)
> +/* For each path in @a path_revs, send a 'lock' command to the server.
> + Used with 1.2.x series servers which support locking, but of only
> + one path at a time. ra_svn_lock, which supports 'lock-many'
> + is now the default. See svn_ra_lock comments for interface details. */
> +static svn_error_t *ra_svn_lock_compat(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)

I like the doc string! Puts the most important information up front,
then refers the reader to the right place for further details.

If you're going to use Doxygen markup, though, use it consistently.
The function names to which you refer should also be marked up (I
can't remember what the right code is, but other doc strings should
have examples). Either that, or path_revs should *not* be marked up
and you should just put it in ALL_CAPS, which is our informal
convention for parameters in non-Doxygen doc strings.

> {
> ra_svn_session_baton_t *sess = session->priv;
> svn_ra_svn_conn_t* conn = sess->conn;
> @@ -1410,8 +1414,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,20 +1461,22 @@
> return SVN_NO_ERROR;
> }

Now that the function has been renamed this way, the removal of the
comment makes sense to me, yup.

> -static svn_error_t *ra_svn_unlock(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)
> +/* For each path in @path_tokens, send an 'unlock' command to the server.
> + Used with 1.2.x series servers which support unlocking, but of only
> + one path at a time. ra_svn_unlock, which supports 'unlock-many' is
> + now the default. See svn_ra_unlock comments for interface details. */
> +static svn_error_t *ra_svn_unlock_compat(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 *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 +1521,189 @@
> return SVN_NO_ERROR;
> }

Same.

> +/* Send a 'lock-many' command to the server to lock all paths in
> + @a path_revs. See svn_ra_lock comments for interface details.
> + @since new in 1.3. */
> +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,
> + void *lock_baton,
> + apr_pool_t *pool)

Strike the word "comments" from that doc string. "Comments" by itself
usually refers to comments in code, not to doc strings. Just say "See
svn_ra_lock() for interface details."

(By the way, I just checked in svn_client.h to see what we do for
function references from Doxygenated doc strings. It looks like our
convention is to use empty parens: "svn_ra_lock()" instead of
"svn_ra_lock".)

I don't think there's any need for "@since new in ..." on internal
static functions.

> +{
> + 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 *condensed_tgt_paths;
> + condensed_tgt_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(condensed_tgt_paths, const char*) = path;
> + revnum = val;
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?r)",
> + path, *revnum));
> + }
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> +
> + err = handle_auth_request(sess, pool);
> +
> + /* 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_compat(session, path_revs, comment, force, lock_func,
> + lock_baton, pool);

The documentation for 'svn_error_t' doesn't make it clear whether
err->message can ever be NULL. Our code seems to be inconsistent on
this point. This is not a problem with your patch, it's just
something we ought to raise separately on the dev@ list. I'll bring
it up.

> + /* Unknown error */
> + if (err)
> + return err;
> +
> + /* svn_ra_svn_read_cmd_response unusable as it parses the params,
> + * instead of returning a list over which to iterate. This 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 (list, pool);
> + }

This missing verb "is" in the first clause of the first sentence made
this hard to read. How about "... is unusable because it parses the
params, ..." ?

I'm surprised you put the body of the 'if' in curly braces here, but
not above for the ra_svn_lock_compat() fallback case. Personally, I
like to use braces whenever the body is going to be greater than two
lines, except in certain rare circumstances. This is entirely a
matter of taste, nothing wrong with your patch, it was just a
surprising choice that you would use them here but not earlier.

> + if (err && !SVN_ERR_IS_LOCK_ERROR (err))
> + return err;
> +
> + for (i = 0; i < list->nelts; ++i)
> + {
> + svn_lock_t *lock;
> + const char *condensed_tgt_path;
> +
> + svn_pool_clear (subpool);
> + condensed_tgt_path = APR_ARRAY_IDX(condensed_tgt_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, condensed_tgt_path, TRUE,
> + err ? NULL : lock,
> + err, subpool); }
> +
> + if (callback_err)
> + return callback_err;
> + }
> +
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}

Excellent subpool usage.

> +/* Send an 'unlock-many' command to the server to unlock all paths in
> + @a path_tokens. See svn_ra_unlock comments for interface details.
> + @since new in 1.3.*/
> +static svn_error_t *ra_svn_unlock(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)

Same about "()" after function name. Same about "@since".

> +{
> + 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;

Mixture of space-before-paren and no-space-before-paren styles (in
function calls, I mean). This inconsistency is going on throughout
the patch, actually, but since the predominant style of the file seems
to be no-space, just go with that.

I won't call out all the examples, but three are: the svn_pool_create
and svn_pool_clear calls above, and the strcmp below.

> + if (strcmp (val, "") != 0)
> + token = val;
> + else
> + token = NULL;
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?c)",
> + path,token));
> + }
> + 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_compat(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);
> + if (callback_err)
> + return callback_err;
> + }

You are testing callback_err when it might not be initialized.
Suppose lock_func is NULL; then what value will callback_err have when
you test it? It is undefined.

> + 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,
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 15189)
> +++ subversion/libsvn_ra_svn/protocol (working copy)
> @@ -333,10 +333,19 @@
> current-rev:number ] )
> response: ( lock:lockdesc )
>
> + lock-many
> + params: ( [ comment:string ] force:bool ( (path:string [
> + current-rev:number ] ) ... ) )
> + response: ( ( lock:lockdesc ) ... )
> +
> unlock
> params: ( path:string [ token:string ] force:bool )
> response: ( )
>
> + unlock-many
> + params: ( force:bool ( ( path:string [ token:string ] ) ... ) )
> + response: ( )
> +
> get-lock
> params: ( path:string )
> response: ( [ lock:lockdesc ] )
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 15189)
> +++ 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_array_header_t *params,
> + apr_pool_t *pool)
> +{
> + const char *message, *file;
> + svn_error_t *err = NULL;
> + svn_ra_svn_item_t *elt;
> + int i;
> + apr_uint64_t apr_err, line;
> + apr_pool_t *subpool = svn_pool_create(pool);
> +
> + if (params->nelts == 0)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Empty error list"));
> +
> + /* Rebuild the error list from the end, to avoid reversing the order. */
> + for (i = params->nelts - 1; i >= 0; i--)
> + {
> + svn_pool_clear(subpool);
> + 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;
> + }
> +
> + svn_pool_destroy(subpool);
> + return err;
> +}

Again, nice subpool usage IMHO.

> 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, &params));
> 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 (params, pool);
> }

Lovely refactoring :-).

> 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 15189)
> +++ 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_array_header_t *params,
> + apr_pool_t *pool);
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/svnserve/serve.c
> ===================================================================
> --- subversion/svnserve/serve.c (revision 15189)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -1407,6 +1407,80 @@
> 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;
> + apr_pool_t *subpool;
> + 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));
> +
> + subpool = svn_pool_create(pool);
> + lock_cmds = apr_array_make(pool, locks->nelts, sizeof(struct lock_cmd));
> +
> + /* Loop through the lock commands. */
> + for (i = 0; i < locks->nelts; ++i)
> + {
> + svn_pool_clear (subpool);
> + 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, subpool, "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;
> + }

I found this loop easy to read, as well as efficient in its
allocations, because of the careful choices of subpool vs pool --
nice!

One thought: couldn't you use subpool for the svn_path_canonicalize()
call? After all, cmd->full_path is going to get allocated into pool
due to the enclosing svn_path_join() anyway, so there's no lifetime
issue.

> + 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));
> + }

What a pity that you can't use subpool for these calls, due to the
need to allocate '&cmd->l'. The only way out of that would be to do

   cmd->l = svn_deep_copy_lock(cmd->l, pool);

or something, but AFAIK we don't have such a function. Alternatively,
svn_repos_fs_lock() and svn_fs_lock() could have take two pools each
instead of one, using one for temporary allocation, and the other for
the return-by-reference parameter... No big deal, just thinking out
loud. Ignore me :-).

> + /* (success( (ccc(?c)c(?c) ... )) */
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w(!", "success"));
> + for (i = 0; i < lock_cmds->nelts; i++)
> + {
> + svn_pool_clear (subpool);
> + struct lock_cmd *cmd = &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> + SVN_ERR(write_lock(conn, subpool, cmd->l));
> + }
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)"));
> +
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *unlock(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> apr_array_header_t *params, void *baton)
> {
> @@ -1430,6 +1504,64 @@
> 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;
> + apr_pool_t *subpool;
> + 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));
> +
> + subpool = svn_pool_create (pool);
> + /* Loop through the unlock commands. */
> + for (i = 0; i < unlock_tokens->nelts; i++)
> + {
> + svn_pool_clear (subpool);
> + 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;
> + }

Same comment about svn_path_canonicalize().

Why is it that here you pass 'pool' to svn_ra_svn_parse_tuple(), but
earlier in lock_many() you passed 'subpool'?

I didn't realize it before, but I guess there's a problem with using
'subpool', because you're allocating cmd->path and cmd->token, and the
cmd structure as a whole lives beyond the loop. On the other hand,
cmd->path will never be used again outside this loop. If you used
subpool, you could just manually recopy cmd->token into more permanent
allocation...

Well, whichever way you want to fix this is fine, just be correct and
be consistent.

> + /* 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++)
> + {
> + svn_pool_clear (subpool);
> + 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, subpool));
> + }

/me nods

Okay, you can use subpool here because not returning a lock, gotcha.

> + SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> +
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *get_lock(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> apr_array_header_t *params, void *baton)
> {
> @@ -1509,7 +1641,9 @@
> { "get-locations", get_locations },
> { "get-file-revs", get_file_revs },
> { "lock", lock },
> + { "lock-many", lock_many },
> { "unlock", unlock },
> + { "unlock-many", unlock_many },
> { "get-lock", get_lock },
> { "get-locks", get_locks },
> { NULL }

I think we're getting close to commit now. Eagerly awaiting v6.

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 5 21:31:00 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.