You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by VK Sameer <sa...@collab.net> on 2005/06/01 12:57:39 UTC
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Updated log and patch attached based on reviews by Karl Fogel and Julian
Foad.
Thanks
Sameer
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by VK Sameer <sa...@collab.net>.
On Wed, 2005-07-06 at 12:50 -0500, kfogel@collab.net wrote:
> VK Sameer <sa...@collab.net> writes:
> > > 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.
>
> Well, I'm not proposing that you fix space-before-paren
> inconsistencies in existing code -- as you say, that's not part of
> this change. But when you cut and paste code to a new place, then fix
> any inconsistencies in the new place!
But, of course! I didn't mean to imply it's not my responsibility, just
pointing out that there are formatting inconsistencies in the current
code. My bad in not noticing the inconsistencies in copy-pasted code in
any case.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> > 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.
Well, I'm not proposing that you fix space-before-paren
inconsistencies in existing code -- as you say, that's not part of
this change. But when you cut and paste code to a new place, then fix
any inconsistencies in the new place! Cutting and pasting is merely
an editing convenience for you; you're still the author of the new
code, and there is no reason to preserve formatting problems inherited
from the code's original source.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2005-07-05 at 13:40 -0500, kfogel@collab.net wrote:
> VK Sameer <sa...@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
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by Christopher Ness <ch...@nesser.org>.
On Wed, 2005-07-06 at 01:13 +0100, Philip Martin wrote:
> strncmp is neither safer nor equivalent.
Fair enough, I should have done more legwork before posting my
misconceptions.
http://www.macdonald.egate.net/CompSci/hstrings.html#strcmp
Cheers,
Chris
--
PGP Public Key: http://www.nesser.org/pgp-key/
21:57:03 up 3:44, 1 user, load average: 0.32, 0.34, 0.38
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by Philip Martin <ph...@codematters.co.uk>.
Christopher Ness <ch...@nesser.org> writes:
> On Tue, 2005-07-05 at 21:07 +0100, Julian Foad wrote:
>> kfogel@collab.net wrote:
>> > VK Sameer <sa...@collab.net> writes:
>> >>+ strcmp (err->message, "Unknown command 'lock-many'") == 0)
>
> Since you seem to know the length of the strings you want to compare in
> your 6 instances of strcmp() shouldn't you be using the safer strncmp()
> function?
strncmp is neither safer nor equivalent.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by Christopher Ness <ch...@nesser.org>.
On Tue, 2005-07-05 at 21:07 +0100, Julian Foad wrote:
> kfogel@collab.net wrote:
> > VK Sameer <sa...@collab.net> writes:
> >>+ strcmp (err->message, "Unknown command 'lock-many'") == 0)
Since you seem to know the length of the strings you want to compare in
your 6 instances of strcmp() shouldn't you be using the safer strncmp()
function?
Cheers,
Chris
--
PGP Public Key: http://www.nesser.org/pgp-key/
18:30:48 up 18 min, 2 users, load average: 0.24, 0.20, 0.16
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by VK Sameer <sa...@collab.net>.
On Wed, 2005-07-06 at 09:30 +0100, Julian Foad wrote:
> VK Sameer wrote:
> > On Tue, 2005-07-05 at 21:07 +0100, Julian Foad wrote:
> >>>VK Sameer <sa...@collab.net> writes:
> >>>>+ 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 &&
> >>>>+ strcmp (err->message, "Unknown command 'lock-many'") == 0)
> >>>>+ return ra_svn_lock_compat(session, path_revs, comment, force, lock_func,
> >>>>+ lock_baton, pool);
> >>
> >>(I'm chipping in with no knowledge again.) Is it really necessary and
> >>appropriate to do a string comparison of the error message text? It appears to
> >>me that just testing the error code would be sufficient.
> >
> > I don't have a strong opinion on this. It is mainly (ultra-?) defensive
> > programming for a garbled marshalling/unmarshalling situation. If that
> > seems far-fetched, I'll take it out.
>
> That does seem far-fetched.
>
> The way I think about issues like this is to imagine two Subversion systems
> side by side, one that includes this test, and the other identical except that
> it does not. I ask myself, "Is there any situation in which the former would
> behave better than the latter?"
>
> In this case I can't imagine any such situation. If a server responds to
> "lock-many" with UNKNOWN_CMD:"Some unexpected text", it must be a communication
> error or a broken server or a customised server (e.g. translated into another
> language). If the communication or the server is broken, I can't imagine how
> attempting a plain "lock" could do any more harm. If it is a customised
> server, then we really want to attempt the plain "lock". Therefore I believe
> this comparison (and the similar one for "unlock") should not be present.
OK, I've removed the strcmp's in latest edition of the patch.
Thanks for the use cases.
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by Julian Foad <ju...@btopenworld.com>.
VK Sameer wrote:
> On Tue, 2005-07-05 at 21:07 +0100, Julian Foad wrote:
>>>VK Sameer <sa...@collab.net> writes:
>>>>+ 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 &&
>>>>+ strcmp (err->message, "Unknown command 'lock-many'") == 0)
>>>>+ return ra_svn_lock_compat(session, path_revs, comment, force, lock_func,
>>>>+ lock_baton, pool);
>>
>>(I'm chipping in with no knowledge again.) Is it really necessary and
>>appropriate to do a string comparison of the error message text? It appears to
>>me that just testing the error code would be sufficient.
>
> I don't have a strong opinion on this. It is mainly (ultra-?) defensive
> programming for a garbled marshalling/unmarshalling situation. If that
> seems far-fetched, I'll take it out.
That does seem far-fetched.
The way I think about issues like this is to imagine two Subversion systems
side by side, one that includes this test, and the other identical except that
it does not. I ask myself, "Is there any situation in which the former would
behave better than the latter?"
In this case I can't imagine any such situation. If a server responds to
"lock-many" with UNKNOWN_CMD:"Some unexpected text", it must be a communication
error or a broken server or a customised server (e.g. translated into another
language). If the communication or the server is broken, I can't imagine how
attempting a plain "lock" could do any more harm. If it is a customised
server, then we really want to attempt the plain "lock". Therefore I believe
this comparison (and the similar one for "unlock") should not be present.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2005-07-05 at 21:07 +0100, Julian Foad wrote:
> kfogel@collab.net wrote:
> > VK Sameer <sa...@collab.net> writes:
> >>Index: subversion/libsvn_ra_svn/client.c
> >>===================================================================
> >>--- subversion/libsvn_ra_svn/client.c (revision 15189)
> >>+++ subversion/libsvn_ra_svn/client.c (working copy)
> [...]
> >>+ 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 &&
> >>+ strcmp (err->message, "Unknown command 'lock-many'") == 0)
> >>+ return ra_svn_lock_compat(session, path_revs, comment, force, lock_func,
> >>+ lock_baton, pool);
>
> (I'm chipping in with no knowledge again.) Is it really necessary and
> appropriate to do a string comparison of the error message text? It appears to
> me that just testing the error code would be sufficient.
I don't have a strong opinion on this. It is mainly (ultra-?) defensive
programming for a garbled marshalling/unmarshalling situation. If that
seems far-fetched, I'll take it out.
Thanks
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> VK Sameer <sa...@collab.net> writes:
>>Index: subversion/libsvn_ra_svn/client.c
>>===================================================================
>>--- subversion/libsvn_ra_svn/client.c (revision 15189)
>>+++ subversion/libsvn_ra_svn/client.c (working copy)
[...]
>>+ 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 &&
>>+ strcmp (err->message, "Unknown command 'lock-many'") == 0)
>>+ return ra_svn_lock_compat(session, path_revs, comment, force, lock_func,
>>+ lock_baton, pool);
(I'm chipping in with no knowledge again.) Is it really necessary and
appropriate to do a string comparison of the error message text? It appears to
me that just testing the error code would be sufficient.
- Julian
> The documentation for 'svn_error_t' doesn't make it clear whether
> err->message can ever be NULL. Our code seems to be inconsistent on
> this point. This is not a problem with your patch, it's just
> something we ought to raise separately on the dev@ list. I'll bring
> it up.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v6
Posted by VK Sameer <sa...@collab.net>.
On Thu, 2005-07-07 at 10:31 -0500, kfogel@collab.net wrote:
> Summary:
>
> This looks great. I'm going to apply it, make the minor tweaks noted
> above, and run the regression tests (and check over ethereal to make
> sure many locks are sent at once). Assuming that all passes, I'll
> commit, no need for a v7.
>
> Thanks!
>
> -Karl
This was a major education in tightening up code and being consistent.
Thanks for your patience, Karl!
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v6
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> 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): Function pointers for "lock" and "unlock" changed to
> ra_svn_lock_many and ra_svn_unlock_many, respectively.
I think you meant just "lock_many" and "unlock_many" :-). They don't
have "ra_svn_" prefixes.
> (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
> * 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.
Other than the typo above, this log message looks great to me! Of
course, at this point I'm already very familiar with the problem and
with the previous patches. But still, one reading of this log message
was enough to totally comprehend the change -- it really prepared my
mind for the patch.
I recommend putting a blank line before each new "* file..." block,
but that's a minor nit.
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 15260)
> +++ 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() docstring 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)
Beautiful!
I suppose people might get confused and think we meant "ra_svn_lock()"
where it says "svn_ra_lock()", but if they look into it they'll soon
discover that the text is accurate.
> {
> 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.
> + Used with 1.2.x series servers which support unlocking, but of only
> + one path at a time. ra_svn_unlock(), which supports 'unlock-many' is
> + now the default. See svn_ra_unlock() docstring for interface details. */
> +static svn_error_t *ra_svn_unlock_compat(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)
Likewise beautiful.
> {
> ra_svn_session_baton_t *sess = session->priv;
> svn_ra_svn_conn_t* conn = sess->conn;
> apr_hash_index_t *hi;
> apr_pool_t *iterpool = svn_pool_create (pool);
>
> - /* ### TODO for 1.3: Send all the lock tokens over the wire at once.
> - This loop is just a temporary shim. */
> for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> @@ -1517,6 +1521,183 @@
> return SVN_NO_ERROR;
> }
>
> +/* Send a 'lock-many' command to the server to lock all paths in
> + @a path_revs. See svn_ra_lock() docstring 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)
This doc string is slightly inaccurate, because this function might
actually send 'lock' instead of 'lock-many', in the fallback case.
(Yes, I consider it to be this function sending the command, even
though it calls another function to do so, because the impetus comes
from this function -- i.e., calling this function can cause the action
to happen, so it's part of this function's API).
Anyway, this can all be avoided by simply not going into
implementation details. Say instead
/* Tell the server to lock all paths in @a path_revs. See the
svn_ra_lock() doc string for interface details. */
...or something like that.
> +{
> + 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;
> + 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*));
> +
> + /* (lock-many (?c) b ( (c(?r)) ...) ) */
> + 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;
> + 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);
> +
> + /* 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)
> + {
> + 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"));
> +
I think we usually start error strings with a capital letter.
> + 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;
> +}
> +
> +/* Send an 'unlock-many' command to the server to unlock all paths in
> + @a path_tokens. See svn_ra_unlock() docstring 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)
Same comment about how going into implementation details led to a
minor inaccuracy in the doc string.
> +{
> + ra_svn_session_baton_t *sess = session->priv;
> + svn_ra_svn_conn_t* conn = sess->conn;
> + 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)) ...) ) ) */
> + 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));
> + }
> +
> + 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);
> +
> + /* 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;
> + }
> +
> + svn_pool_destroy(subpool);
> +
> + return SVN_NO_ERROR;
> +}
Looks great.
> static svn_error_t *ra_svn_get_lock(svn_ra_session_t *session,
> svn_lock_t **lock,
> const char *path,
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 15260)
> +++ subversion/libsvn_ra_svn/protocol (working copy)
> @@ -333,10 +333,19 @@
> 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: ( )
> +
> get-lock
> params: ( path:string )
> response: ( [ lock:lockdesc ] )
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 15260)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -741,17 +741,51 @@
>
> /* --- READING AND WRITING COMMANDS AND RESPONSES --- */
>
> +svn_error_t *svn_ra_svn__handle_failure_status(apr_array_header_t *params,
> + apr_pool_t *pool)
> +{
> + const char *message, *file;
> + svn_error_t *err = NULL;
> + svn_ra_svn_item_t *elt;
> + int i;
> + apr_uint64_t apr_err, line;
> + apr_pool_t *subpool = svn_pool_create(pool);
> +
> + if (params->nelts == 0)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Empty error list"));
> +
> + /* Rebuild the error list from the end, to avoid reversing the order. */
> + for (i = params->nelts - 1; i >= 0; i--)
> + {
> + svn_pool_clear(subpool);
> + elt = &((svn_ra_svn_item_t *) params->elts)[i];
> + if (elt->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Malformed error list"));
> + SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, subpool, "nccn", &apr_err,
> + &message, &file, &line));
> + /* The message field should have been optional, but we can't
> + easily change that, so "" means a nonexistent message. */
> + if (!*message)
> + message = NULL;
> + err = svn_error_create(apr_err, err, message);
> + err->file = apr_pstrdup(err->pool, file);
> + err->line = line;
> + }
> +
> + svn_pool_destroy(subpool);
> + return err;
> +}
Nice.
> svn_error_t *svn_ra_svn_read_cmd_response(svn_ra_svn_conn_t *conn,
> apr_pool_t *pool,
> const char *fmt, ...)
> {
> va_list ap;
> - const char *status, *message, *file;
> + const char *status;
> apr_array_header_t *params;
> svn_error_t *err;
> - svn_ra_svn_item_t *elt;
> - int i;
> - apr_uint64_t apr_err, line;
>
> SVN_ERR(svn_ra_svn_read_tuple(conn, pool, "wl", &status, ¶ms));
> if (strcmp(status, "success") == 0)
> @@ -763,28 +797,7 @@
> }
> else if (strcmp(status, "failure") == 0)
> {
> - /* Rebuild the error list from the end, to avoid reversing the order. */
> - if (params->nelts == 0)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Empty error list"));
> - err = NULL;
> - for (i = params->nelts - 1; i >= 0; i--)
> - {
> - elt = &((svn_ra_svn_item_t *) params->elts)[i];
> - if (elt->kind != SVN_RA_SVN_LIST)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Malformed error list"));
> - SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, pool, "nccn", &apr_err,
> - &message, &file, &line));
> - /* The message field should have been optional, but we can't
> - easily change that, so "" means a nonexistent message. */
> - if (!*message)
> - message = NULL;
> - err = svn_error_create(apr_err, err, message);
> - err->file = apr_pstrdup(err->pool, file);
> - err->line = line;
> - }
> - return err;
> + return svn_ra_svn__handle_failure_status (params, pool);
> }
>
> return svn_error_createf(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (revision 15260)
> +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> @@ -87,6 +87,13 @@
> const char *user, const char *password,
> const char **message);
>
> +/* Return an error chain based on @a params (which contains a
> + * 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 */
> Index: subversion/svnserve/serve.c
> ===================================================================
> --- subversion/svnserve/serve.c (revision 15260)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -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);
It's interesting that in this function, you do not initialize subpool
where you declare it, whereas in other functions you do. I don't
think it matters much one way or the other, just noting the
inconsistency. Why? Hemingway said it best: "Because I am a bastard."
:-)
> + 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, subpool),
> + pool);
> +
> + APR_ARRAY_PUSH(lock_cmds, struct lock_cmd) = *cmd;
> + }
> +
> + SVN_ERR(must_have_write_access(conn, pool, b, TRUE));
Technically you could use subpool for this check, but it really
doesn't matter. Maybe it's better the way you have it, to keep
subpool consistently used as a loop 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,
> + force, pool));
> + }
> +
> + /* (success( (ccc(?c)c(?c) ... )) */
> + 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, "!)"));
Similar comment about pool vs subpool.
> + 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);
> + /* Loop through the unlock commands. */
> + for (i = 0; i < unlock_tokens->nelts; i++)
Here you initialize the subpool right before the loop in which it's
used :-) Actually, this is the way I prefer, too. Again, just idle
comments, I don't think it's a big deal either way.
> + {
> + 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, subpool, "c(?c)",
> + &cmd->path, &cmd->token));
> +
> + 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;
> + }
> +
> + /* Username required unless force was specified. */
> + SVN_ERR(must_have_write_access(conn, pool, b, ! force));
Same comment as before -- i.e., just as ignorable as before :-).
> + /* 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));
> + }
> +
> + SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
Ditto.
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *get_lock(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> apr_array_header_t *params, void *baton)
> {
> @@ -1509,7 +1648,9 @@
> { "get-locations", get_locations },
> { "get-file-revs", get_file_revs },
> { "lock", lock },
> + { "lock-many", lock_many },
> { "unlock", unlock },
> + { "unlock-many", unlock_many },
> { "get-lock", get_lock },
> { "get-locks", get_locks },
> { NULL }
Summary:
This looks great. I'm going to apply it, make the minor tweaks noted
above, and run the regression tests (and check over ethereal to make
sure many locks are sent at once). Assuming that all passes, I'll
commit, no need for a v7.
Thanks!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v6
Posted by VK Sameer <sa...@collab.net>.
Updated patch, after review by Karl Fogel and Julian Foad.
Regards
Sameer
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by kf...@collab.net.
VK Sameer <sa...@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.
There are now two mails entitled
"Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4"
There's this one, and there's another from a few days earlier. The
other one started with this paragraph:
> Updated patch, accepts all Karl's comments. The current implementation
> of ra_svn_lock and ra_svn_unlock have been changed to ra_svn_lock_compat
> and ra_svn_unlock_compat.
I'll just review this one, under the assumption that it's really v5.
If you post another one, please call it v6 just to avoid any possible
confusion :-).
> 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?
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?
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.
> 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.
> {
> 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;
> }
Now that the function has been renamed this way, the removal of the
comment makes sense to me, yup.
> -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.
> + Used with 1.2.x series servers which support unlocking, but of only
> + one path at a time. ra_svn_unlock, which supports 'unlock-many' is
> + now the default. See svn_ra_unlock comments for interface details. */
> +static svn_error_t *ra_svn_unlock_compat(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;
> apr_hash_index_t *hi;
> apr_pool_t *iterpool = svn_pool_create (pool);
>
> - /* ### TODO for 1.3: Send all the lock tokens over the wire at once.
> - This loop is just a temporary shim. */
> for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> @@ -1517,6 +1521,189 @@
> return SVN_NO_ERROR;
> }
Same.
> +/* 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."
(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".)
I don't think there's any need for "@since new in ..." on internal
static functions.
> +{
> + 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;
> + 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*));
> +
> + /* (lock-many (?c) b ( (c(?r)) ...) ) */
> + 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;
> + 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 &&
> + strcmp (err->message, "Unknown command 'lock-many'") == 0)
> + return ra_svn_lock_compat(session, path_revs, comment, force, lock_func,
> + lock_baton, pool);
The documentation for 'svn_error_t' doesn't make it clear whether
err->message can ever be NULL. Our code seems to be inconsistent on
this point. This is not a problem with your patch, it's just
something we ought to raise separately on the dev@ list. I'll bring
it up.
> + /* Unknown error */
> + if (err)
> + return err;
> +
> + /* 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, ..." ?
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.
> + 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.
> +/* Send an 'unlock-many' command to the server to unlock all paths in
> + @a path_tokens. See svn_ra_unlock comments for interface details.
> + @since new in 1.3.*/
> +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)
Same about "()" after function name. Same about "@since".
> +{
> + ra_svn_session_baton_t *sess = session->priv;
> + svn_ra_svn_conn_t* conn = sess->conn;
> + apr_hash_index_t *hi;
> + apr_pool_t *subpool = svn_pool_create (pool);
> + svn_error_t *err, *callback_err;
> +
> + /* (unlock-many (b ( (c(?c)) ...) ) ) */
> + 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;
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.
I won't call out all the examples, but three are: the svn_pool_create
and svn_pool_clear calls above, and the strcmp below.
> + 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.
> + svn_error_clear (err);
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *ra_svn_get_lock(svn_ra_session_t *session,
> svn_lock_t **lock,
> const char *path,
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 15189)
> +++ subversion/libsvn_ra_svn/protocol (working copy)
> @@ -333,10 +333,19 @@
> 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: ( )
> +
> get-lock
> params: ( path:string )
> response: ( [ lock:lockdesc ] )
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 15189)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -741,17 +741,51 @@
>
> /* --- READING AND WRITING COMMANDS AND RESPONSES --- */
>
> +svn_error_t *svn_ra_svn__handle_failure_status(apr_array_header_t *params,
> + apr_pool_t *pool)
> +{
> + const char *message, *file;
> + svn_error_t *err = NULL;
> + svn_ra_svn_item_t *elt;
> + int i;
> + apr_uint64_t apr_err, line;
> + apr_pool_t *subpool = svn_pool_create(pool);
> +
> + if (params->nelts == 0)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Empty error list"));
> +
> + /* Rebuild the error list from the end, to avoid reversing the order. */
> + for (i = params->nelts - 1; i >= 0; i--)
> + {
> + svn_pool_clear(subpool);
> + elt = &((svn_ra_svn_item_t *) params->elts)[i];
> + if (elt->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Malformed error list"));
> + SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, subpool, "nccn", &apr_err,
> + &message, &file, &line));
> + /* The message field should have been optional, but we can't
> + easily change that, so "" means a nonexistent message. */
> + if (!*message)
> + message = NULL;
> + err = svn_error_create(apr_err, err, message);
> + err->file = apr_pstrdup(err->pool, file);
> + err->line = line;
> + }
> +
> + svn_pool_destroy(subpool);
> + return err;
> +}
Again, nice subpool usage IMHO.
> svn_error_t *svn_ra_svn_read_cmd_response(svn_ra_svn_conn_t *conn,
> apr_pool_t *pool,
> const char *fmt, ...)
> {
> va_list ap;
> - const char *status, *message, *file;
> + const char *status;
> apr_array_header_t *params;
> svn_error_t *err;
> - svn_ra_svn_item_t *elt;
> - int i;
> - apr_uint64_t apr_err, line;
>
> SVN_ERR(svn_ra_svn_read_tuple(conn, pool, "wl", &status, ¶ms));
> if (strcmp(status, "success") == 0)
> @@ -763,28 +797,7 @@
> }
> else if (strcmp(status, "failure") == 0)
> {
> - /* Rebuild the error list from the end, to avoid reversing the order. */
> - if (params->nelts == 0)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Empty error list"));
> - err = NULL;
> - for (i = params->nelts - 1; i >= 0; i--)
> - {
> - elt = &((svn_ra_svn_item_t *) params->elts)[i];
> - if (elt->kind != SVN_RA_SVN_LIST)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Malformed error list"));
> - SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, pool, "nccn", &apr_err,
> - &message, &file, &line));
> - /* The message field should have been optional, but we can't
> - easily change that, so "" means a nonexistent message. */
> - if (!*message)
> - message = NULL;
> - err = svn_error_create(apr_err, err, message);
> - err->file = apr_pstrdup(err->pool, file);
> - err->line = line;
> - }
> - return err;
> + return svn_ra_svn__handle_failure_status (params, pool);
> }
Lovely refactoring :-).
> return svn_error_createf(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (revision 15189)
> +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> @@ -87,6 +87,13 @@
> const char *user, const char *password,
> const char **message);
>
> +/* Return an error chain based on @a params (which contains a
> + * 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 */
> 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;
> + }
I found this loop easy to read, as well as efficient in its
allocations, because of the careful choices of subpool vs pool --
nice!
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.
> + 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,
> + 0, /* No expiration time. */
> + cmd->current_rev,
> + force, pool));
> + }
What a pity that you can't use subpool for these calls, due to the
need to allocate '&cmd->l'. The only way out of that would be to do
cmd->l = svn_deep_copy_lock(cmd->l, pool);
or something, but AFAIK we don't have such a function. Alternatively,
svn_repos_fs_lock() and svn_fs_lock() could have take two pools each
instead of one, using one for temporary allocation, and the other for
the return-by-reference parameter... No big deal, just thinking out
loud. Ignore me :-).
> + /* (success( (ccc(?c)c(?c) ... )) */
> + 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 +1504,64 @@
> 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);
> + /* 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'?
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...
Well, whichever way you want to fix this is fine, just be correct and
be consistent.
> + /* 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));
> + }
/me nods
Okay, you can use subpool here because not returning a lock, gotcha.
> + SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> +
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *get_lock(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> apr_array_header_t *params, void *baton)
> {
> @@ -1509,7 +1641,9 @@
> { "get-locations", get_locations },
> { "get-file-revs", get_file_revs },
> { "lock", lock },
> + { "lock-many", lock_many },
> { "unlock", unlock },
> + { "unlock-many", unlock_many },
> { "get-lock", get_lock },
> { "get-locks", get_locks },
> { NULL }
I think we're getting close to commit now. Eagerly awaiting v6.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by VK Sameer <sa...@collab.net>.
Added the protocol document, removed an incorrectly commented-out
svn_pool_clear, and did a little reformatting for ease in reading the
patch.
Regards
Sameer
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by VK Sameer <sa...@collab.net>.
Updated patch, accepts all Karl's comments. The current implementation
of ra_svn_lock and ra_svn_unlock have been changed to ra_svn_lock_compat
and ra_svn_unlock_compat.
Re: [PATCH]: issue #2264 - multiple locks over ra_svn - v4
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> Updated log and patch attached based on reviews by Karl Fogel and Julian
> Foad.
>
> 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_many): New function implementing 'lock-many' verb.
> (svn_ra_unlock_many): New function implementing 'unlock-many' verb
> * 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.
>
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 14906)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1396,11 +1396,13 @@
> return SVN_NO_ERROR;
> }
>
> +/* For each path in @a path_revs, send a 'lock' command to the server.
> + See svn_ra_lock comments 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,
> + svn_ra_lock_callback_t lock_func,
> void *lock_baton,
> apr_pool_t *pool)
> {
No big deal, but try to avoid spurious whitespace changes in a
functional patch.
Nice doc string. I was going to complain that it doesn't mention
every parameter, but then I realized it does if one follows the
directions to look at svn_ra_lock() :-).
> @@ -1410,8 +1412,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,6 +1459,111 @@
> return SVN_NO_ERROR;
> }
>
Hmmm. You removed the comment saying this loop is just a temporary
shim, but the loop itself remains. So it appears this shim isn't so
temporary after all :-) ?
I think what you want to do is explain in a comment above this
function that this is a compat function, and say why. This is
discussed more below.
In http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=100933,
reviewing an earlier iteration of this patch, I wrote:
> > > Hey, I have an idea! Since the protocol words are "unlock_many" and
> > > "unlock", why don't you make those be the names of the corresponding
> > > internal functions? In other words, "ra_svn_unlock_many()", etc.
> > > Same goes for the locking functions.
> >
> > Keeping the old names was based on Ringstrom's and Fitz's comments on
> > IRC about not revving the interface. The current functions already have
> > the required multi-path interface, so there would be no difference in
> > the parameters of ra_svn_lock and ra_svn_lock_many. A new function name
> > would also need propagation to libsvn_ra code as well.
>
> Oops, I didn't understand this. These are internal static functions.
> They are not part of any published interface, and therefore there are
> no API concerns -- no "revving" would be happening here. You can call
> them anything you want; you might as well call them something that
> matches their protocol commands in an intuitive way.
>
> When you say "the current functions", which ones exactly are you
> referring to?
I don't think that question ever got answered, but now I see the
answer: you were referring to the public functions svn_ra_lock() and
svn_ra_unlock(). In that case, we have two choices, I guess:
1. Make the internal function names match the public APIs.
2. Make the internal function names match the wire protocol words.
If 1, then ra_svn_lock() and ra_svn_unlock() are the way to go. If 2,
then ra_svn_lock_many() and ra_svn_unlock_many() are the way.
Now that I understand what "the current functions" are, I think 1 is
better. Of course, somewhere you can have a comment explaining why,
historically, the 'lock' protocol word is used for locking a single
path, while 'lock-many' is used for multiple paths. (In retrospect,
it's a pity we didn't start out with 'lock-single' or something, so we
could just go with 'lock' for the multipath version. Oh well.)
> +/* @since new in 1.3.
> + Send a 'lock-many' command to the server to lock all paths in
> + @a path_revs. See svn_ra_lock comments for interface details. */
> +static svn_error_t *ra_svn_lock_many(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)
Internal functions do not get "@since" tags. Actually, they usually
don't get Doxygen markup at all, but there's no harm in using Doxygen
markup either.
So, continuing with the earlier theme, now we have two internal
functions:
ra_svn_lock()
ra_svn_lock_many()
They have exactly the same signature, because they both implement the
back-end of the public svn_ra_lock(). The one we try by default is
ra_svn_lock_many(), and only if it fails (due to pre-1.3 server) do we
fall back to ra_svn_lock(). Therefore, why not do what you said in
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=100905
and use
ra_svn_lock_single() /* currently 'ra_svn_lock()' in your patch */
ra_svn_lock() /* currently 'ra_svn_lock_many()' in your patch */
??
But actually ra_svn_lock_compat() would be even better, since it still
*takes* multiple paths, but just sends them one at a time, for
compatibility purposes.
> +{
> + 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;
> + apr_pool_t *subpool = svn_pool_create (pool);
> + const char *status;
> + apr_array_header_t *save_paths; /* condensed target paths */
> + save_paths = apr_array_make(pool, 1, sizeof(const char*));
> +
> + /* (lock-many (?c) b ( (c(?r)) ...) ) */
> + 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(save_paths, const char*) = path;
> + revnum = val;
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?r)",
> + path, *revnum));
> + }
Excellent pool/subpool technique. But, you might want to move the
blank line to before "svn_pool_clear(subpool)" instead of after.
Right now, the pool clear looks like it's part of the declarations,
which it's really not.
By the way, if you just named "save_paths" "condensed_target_paths",
you wouldn't need that comment by its declaration :-). Then you could
also initialize it in the same statement where you declare it, like
you do for other variables.
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!))"));
> +
> + err = handle_auth_request(sess, pool);
Huh, so handle_auth_request() is how errors are detected after a
request? Okay. I often wish the ra_svn code had doc strings on all
the functions; but that's nothing to do with your patch.
> + /* 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 &&
> + strcmp (err->message, "Unknown command 'lock-many'") == 0)
> + return ra_svn_lock(session, path_revs, comment, force, lock_func,
> + lock_baton, pool);
> +
> + /* Unknown error */
> + if (err)
> + return err;
> +
> + /* svn_ra_svn_read_cmd_response unusable as it doesn't return
> + * unparsed params. This also 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 (pool, list);
> + }
This comment was a little too terse for me to understand, though maybe
if I were more expert in ra_svn it would instantly make sense.
Btw, the pool usage looks excellent so far, nice choices.
> + if (err && !SVN_ERR_IS_LOCK_ERROR (err))
> + return err;
> +
> + for (i = 0; i < list->nelts; ++i)
> + {
> + svn_lock_t *lock;
> + const char *saved_path;
> + svn_pool_clear (subpool);
> +
Same thing about about blank line after rather than before.
> + saved_path = APR_ARRAY_IDX(save_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, saved_path, TRUE,
> + err ? NULL : lock,
> + err, subpool); }
> +
> + if (callback_err)
> + return callback_err;
> + }
> +
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
Everything else looked fine; good pool/subpool choices.
All the comments I made about the lock code apply also to the unlock
code, so I won't repeat them below, I'll only say new things:
> +/* For each path in @path_tokens, send an 'unlock' command to the server.
> + See svn_ra_unlock comments for interface details. */
> static svn_error_t *ra_svn_unlock(svn_ra_session_t *session,
> apr_hash_t *path_tokens,
> svn_boolean_t force,
> @@ -1471,8 +1576,6 @@
> apr_hash_index_t *hi;
> apr_pool_t *iterpool = svn_pool_create (pool);
>
> - /* ### TODO for 1.3: Send all the lock tokens over the wire at once.
> - This loop is just a temporary shim. */
> for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> @@ -1517,6 +1620,87 @@
> return SVN_NO_ERROR;
> }
>
> +/* @since new in 1.3.
> + Send an 'unlock-many' command to the server to unlock all paths in
> + @a path_tokens. See svn_ra_unlock comments for interface details. */
> +static svn_error_t *ra_svn_unlock_many(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;
> + apr_hash_index_t *hi;
> + apr_pool_t *subpool = svn_pool_create (pool);
> + svn_error_t *err, *callback_err;
> +
> + /* (unlock-many (b ( (c(?c)) ...) ) ) */
> + 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;
Heh, here you did the blank line before clear, cool.
> + if (strcmp (val, "") != 0)
> + token = val;
> + else
> + token = NULL;
> +
> + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "c(?c)",
> + path,token));
> + }
Weird indentation there.
> + 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(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);
Why the newline? Doesn't this fit within 80 columns?
> + if (callback_err)
> + return callback_err;
> + }
> +
> + svn_error_clear (err);
> + svn_pool_destroy (subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *ra_svn_get_lock(svn_ra_session_t *session,
> svn_lock_t **lock,
> const char *path,
> @@ -1558,7 +1742,7 @@
>
> /* Servers before 1.2 doesn't support locking. Check this here. */
> SVN_ERR(handle_unsupported_cmd(handle_auth_request(sess, pool),
> - _("Server doesn't support the get-lock "
> + _("Server doesn't support the get-locks "
> "command")));
Now I have learned that you are correcting a typo, which you pointed
out after I misunderstood it in earlier reviews :-). But, I don't
think you mention this in the log message?
> SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, "l", &list));
> @@ -1604,8 +1788,8 @@
> ra_svn_get_repos_root,
> ra_svn_get_locations,
> ra_svn_get_file_revs,
> - ra_svn_lock,
> - ra_svn_unlock,
> + ra_svn_lock_many,
> + ra_svn_unlock_many,
> ra_svn_get_lock,
> ra_svn_get_locks,
> };
Okay, obviously this part may change depending on what you do about
the function names.
> @@ -1646,3 +1830,4 @@
> #define INITFUNC svn_ra_svn__init
> #define COMPAT_INITFUNC svn_ra_svn_init
> #include "../libsvn_ra/wrapper_template.h"
> +
Is this just a spurious whitespace change?
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 14906)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -741,17 +741,51 @@
>
> /* --- READING AND WRITING COMMANDS AND RESPONSES --- */
>
> +svn_error_t *svn_ra_svn__handle_failure_status(apr_pool_t *pool,
> + apr_array_header_t *params)
> +{
> + const char *message, *file;
> + svn_error_t *err;
> + svn_ra_svn_item_t *elt;
> + int i;
> + apr_uint64_t apr_err, line;
> + apr_pool_t *subpool = svn_pool_create(pool);
> +
> + /* Rebuild the error list from the end, to avoid reversing the order. */
> + if (params->nelts == 0)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Empty error list"));
Shouldn't that comment go before the for-loop to which it applies, not
before this error condition check?
> + err = NULL;
> + for (i = params->nelts - 1; i >= 0; i--)
> + {
> + apr_pool_clear(subpool);
svn_pool_clear(), not apr_pool_clear()
If you're going to initialize 'err' right before the loop, instead of
at its declaration, why not do the same with 'subpool', which is
essentially a loop pool? I'm not sure I prefer one way or the other,
I just favor consistency.
> + elt = &((svn_ra_svn_item_t *) params->elts)[i];
> + if (elt->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + _("Malformed error list"));
> + SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, subpool, "nccn", &apr_err,
> + &message, &file, &line));
> + /* The message field should have been optional, but we can't
> + easily change that, so "" means a nonexistent message. */
> + if (!*message)
> + message = NULL;
> + err = svn_error_create(apr_err, err, message);
> + err->file = apr_pstrdup(err->pool, file);
> + err->line = line;
> + }
> +
> + apr_pool_destroy(subpool);
> + return err;
> +}
> +
> svn_error_t *svn_ra_svn_read_cmd_response(svn_ra_svn_conn_t *conn,
> apr_pool_t *pool,
> const char *fmt, ...)
> {
> va_list ap;
> - const char *status, *message, *file;
> + const char *status;
> apr_array_header_t *params;
> svn_error_t *err;
> - svn_ra_svn_item_t *elt;
> - int i;
> - apr_uint64_t apr_err, line;
>
> SVN_ERR(svn_ra_svn_read_tuple(conn, pool, "wl", &status, ¶ms));
> if (strcmp(status, "success") == 0)
> @@ -763,28 +797,7 @@
> }
> else if (strcmp(status, "failure") == 0)
> {
> - /* Rebuild the error list from the end, to avoid reversing the order. */
> - if (params->nelts == 0)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Empty error list"));
> - err = NULL;
> - for (i = params->nelts - 1; i >= 0; i--)
> - {
> - elt = &((svn_ra_svn_item_t *) params->elts)[i];
> - if (elt->kind != SVN_RA_SVN_LIST)
> - return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> - _("Malformed error list"));
> - SVN_ERR(svn_ra_svn_parse_tuple(elt->u.list, pool, "nccn", &apr_err,
> - &message, &file, &line));
> - /* The message field should have been optional, but we can't
> - easily change that, so "" means a nonexistent message. */
> - if (!*message)
> - message = NULL;
> - err = svn_error_create(apr_err, err, message);
> - err->file = apr_pstrdup(err->pool, file);
> - err->line = line;
> - }
> - return err;
> + return svn_ra_svn__handle_failure_status (pool, params);
> }
Nice to see this factoring, yah.
> return svn_error_createf(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> Index: subversion/libsvn_ra_svn/ra_svn.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn.h (revision 14906)
> +++ subversion/libsvn_ra_svn/ra_svn.h (working copy)
> @@ -87,6 +87,13 @@
> const char *user, const char *password,
> const char **message);
>
> +/* Return an error chain based on @a params (which contains a
> + * 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_pool_t *pool,
> + apr_array_header_t *params);
> +
I think it's standard to put 'pool' as the last parameter -- or at
least, to put semantically important stuff like 'params' before a pool
argument. (And as long as we're on the subject, return-by-reference
parameters always come first by convention, but that's not an issue
here.)
> --- subversion/svnserve/serve.c (revision 14906)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -1407,6 +1407,74 @@
> 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;
> + 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));
> +
> + lock_cmds = apr_array_make(pool, locks->nelts, sizeof(struct lock_cmd));
> +
> + /* Loop through the lock commands. */
> + for (i = 0; i < locks->nelts; ++i)
> + {
> + 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, pool, "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;
> + }
If you had a subpool, you could pass it to svn_ra_svn_parse_tuple().
Since you're canonicalizing cmd->path later anyway, you wouldn't even
need additional code to copy cmd->path from the temporary pool into
the permanent one -- you've already got that code anyway.
> + 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,
> + 0, /* No expiration time. */
> + cmd->current_rev,
> + force, pool));
> + }
> +
> + /* (success( (ccc(?c)c(?c) ... )) */
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w(!", "success"));
> + for (i = 0; i < lock_cmds->nelts; i++)
> + {
> + struct lock_cmd *cmd = &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> + SVN_ERR(write_lock(conn, pool, cmd->l));
> + }
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)"));
> +
> + return SVN_NO_ERROR;
> +}
Again, if you had a subpool, you could pass it to write_lock() in the
for-loop.
> @@ -1430,6 +1498,58 @@
> 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;
> + 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));
> +
> + /* 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);
> + 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;
> + }
> +
> + /* 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++)
> + {
> + 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, pool));
> + }
> +
> + SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> +
> + return SVN_NO_ERROR;
> +}
Same comments apply as for the locking code, so I won't repeat them.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org