On Tue, 2005-07-05 at 13:40 -0500, kfogel@collab.net wrote:
> 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.
>
> If you post another one, please call it v6 just to avoid any possible
> confusion :-).
OK, will do.
> > 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?
Yes, will fix. I've been getting confused with svn_ra and ra_svn :)
> 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?
Sure, why not :) Fixed.
> 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.
Added # for vaporware :)
> > 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.
Added () to function reference.
> > +/* 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."
Well, I put in svn_ra_lock() docstring instead. Just because I've not
figured out how to get to the header reference from vim yet :)
> (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".)
Thanks, I've changed the comments to use that.
> I don't think there's any need for "@since new in ..." on internal
> static functions.
OK, finally removed it.
> > + /* 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, ..." ?
Fixed.
> 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.
Sorry, it was left in after a debugging session. Removed.
> > + 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.
It should be, you showed me how to do it :)
> 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.
OK. I should mention, though, the patch contains the inconsistencies
because of copy-n-paste from other functions. I would have made changes
to ra_svn_(un)lock_compat(), but didn't to avoid changes not related to
the issue in the patch.
> > + 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.
Fixed.
> > 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;
> > + }
>
> 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.
Yup, fixed.
> > +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'?
Oversight, fixed.
> 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...
IIUC, svn_ra_svn_parse_tuple() doesn't use its pool parameter. The
struct cmd* is allocated using 'pool', so there shouldn't be a problem
using subpool instead, right? IOW, cmd->token has already got memory
allocated in 'pool' and points to an existing location in memory, not
one that is created in svn_ra_svn_parse_tuple().
> I think we're getting close to commit now. Eagerly awaiting v6.
Cool, I will sent it out as soon as I'm done testing.
Thanks, as usual, for the detailed review.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 6 09:55:25 2005