On Thu, 7 Jul 2005 kfogel@tigris.org wrote:
Hi Karl and VK Sameer,
I've intended for some time to take a closer look at this, but haven't
been able to find the time. But this commit "forced me":-)
There are some small things. The big problem, however, is the semantic
change regarding certain non-fatal errors. (See below) Sorry for not
pointing that out earlier.
> Author: kfogel
> Date: Thu Jul 7 13:03:23 2005
> New Revision: 15287
>
> Log:
...
> * subversion/libsvn_ra_svn/client.c:
> (ra_svn_vtable): Function pointers for "lock" and "unlock" changed to
> lock_many and unlock_many, respectively.
> (ra_svn_lock_compat): Old ra_svn_lock, fallback for 1.2.x servers.
> (ra_svn_unlock_compat): Old ra_svn_unlock, fallback for 1.2.x servers.
> (ra_svn_lock): Modified to implement "lock-many" verb.
> (ra_svn_unlock): Modified to implement "unlock-many" verb
Does these really "implement" the X verb? I'd say "use the (un)lock-many
command". or something. (Also, missing period after the last sentence.)
>
> * 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.
>
The procotol spec talks about commands, not verbs. I think it is good to
be consistent.
>
> Modified: trunk/subversion/libsvn_ra_svn/client.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_svn/client.c?rev=15287&p1=trunk/subversion/libsvn_ra_svn/client.c&p2=trunk/subversion/libsvn_ra_svn/client.c&r1=15286&r2=15287
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/client.c (original)
> +++ trunk/subversion/libsvn_ra_svn/client.c Thu Jul 7 13:03:23 2005
> @@ -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() docstring for interface details. */
We don't use doxygen markup in internal documentation.
> +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)
> {
> 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;
> }
>
> -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.
Same here, and this is even incorrect:-)
> +/* Tell the server to lock all paths in @a path_revs.
And here.
> + See svn_ra_lock() 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,
> + void *lock_baton,
> + apr_pool_t *pool)
> +{
> + ra_svn_session_baton_t *sess = session->priv;
> + svn_ra_svn_conn_t* conn = sess->conn;
Misplace *.
> + 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*));
Missing space before *.
> +
> + /* (lock-many (?c) b ( (c(?r)) ...) ) */
I don't think we need this, since it is documented in the protocol, but
that's a matter of taste.
> + 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;
Missing space before *.
> + 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)
> + return ra_svn_lock_compat(session, path_revs, comment, force, lock_func,
> + lock_baton, pool);
> +
Error leak.
> + /* Unknown error */
That comment could go, it serves no purpose.
> + if (err)
> + return err;
> +
> + /* svn_ra_svn_read_cmd_response() is 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);
> +
What happens to err here. If an error was returned, status is garbage.
> + if (err && !SVN_ERR_IS_LOCK_ERROR(err))
> + return err;
> +
Parsing a tuple can never result in a locking error, so the above is
bogus.
> + 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"));
> +
Too much indentation.
> + if (lock_func)
> + callback_err = lock_func(lock_baton, condensed_tgt_path, TRUE,
> + err ? NULL : lock,
err can never be non-NULL here.
> + err, subpool);
> +
> + if (callback_err)
> + return callback_err;
What's wrong with SVN_ERR?
> + }
> +
> + svn_pool_destroy(subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Tell the server to lock all paths in @a path_tokens.
s/lock/unlock/
And no @ markup.
> + See svn_ra_unlock() for interface details. */
> +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)
> +{
> + ra_svn_session_baton_t *sess = session->priv;
> + svn_ra_svn_conn_t* conn = sess->conn;
Misplaced *.
> + apr_hash_index_t *hi;
> + apr_pool_t *subpool = svn_pool_create(pool);
> + svn_error_t *err, *callback_err = NULL;
> +
> + /* (unlock-many (b ( (c(?c)) ...) ) ) */
Unnecessary comment (IMO).
> + 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;
> +
> + if (strcmp(val, "") != 0)
> + token = val;
> + else
> + token = NULL;
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?c)", path,token));
Missing space after last comma.
> + }
> +
> + 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)
> + return ra_svn_unlock_compat(session, path_tokens, force, lock_func,
> + lock_baton, pool);
> +
Error leak.
> + /* Unknown error */
Unnecessary comment.
> + if (err)
> + return err;
> +
> + err = svn_ra_svn_read_cmd_response(conn, pool, "");
> +
> + if (err && !SVN_ERR_IS_UNLOCK_ERROR(err))
> + return err;
> +
Else, maybe leak 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;
Why not SVN_ERR?
> + }
> +
> + svn_pool_destroy(subpool);
>
> return SVN_NO_ERROR;
> }
>
There is a bigger issue with tha above two functions. They don't report
non-fatal errors per path like the other RA layers (and the compat
functions in ra_svn) do. I'll discuss this further below when we come to
the protocol document.
> Modified: trunk/subversion/libsvn_ra_svn/protocol
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_svn/protocol?rev=15287&p1=trunk/subversion/libsvn_ra_svn/protocol&p2=trunk/subversion/libsvn_ra_svn/protocol&r1=15286&r2=15287
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/protocol (original)
> +++ trunk/subversion/libsvn_ra_svn/protocol Thu Jul 7 13:03:23 2005
> @@ -333,8 +333,17 @@
> 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: ( )
>
As I said above, these commands can't return the status per path. So, if I
run
svn lock a b c
and a dn c are locked, but b was already locked, what happens? Same
applies for unlock.
What I suggest is to model the response after the response for the log
command. For example:
Server sends one command response per path, followed by "done", followed
by a final command response. If a fatal error happens, there may be less
command responses than paths in the request. (You can fix up the prose to
be more consistent with the protocol spec.)
This makes it possible for the client to differentiate between non-fatal
locking errors and a fatal error that stops processing early.
> get-lock
>
> Modified: trunk/subversion/libsvn_ra_svn/ra_svn.h
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_svn/ra_svn.h?rev=15287&p1=trunk/subversion/libsvn_ra_svn/ra_svn.h&p2=trunk/subversion/libsvn_ra_svn/ra_svn.h&r1=15286&r2=15287
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/ra_svn.h (original)
> +++ trunk/subversion/libsvn_ra_svn/ra_svn.h Thu Jul 7 13:03:23 2005
> @@ -87,6 +87,13 @@
> const char *user, const char *password,
> const char **message);
>
> +/* Return an error chain based on @a params (which contains a
No doxygen markup.
> + * 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 */
>
> Modified: trunk/subversion/svnserve/serve.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/svnserve/serve.c?rev=15287&p1=trunk/subversion/svnserve/serve.c&p2=trunk/subversion/svnserve/serve.c&r1=15286&r2=15287
> ==============================================================================
> --- trunk/subversion/svnserve/serve.c (original)
> +++ trunk/subversion/svnserve/serve.c Thu Jul 7 13:03:23 2005
> @@ -1407,6 +1407,82 @@
> 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");
\n in error message.
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, subpool, "c(?r)",
> + &cmd->path, &cmd->current_rev));
> +
cmd->path is allocated in subpool. Luckily, it isn't used outside of this
loop iteration, but why store it in the struct then?
> + cmd->full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(cmd->path, subpool),
> + pool);
> +
> + APR_ARRAY_PUSH(lock_cmds, struct lock_cmd) = *cmd;
> + }
> +
Why store all the paths/revs to lock instead of just locking them while
you parse the tuple? If you do the protocol change I proposed above (or
something similar), you could just write responses in the same loop as
well. I think this is more about code clarity than memory usage.
> + 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,
That 0 should be FALSE, since it is a boolean.
> + 0, /* No expiration time. */
> + cmd->current_rev,
> + force, pool));
Compare this with the corresponding code in svn_ra_local__lock. That code
will continue on particular errors (controlled by SVN_ERR_IS_LOCK_ERROR)).
We need to do the same here. (And there is a 0 that should be FALSE there
as well. Grrrrr!)
Also, you use pool to call svn_repos_fs_lock. You should use subpool and
then just svn_lock_dup() the returned lock if you need to store it. (This
becomes irrelevant if you make this into just one loop.)
> + }
> +
> + /* (success( (ccc(?c)c(?c) ... )) */
I don't think that comment is necessary.
> + 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 +1506,69 @@
> 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);
A blank line would be good here.
> + /* 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));
Why allocate this struct in pool and then copy it into the array? (I think
we have the same in the lock function above?)
> + 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");
> +
Newline at the end of error message.
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, subpool, "c(?c)",
> + &cmd->path, &cmd->token));
> +
Same about cmd->path as above.
> + cmd->full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(cmd->path, subpool),
> + pool);
> +
> + APR_ARRAY_PUSH(unlock_cmds, struct unlock_cmd) = *cmd;
Just to be clear, here struct is copied by value, not a pointer to it.
> + }
> +
> + /* 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));
Same about early exit on all errors.
Best Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 8 23:07:50 2005