lundblad@tigris.org writes:
> --- trunk/subversion/libsvn_ra_svn/client.c (original)
> +++ trunk/subversion/libsvn_ra_svn/client.c Mon Sep 26 14:57:57 2005
> @@ -1553,15 +1553,13 @@
> {
> 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;
> + svn_error_t *err, *callback_err = SVN_NO_ERROR;
> 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 *));
> + svn_lock_t *lock;
> + apr_array_header_t *list = NULL;
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w((?c)b(!", "lock-many",
> comment, steal_lock));
> @@ -1576,11 +1574,9 @@
> 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, subpool, "c(?r)", path, *revnum));
> }
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> @@ -1590,50 +1586,74 @@
> /* 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 lock information. */
> + for (hi = apr_hash_first(pool, path_revs); hi; hi = apr_hash_next(hi))
> {
A little bell went off in my head, here. The comment (and common
sense) say we're looping over the responses to get lock information --
presumably information about which locks succeeded and which locks
failed.
But we're not looping over the responses. We're looping over
path_revs, a parameter that was passed in to this function. This
doesn't quite agree with the comment, and also makes me wonder about
correctness. But maybe the loop code below will clarify. Continuing...
> - svn_lock_t *lock;
> - const char *condensed_tgt_path;
> + const void *key;
> + const char *path;
> +
> + apr_hash_this(hi, &key, NULL, NULL);
> + path = key;
Okay, so now we have a path that's one of the paths passed in.
> 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, subpool, &elt));
> +
> + if (elt->kind == SVN_RA_SVN_WORD && strcmp(elt->u.word, "done") == 0)
> + break;
Okay, that must be the terminator for the responses. See comments
below regarding that.
> if (elt->kind != SVN_RA_SVN_LIST)
> return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Lock element not a list"));
> + _("Lock response 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"));
> + SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, subpool, "wl", &status,
> + &list));
> +
> + if (strcmp(status, "failure") == 0)
> + err = svn_ra_svn__handle_failure_status(list, subpool);
> + else if (strcmp(status, "success") == 0)
> + {
> + SVN_ERR(parse_lock(list, subpool, &lock));
> + err = NULL;
> + }
> + else
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Unknown status for lock command"));
>
> if (lock_func)
> - callback_err = lock_func(lock_baton, condensed_tgt_path, TRUE,
> + callback_err = lock_func(lock_baton, path, TRUE,
> err ? NULL : lock,
> err, subpool);
> + else
> + callback_err = SVN_NO_ERROR;
> +
> + svn_error_clear(err);
>
> if (callback_err)
> return callback_err;
> }
I think I see why this works (but it feels brittle).
We're depending on the fact that the responses will be in the same
order as path_revs. Thus, when we call lock_func(), we are *assuming*
that the 'path' and 'lock' parameters match each other -- that this
lock is for that path.
In that case, why do we check for an early "done" word? If we don't
see exactly the same number of responses (success + failures) as there
were paths, shouldn't that itself be an error?
Also, is there no way we can check that the current 'path' value
matches the path of the lock?
This next bit of code...
> + /* If we didn't break early above, and the whole hash was traversed,
> + read the final "done" from the server. */
> + if (!hi)
> + {
> + SVN_ERR(svn_ra_svn_read_item(conn, pool, &elt));
> + if (elt->kind != SVN_RA_SVN_WORD || strcmp(elt->u.word, "done") != 0)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Didn't receive end marker for lock "
> + "responses"));
> + }
...seems right to me, because it expects the "done" to be where it
should be, at the end of exactly the right number of respones.
The situation with ra_svn_unlock is similar:
> @@ -1653,14 +1673,17 @@
> apr_hash_index_t *hi;
> apr_pool_t *subpool = svn_pool_create(pool);
> svn_error_t *err, *callback_err = NULL;
> + svn_ra_svn_item_t *elt;
> + const char *status = NULL;
> + apr_array_header_t *list = NULL;
> + const void *key;
> + const char *path;
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w(b(!", "unlock-many",
> break_lock));
>
> 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;
>
> @@ -1672,8 +1695,8 @@
> 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, subpool, "c(?c)", path, token));
> }
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> @@ -1684,32 +1707,70 @@
> * to 'unlock'.
> */
> if (err && err->apr_err == SVN_ERR_RA_SVN_UNKNOWN_CMD)
> - return ra_svn_unlock_compat(session, path_tokens, break_lock, lock_func,
> - lock_baton, pool);
> + {
> + svn_error_clear(err);
> + return ra_svn_unlock_compat(session, path_tokens, break_lock, lock_func,
> + lock_baton, pool);
> + }
>
> if (err)
> return err;
>
> - err = svn_ra_svn_read_cmd_response(conn, pool, "");
> -
> - if (err && !SVN_ERR_IS_UNLOCK_ERROR(err))
> - return err;
> -
> + /* Loop over responses to unlock files. */
> for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi))
> {
> - const void *key;
> - const char *path;
> -
> svn_pool_clear(subpool);
> +
> + SVN_ERR(svn_ra_svn_read_item(conn, subpool, &elt));
> +
> + if (elt->kind == SVN_RA_SVN_WORD && (strcmp(elt->u.word, "done") == 0))
> + break;
> +
> apr_hash_this(hi, &key, NULL, NULL);
> path = key;
Here we set 'path' to the current key from 'path_tokens'...
> + if (elt->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Unlock response not a list"));
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, subpool, "wl", &status,
> + &list));
> +
> + if (strcmp(status, "failure") == 0)
> + err = svn_ra_svn__handle_failure_status(list, subpool);
> + else if (strcmp(status, "success") == 0)
> + {
> + SVN_ERR(svn_ra_svn_parse_tuple(list, subpool, "c", &path));
> + err = SVN_NO_ERROR;
> + }
...but then here, we (re)set 'path' to whatever we parsed from the
successful response, overwriting the previous value. Should we at
least do a sanity check, to make sure that the received path is the
same as the path we currently hold?
Same questions about the "done" check apply here, as in ra_svn_lock().
> --- trunk/subversion/libsvn_ra_svn/protocol (original)
> +++ trunk/subversion/libsvn_ra_svn/protocol Mon Sep 26 14:57:57 2005
>
> [...]
>
> --- trunk/subversion/svnserve/serve.c (original)
> +++ trunk/subversion/svnserve/serve.c Mon Sep 26 14:57:57 2005
>
> [...]
Stuff in these two files looked good to me!
Pardon me if I'm misunderstanding some requirements of the ra_svn
protocol above. Well, pardon me and educate me :-).
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 26 22:50:06 2005