Thank you for the detailed review, Karl.
On Mon, 2005-05-30 at 16:31 -0500, kfogel@collab.net wrote:
> "VK Sameer" <sameer@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/include/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.
>
> svn_ra_svn_handle_failure_status() is a new, externally public
> function, yet its only callers are in libsvn_ra_svn/ (I checked that
> in the code as well as here in the log message).
>
> Why isn't it internally (i.e., "svn_ra_svn__handle_failure_status")
> instead of externally public?
I had to pull some code out of the current public function (used by both
svnserve/serve.c and libsvn_ra_svn/client.c) and thought that it would
be better to make that code a publicly accessible function as well.
Makes sense not to do that until it is necessary. Will fix.
> > * subversion/libsvn_ra_svn/client.c:
> > (svn_ra_lock): New function implementing 'lock-many' verb
> > (svn_ra_lock0): Old svn_ra_lock function, now the fallback for the
> > 'lock' verb.
> > (svn_ra_unlock): New function implementing 'unlock-many' verb
> > (svn_ra_unlock0): Old svn_ra_unlock function, now the fallback for the
> > 'unlock' verb.
>
> You reversed some stuff. Their names are "ra_svn_lock",
> "ra_svn_lock0", "ra_svn_unlock", and "ra_svn_unlock0"
>
> Instead of "0", could you use a more descriptive suffix, like
> "_single" or something? These are internal functions, there's no need
> to reflect our public deprecation conventions in reverse.
OK, I was definitely waiting for suggestions on that one :)
> > * subversion/libsvn_ra_svn/serve.c:
> > (lock_many): New function
> > (unlock_many): New function
>
> I haven't looked at the code yet, but I can see in the log message
> that you added two static functions to libsvn_ra_svn/serve.c (they
> must be static, because they have no "svn_..." prefixes). Since
> they're static, they can only be called from within the same file.
> Yet there are no other changes listed for that file, so no calls to
> these new functions were added. Either:
>
> a) The log message is incomplete, or
> b) These functions really are unused :-).
>
> I'm guessing it's (a). On to the code...
These get called by being present in main_commands[]. OK, will add more
verbiage.
> > Index: subversion/include/svn_ra_svn.h
> > ===================================================================
> > --- subversion/include/svn_ra_svn.h (revision 14823)
> > +++ subversion/include/svn_ra_svn.h (working copy)
> > @@ -267,6 +267,12 @@
> > apr_pool_t *pool,
> > const char *fmt, ...);
> >
> > +/** Parse the error messages in @params from a command response with a
> > + * failure status and generate errors from its contents using @pool.
> > + */
> > +svn_error_t *svn_ra_svn_handle_failure_status (apr_pool_t *pool,
> > + apr_array_header_t *params);
> > +
>
> I realize that there is some extreme (but consistent) terseness going
> on already in svn_ra_svn.h, but something about the ordering of the
> words here made it difficult to figure out what this does. I had to
> read it several times.
>
> The wording above implies that the errors will be allocated in @a pool
Ah, sorry, will fix.
> (not "@pool", by the way). But they're not; errors are always
> allocated in subpools of the global pool. @a pool is only used here
> for temporary work.
>
> Also, the function returns exactly one error, yet the doc string
> claims it "generates errors". How are these multiple errors returned?
> Are they an error chain dangling off one returned error? I guess so,
> based on the code later.
>
> Here's an attempt at a restatement:
>
> /** 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 @pool for temporary allocation.
> */
>
> Is that right?
I'm not sure. The function seems to be building an error chain using the
incoming @a pool. My ignorance of pool usage is probably woefully
obvious, but since the errors allocated using pool will be used in the
caller, is it really for temporary allocation?
> > Index: subversion/libsvn_ra_svn/client.c
> > ===================================================================
> > --- subversion/libsvn_ra_svn/client.c (revision 14823)
> > +++ subversion/libsvn_ra_svn/client.c (working copy)
> > @@ -1396,7 +1396,7 @@
> > return SVN_NO_ERROR;
> > }
> >
> > -static svn_error_t *ra_svn_lock(svn_ra_session_t *session,
> > +static svn_error_t *ra_svn_lock0(svn_ra_session_t *session,
> > apr_hash_t *path_revs,
> > const char *comment,
> > svn_boolean_t force,
>
> Whatever suffix you choose, remembre to reindent the other parameters
> here too.
OK.
> > @@ -1459,7 +1457,110 @@
> > return SVN_NO_ERROR;
> > }
> >
> > -static svn_error_t *ra_svn_unlock(svn_ra_session_t *session,
> > +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)
>
> Heh, that reversal (unlock->lock) is just an artifact of the diffing
> algorithm, I guess. Still throws me sometimes, though.
Yeah, I wasn't too happy about that either.
> > +{
> > + 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(subpool, 1, sizeof(const char*));
> > +
> > + /* (lock-many (?c) b ( (c(?r)) ...) ) */
> > + // memory leak because subpool is not being destroyed?
> > + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "w((?c)b(!",
> > + "lock-many", comment, force));
> > +
> > + for (hi = apr_hash_first(subpool, path_revs); hi; hi = apr_hash_next(hi))
> > + {
> > + const void *key;
> > + const char *path;
> > + void *val;
> > + svn_revnum_t *revnum;
> > +
> > + 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));
> > + }
>
> Why not use the usual loop pool idiom?
>
> The subpool should be used by the loop, as it already is, but also
> cleared at the top of the loop, as described in HACKING, see the
> section called "APR pool usage conventions"). The apr_hash_first()
> would have to take 'pool', then, of course.
Yes, but save_paths is used later in the function. The sequence
ra_svn_lock and ra_svn_unlock follows is:
loop #1 to build batch svn command
send batch command
loop #2 to handle batch command response
Some information built up in loop #1 using subpool is used later in loop
#2. Hence the non-idiomatic use of subpool. Does it make more sense to
use pool throughout?
Also, my pool usage may be very simplistic since I avoided using the
incoming "pool" parameter once "subpool" was created. Using "subpool"
alone throughout the function seemed to reduce the effort in figuring
out when a pool needs to be destroyed at function return points. (Not
that I have been consistent in the destruction).
> If this were the only loop, the subpool would be destroyed after the
> loop. But in this case, you may want to use it again in a later loop
> in the same function, so you wouldn't destroy it yet.
>
> > + SVN_ERR(svn_ra_svn_write_tuple(conn, subpool, "!))"));
>
> Since you already have subpool, you might as well use it here, but
> ordinarily you would use 'pool', since this is not in a loop.
Yes, I definitely need to understand pool usage better.
> However, since there are later loops that will use 'subpool', you
> would *definitely* use 'pool' here if you were going to allocate data
> that needed to live beyond the next (potential) call to
> svn_pool_clear(subpool). Thus, it may be safer to use 'pool' anyway,
> because that way this sort of unexpected-clear bug can't be introduced
> later.
>
> > + err = handle_auth_request(sess, subpool);
>
> Same.
>
> > + /* 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)
> > + {
> > + svn_pool_destroy (subpool);
> > + return ra_svn_lock0(session, path_revs, comment, force,
> > + lock_func, lock_baton, pool);
> > + }
>
> No need to destroy the subpool, because you will have been careful to
> clear it on each iteration of any loops anyway, so the amount of extra
> memory kept if you don't destroy it will be O(1) not O(N).
Aha, so that explains why I don't have to worry about subpool leaking
memory through an SVN_ERR. There's still the issue of save_paths,
however.
> > + /* Servers before 1.2 doesn't support locking. Check this here. */
> > + SVN_ERR(handle_unsupported_cmd(err, _("Server doesn't support the "
> > + "lock command")));
> > +
> > + /* svn_ra_svn_read_cmd_response unusable as it doesn't
> > + * return unparsed params. */
> > + err = svn_ra_svn_read_tuple(conn, subpool, "wl", &status, &list);
>
> Same about using 'subpool' vs 'pool'.
>
> > + /* Handle failure status explicitly */
> > + if (strcmp (status, "failure") == 0)
> > + {
> > + return svn_ra_svn_handle_failure_status (subpool, list);
> > + }
>
> Same.
>
> > + 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;
> > +
> > + 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;
> > + }
>
> And here's that second loop. Clear 'subpool' at the top of the loop,
> as per HACKING.
>
> > + svn_pool_destroy (subpool);
>
> Yes, now you can destroy it.
>
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +static svn_error_t *ra_svn_unlock0(svn_ra_session_t *session,
> > apr_hash_t *path_tokens,
> > svn_boolean_t force,
> > svn_ra_lock_callback_t lock_func,
>
> Again, better name.
>
> (If these functions were documented, it would be much easier to know
> what's going on here. I realize that this code was not internally
> documented when you arrived.)
OK, hint taken :), will document them as well.
> > @@ -1517,6 +1616,89 @@
> > 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)
> > +{
> > + 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, subpool, "w(b(!",
> > + "unlock-many",force));
> > + for (hi = apr_hash_first(subpool, path_tokens); hi; hi = apr_hash_next(hi))
> > + {
> > + const void *key;
> > + const char *path;
> > + void *val;
> > + const char *token;
> > +
> > + 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,subpool,"!))"));
> > +
> > + err = handle_auth_request(sess, subpool);
> > +
> > + /* 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)
> > + {
> > + svn_error_clear (err);
> > + return ra_svn_unlock0(session, path_tokens,
> > + force,lock_func,lock_baton,subpool);
> > + }
>
> 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.
> That would make the patch cleaner and easier to review (no old
> functions change names), and the code itself would be easier to
> follow.
I'll change from "0" to "_single". Maybe that will make it easier.
> Also, same comments about pool/subpool usage above... and below:
Here it definitely needs to be fixed.
> > + /* Servers before 1.2 don't support locking. Check this here. */
> > + SVN_ERR(handle_unsupported_cmd(err, _("Server doesn't support the "
> > + "unlock command")));
> > +
> > + /* Unknown error */
> > + if (err)
> > + return err;
> > +
> > + err = svn_ra_svn_read_cmd_response(conn, subpool, "");
> > +
> > + if (err && !SVN_ERR_IS_UNLOCK_ERROR (err))
> > + return err;
> > +
> > + for (hi = apr_hash_first(subpool, path_tokens); hi; hi = apr_hash_next(hi))
> > + {
> > + const void *key;
> > + const char *path;
> > +
> > + 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_error_clear (err);
> > + svn_pool_destroy (subpool);
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> ...same about pools/subpools.
>
> > @@ -1558,7 +1740,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")));
> >
>
> This looks like you added support for handling servers that don't know
> about "get-locks", but you *removed* support for handling servers that
> don't know about "get-lock". That's bad, and not just because it
> makes the above comment inapplicable to the code! :-)
Umm, it was actually a copy-paste fix :) since the message is in
ra_svn_get_locks(), not ra_svn_get_lock(). But as it's totally unrelated
to this patch, I'll take it out.
> > Index: subversion/libsvn_ra_svn/protocol
> > ===================================================================
> > --- subversion/libsvn_ra_svn/protocol (revision 14823)
> > +++ 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 ] )
>
> Just a note: I see that "get-locks" is already defined, albeit not in
> context here. No problem.
>
> > Index: subversion/libsvn_ra_svn/marshal.c
> > ===================================================================
> > --- subversion/libsvn_ra_svn/marshal.c (revision 14823)
> > +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> > @@ -741,17 +741,47 @@
> >
> > /* --- 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;
> > +
> > + /* 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;
> > +}
>
> Since you're looping, build a subpool and use it where you currently
> use 'pool', clearing it at the top of each iteration, etc.
OK, will do.
> (Yes, I realize you just copied this code from elsewhere.)
>
> > 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 +793,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);
> > }
>
> Yeah, that code should have been doing the loop-pool thing, although
> in practice I'm sure it never matters, since error chains don't get
> that long.
>
> > return svn_error_createf(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> > Index: subversion/svnserve/serve.c
> > ===================================================================
> > --- subversion/svnserve/serve.c (revision 14823)
> > +++ subversion/svnserve/serve.c (working copy)
> > @@ -1407,6 +1407,78 @@
> > 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)
>
> Misindentation.
Sorry, will fix.
> > +{
> > + apr_array_header_t *locks;
> > + apr_array_header_t *lock_cmds;
> > + server_baton_t *b = baton;
> > + 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;
> > + }
> > +
> > + 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 with the loop subpools. But you'll have to be careful, because
> some of the data needs to persist beyond the lifetime of the loop in
> which it was generated. Choose your pools carefully.
Hmm, can't I just stick with subpool?
> > static svn_error_t *unlock(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> > apr_array_header_t *params, void *baton)
> > {
> > @@ -1419,7 +1491,6 @@
> >
> > full_path = svn_path_join(b->fs_path, svn_path_canonicalize(path, pool),
> > pool);
> > -
> > /* Username required unless force was specified. */
> > SVN_ERR(must_have_write_access(conn, pool, b, ! force));
> >
> > @@ -1430,6 +1501,61 @@
> > 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)
>
> Misindentation.
OK.
> > +{
> > + server_baton_t *b = baton;
> > + svn_boolean_t force;
> > + apr_array_header_t *unlock_tokens;
> > + apr_array_header_t *unlock_cmds;
> > + struct unlock_cmd {
> > + const char *path;
> > + const char *token;
> > + const char *full_path;
> > + };
> > + int i;
> > +
> > + 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));
> > +
> > + 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));
> > +
> > + for (i = 0; i < unlock_tokens->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;
> > +}
>
> Gosh. I feel like there's a big code duplication going on, but I'm
> not sure if lock_many and unlock_many can be cleanly factored with
> each other. Might be worth trying; your call.
Unfortunately, the params seem ever-so-slightly incompatible. But will
take a look again.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 31 08:23:22 2005