Hi,
Here is my review of this patch. I start out with some style nits just to
get into the right mod (:-). The important stuff comes further down.
NOTE: I'm -1 on shipping 1.3 with r15287, so we need to get a variant
of this patch in or revert r15287. We don't want to have to maintain a
protocol extension that is ildesigned from the beginning.
> Follow-up for commit r15287 - svn client and server enhancements to
> send multiple locks over ra_svn.
>
In the above summary, you should say what *this* commit changes, not
> what r15287 did.
> * subversion/libsvn_ra_svn/protocol: grammar change
> (lock-many): instead of returning a single response with multiple lock
> tokens, return one lock token per path
> (unlock-many): instead of returning a single successful response, return
> each successfully unlocked path
> * subversion/libsvn_ra_svn/client.c:
> (ra_svn_lock): accept multiple responses from server for lock_many
> (ra_svn_unlock): accept multiple responses from server for unlock_many
lock_many and unlock_many should havedashes instead of underscores,
since they are protocol commands.
> * subversion/svnserve/serve.c:
> (lock_many): return multiple lock tokens, one for each lock token
It is more appropriate to say "lock description" or something, since
you're not only returning lock tokens.
> (unlock_many): return multiple responses, one for each path
You did some stylistic fixes as well, which weren't mentioned in the
log message. Since the non-stylistic part of this patch is
non-trivial, would you mind splitting this patches into two. It is
easier to review then when we can get rid of those style fixes.
[I reordered the patch, so we can start by discussing the protocol
changes.]
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 16180)
> +++ subversion/libsvn_ra_svn/protocol (working copy)
> @@ -337,7 +337,9 @@
> lock-many
> params: ( [ comment:string ] steal-lock:bool ( (path:string
> [ current-rev:number ] ) ... ) )
> - response: ( ( lock:lockdesc ) ... )
> + Before sending response, server sends lock tokens, ending with "done".
Same regarding "lock token" as in the log message.
> + lock-token: ( lock:lockdesc ) | done
The point of this change is to allow marshalling multiple errors, but
the protocol as described above doesn't allow that.
What about the following prototype:
lock-response: ( success ( lock:lockdesc ))
| ( failure ( err:error ... ) )
| done
(Note: The final "done" is necessary to allow for early exit.)
> + response: ( )
>
> unlock
> params: ( path:string [ token:string ] break-lock:bool )
> @@ -345,6 +347,8 @@
>
> unlock-many
> params: ( break-lock:bool ( ( path:string [ token:string ] ) ... ) )
> + Before sending response, server sends unlocked paths, ending with "done".
> + pre-response: ( path:string ) | done
> response: ( )
>
The same applies here.
> get-lock
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 16180)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
...
> @@ -1541,7 +1542,7 @@
> return SVN_NO_ERROR;
> }
>
> -/* Tell the server to lock all paths in @a path_revs.
> +/* Tell the server to lock all paths in path_revs.
We capital argument names in internal docstrings, like PATH_REVS. (I
won't comment on every such instance.)
> See svn_ra_lock() for interface details. */
> static svn_error_t *ra_svn_lock(svn_ra_session_t *session,
> apr_hash_t *path_revs,
> @@ -1552,18 +1553,18 @@
> apr_pool_t *pool)
> {
> ra_svn_session_baton_t *sess = session->priv;
> - svn_ra_svn_conn_t* conn = sess->conn;
> - apr_array_header_t *list;
> + svn_ra_svn_conn_t *conn = sess->conn;
> 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*));
> + const char *condensed_tgt_path;
> + svn_lock_t *lock;
>
> - /* (lock-many (?c) b ( (c(?r)) ...) ) */
> + condensed_tgt_paths = apr_array_make(pool, 1, sizeof(const char *));
> +
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w((?c)b(!", "lock-many",
> comment, steal_lock));
>
> @@ -1577,7 +1578,7 @@
> svn_pool_clear(subpool);
> apr_hash_this(hi, &key, NULL, &val);
> path = key;
> - APR_ARRAY_PUSH(condensed_tgt_paths, const char*) = path;
> + APR_ARRAY_PUSH(condensed_tgt_paths, const char *) = path;
> revnum = val;
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?r)",
> @@ -1591,56 +1592,62 @@
> /* 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, steal_lock,
> - lock_func, lock_baton, pool);
> + {
> + svn_error_clear(err);
> + return ra_svn_lock_compat(session, path_revs, comment, steal_lock,
> + lock_func, lock_baton, pool);
> + }
>
> - /* Unknown error */
> 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);
> -
> - if (err && !SVN_ERR_IS_LOCK_ERROR(err))
> - return err;
> -
> - for (i = 0; i < list->nelts; ++i)
> + /* Loop over responses to get the lock tokens. */
> + svn_boolean_t is_done = FALSE;
this variable is useless, since you break out of the loop anyway.
> + i = 0;
> + while (!is_done)
> {
> - 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);
> + SVN_ERR(svn_ra_svn_read_item(conn, pool, &elt));
>
> - if (elt->kind != SVN_RA_SVN_LIST)
> + if (elt->kind == SVN_RA_SVN_WORD && strcmp(elt->u.word, "done") == 0)
> + {
> + is_done = 1;
> + break;
> + }
> + else if (elt->kind != SVN_RA_SVN_LIST)
> return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Lock element not a list"));
> + _("Location entry not a list"));
Aha, so now we know where you copied it from:-) ^
> + else
> + {
> + err = parse_lock(elt->u.list, subpool, &lock);
> + if (err && !SVN_ERR_IS_LOCK_ERROR(err))
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, err,
> + _("Unable to parse lock data"));
parse_lock won't return any errors related to locking, just parsing
errors. this is related to the protocol discussion.
>
> - 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"));
> + condensed_tgt_path = APR_ARRAY_IDX(condensed_tgt_paths, i,
> + const char *);
> + if (lock_func)
> + callback_err = lock_func(lock_baton, condensed_tgt_path, TRUE,
> + lock, err, subpool);
>
Good! The server doesn't send the locked pathname in the case of
failure, so we can't take it from the lock then.
> - if (lock_func)
> - callback_err = lock_func(lock_baton, condensed_tgt_path, TRUE,
> - err ? NULL : lock,
> - err, subpool);
> + svn_error_clear(err);
>
> - if (callback_err)
> - return callback_err;
> + if (callback_err)
> + return callback_err;
> + }
> + i++;
> }
>
> + /* Read the response. This is so the server would have a chance to
> + * report an error. */
> + SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, ""));
> +
> svn_pool_destroy(subpool);
>
> return SVN_NO_ERROR;
> }
>
> -/* Tell the server to lock all paths in @a path_tokens.
> +/* Tell the server to unlock all paths in path_tokens.
> See svn_ra_unlock() for interface details. */
> static svn_error_t *ra_svn_unlock(svn_ra_session_t *session,
> apr_hash_t *path_tokens,
the same problem is in unlock, so I don't review it right now.
> Index: subversion/svnserve/serve.c
> ===================================================================
> --- subversion/svnserve/serve.c (revision 16180)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -1594,23 +1594,21 @@
> apr_array_header_t *params, void *baton)
> {
> server_baton_t *b = baton;
> - apr_array_header_t *locks, *lock_cmds;
> + apr_array_header_t *locks;
> const char *comment;
> svn_boolean_t steal_lock;
> 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;
> - };
> + const char *path;
> + const char *full_path;
> + svn_revnum_t current_rev;
> + svn_lock_t *l = NULL;
> + svn_error_t *err;
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?c)bl", &comment, &steal_lock,
> &locks));
>
> subpool = svn_pool_create(pool);
> - lock_cmds = apr_array_make(pool, locks->nelts, sizeof(struct lock_cmd));
>
> /* Because we can only send a single auth reply per request, we send
> a reply before parsing the lock commands. This means an authz
> @@ -1621,53 +1619,48 @@
> /* Loop through the lock commands. */
> for (i = 0; i < locks->nelts; ++i)
> {
> - struct lock_cmd *cmd = apr_pcalloc(pool, sizeof(struct lock_cmd));
> + svn_pool_clear(subpool);
> 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");
> + "Lock commands should be list of lists");
>
> - SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "c(?r)",
> - &cmd->path, &cmd->current_rev));
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "c(?r)", &path,
> + ¤t_rev));
>
> - cmd->full_path = svn_path_join(b->fs_path,
> - svn_path_canonicalize(cmd->path, subpool),
> - pool);
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(path, subpool),
> + pool);
full_path should be allocated in subpool as well.
>
> - if (! lookup_access(pool, b, svn_authz_write, cmd->full_path, TRUE))
> + if (! lookup_access(pool, b, svn_authz_write, full_path, TRUE))
> return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR,
> svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED,
> NULL, NULL), NULL);
>
> - APR_ARRAY_PUSH(lock_cmds, struct lock_cmd) = *cmd;
> - }
> + err = svn_repos_fs_lock(&l, b->repos, full_path,
> + NULL, comment, 0,
that last 0 is a boolean, and so should be FALSE for readability.
> + 0, /* No expiration time. */
> + current_rev,
> + steal_lock, pool);
>
> - /* 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,
> - steal_lock, pool));
> + if (err)
> + {
> + if (SVN_ERR_IS_LOCK_ERROR(err))
> + {
> + SVN_ERR(svn_ra_svn_write_cmd_failure(conn, subpool, err));
The protocol doesn't allow this (but it should).
> + svn_error_clear(err);
> + }
> + else
> + SVN_CMD_ERR(err);
In this case, "done" is never sent to the client, since we just return
a failure command response. this means that the client doesn't know
when the last command response is coming.
> @@ -1703,29 +1696,25 @@
> {
> server_baton_t *b = baton;
> svn_boolean_t break_lock;
> - apr_array_header_t *unlock_tokens, *unlock_cmds;
> + apr_array_header_t *unlock_tokens;
> int i;
> apr_pool_t *subpool;
> - struct unlock_cmd {
> - const char *path;
> - const char *full_path;
> - const char *token;
> - };
> + const char *path;
> + const char *full_path;
> + const char *token;
> + svn_error_t *err;
>
> SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "bl", &break_lock,
> &unlock_tokens));
>
> - unlock_cmds =
> - apr_array_make(pool, unlock_tokens->nelts, sizeof(struct unlock_cmd));
> -
> - /* Username required unless force was specified. */
> + /* Username required unless break_lock was specified. */
> SVN_ERR(must_have_access(conn, pool, b, svn_authz_write, NULL, ! break_lock));
>
> subpool = svn_pool_create(pool);
> +
> /* 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);
>
> @@ -1733,39 +1722,40 @@
>
> 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");
> + "Unlock command should be a list of lists");
>
> - SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, subpool, "c(?c)",
> - &cmd->path, &cmd->token));
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, subpool, "c(?c)", &path,
> + &token));
>
> - cmd->full_path = svn_path_join(b->fs_path,
> - svn_path_canonicalize(cmd->path, subpool),
> - pool);
> + full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(path, subpool),
> + subpool);
>
> - if (! lookup_access(pool, b, svn_authz_write, cmd->full_path,
> + if (! lookup_access(subpool, b, svn_authz_write, full_path,
> ! break_lock))
> return svn_error_create(SVN_ERR_RA_SVN_CMD_ERR,
> svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED,
> NULL, NULL), NULL);
>
> - APR_ARRAY_PUSH(unlock_cmds, struct unlock_cmd) = *cmd;
> + err = svn_repos_fs_unlock(b->repos, full_path, token, break_lock,
> + subpool);
> + if (err)
> + {
> + if (SVN_ERR_IS_UNLOCK_ERROR(err))
> + svn_error_clear(err);
Why is this error ignored?
> + else
> + SVN_CMD_ERR(err);
You need to end the "pre-response" with "done" before returning the
error.
> + }
> + else
> + /* Inform client of unlocked path. */
> + SVN_CMD_ERR(svn_ra_svn_write_tuple(conn, subpool, "c", path));
> }
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Sep 22 12:08:58 2005