Peter N. Lundblad wrote:
> 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.
OK, makes sense.
>
>
>>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.
Yes, of course :) Will do.
>>* 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.
OK.
>>* 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.
OK, but lockdesc is already taken. lockinfo?
>> (unlock_many): return multiple responses, one for each path
>
>
> You did some stylistic fixes as well, which weren't mentioned in the
> log message.
Did you mean the comments?
> Since the non-stylistic part of this patch is
> non-trivial, would you mind splitting this patches into two.
No problem.
> 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.
OK, I think I was thinking of token as chunk.
>
>>+ 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.
I very definitely didn't grasp that point. I thought the protocol could
return an error instead of what I called a lock-token.
> 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.)
Is the status required because the server does not allow sending a
message to the client and then continuing with the command?
>>@@ -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.
OK.
>>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.)
OK.
>> 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.
Will remove.
>>+ 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:-) ^
Yes :) Will fix.
>>+ 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.
OK.
>
>>- 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.
OK.
>>
>>-/* 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.
As in lock? OK.
>>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.
OK.
>>- 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.
Sorry, keep forgetting to fix that.
>>+ 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).
OK.
>
>>+ 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.
Oh, OK, will fix.
>>@@ -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?
Will fix.
>
>>+ else
>>+ SVN_CMD_ERR(err);
>
>
> You need to end the "pre-response" with "done" before returning the
> error.
Same as in lock_many, right. Will fix.
Thanks for the detailed review, Peter. I hope to send out an updated
patch today. At present, the lock seems to be working fine, the unlock
still has some debugging to do.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Sep 23 05:00:17 2005