"Peter N. Lundblad" <peter@famlundblad.se> writes:
> I've intended for some time to take a closer look at this, but haven't
> been able to find the time. But this commit "forced me":-)
>
> There are some small things. The big problem, however, is the semantic
> change regarding certain non-fatal errors. (See below) Sorry for not
> pointing that out earlier.
That's okay; problems are as easy to fix after commit as before...
> > Log:
> ...
> > * subversion/libsvn_ra_svn/client.c:
> > (ra_svn_vtable): Function pointers for "lock" and "unlock" changed to
> > lock_many and unlock_many, respectively.
> > (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
>
> Does these really "implement" the X verb? I'd say "use the (un)lock-many
> command". or something. (Also, missing period after the last sentence.)
>
> >
> > * 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.
> >
> The procotol spec talks about commands, not verbs. I think it is good to
> be consistent.
Agreed on all; we'll fix up later (Sameer and I can negotiate as to
who will do it :-) ).
> > Modified: trunk/subversion/libsvn_ra_svn/client.c
> > Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_svn/client.c?rev=15287&p1=trunk/subversion/libsvn_ra_svn/client.c&p2=trunk/subversion/libsvn_ra_svn/client.c&r1=15286&r2=15287
> > ==============================================================================
> > --- trunk/subversion/libsvn_ra_svn/client.c (original)
> > +++ trunk/subversion/libsvn_ra_svn/client.c Thu Jul 7 13:03:23 2005
> > @@ -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. */
>
> We don't use doxygen markup in internal documentation.
>
> > +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)
> > {
> > 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.
>
> Same here, and this is even incorrect:-)
>
> > +/* Tell the server to lock all paths in @a path_revs.
>
> And here.
>
> > + See svn_ra_lock() 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)
> > +{
> > + ra_svn_session_baton_t *sess = session->priv;
> > + svn_ra_svn_conn_t* conn = sess->conn;
>
> Misplace *.
>
> > + 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*));
>
> Missing space before *.
>
> > +
> > + /* (lock-many (?c) b ( (c(?r)) ...) ) */
>
> I don't think we need this, since it is documented in the protocol, but
> that's a matter of taste.
>
> > + 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;
>
> Missing space before *.
Okay, all the above we can fix up -- thanks for noticing.
> > + 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);
> > +
>
> Error leak.
Gotcha, will fix.
> > + /* Unknown error */
>
> That comment could go, it serves no purpose.
>
It helped me, but it's a matter of taste I suppose.
> > + 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);
> > +
> What happens to err here. If an error was returned, status is garbage.
Oops.
> > + if (err && !SVN_ERR_IS_LOCK_ERROR(err))
> > + return err;
> > +
> Parsing a tuple can never result in a locking error, so the above is
> bogus.
Double oops.
> > + 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"));
> > +
>
> Too much indentation.
Hey, good point.
> > + if (lock_func)
> > + callback_err = lock_func(lock_baton, condensed_tgt_path, TRUE,
> > + err ? NULL : lock,
>
> err can never be non-NULL here.
Quite true.
> > + err, subpool);
> > +
> > + if (callback_err)
> > + return callback_err;
>
> What's wrong with SVN_ERR?
Good point.
Okay, at this point I'm going to stop going over this line by line and
ask VK Sameer if he would like to produce a followup patch addressing
all the issues you raise in this mail? That's my idea of "negotiate":
ask Sameer if he'll do it :-). Sameer, if you don't have time, that's
fine, then I'll do it, but you're more familiar with the code and the
protocol.
> There is a bigger issue with tha above two functions. They don't report
> non-fatal errors per path like the other RA layers (and the compat
> functions in ra_svn) do. I'll discuss this further below when we come to
> the protocol document.
>
>
> > Modified: trunk/subversion/libsvn_ra_svn/protocol
> > Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_svn/protocol?rev=15287&p1=trunk/subversion/libsvn_ra_svn/protocol&p2=trunk/subversion/libsvn_ra_svn/protocol&r1=15286&r2=15287
> > ==============================================================================
> > --- trunk/subversion/libsvn_ra_svn/protocol (original)
> > +++ trunk/subversion/libsvn_ra_svn/protocol Thu Jul 7 13:03:23 2005
> > @@ -333,8 +333,17 @@
> > 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: ( )
> >
>
> As I said above, these commands can't return the status per path. So, if I
> run
> svn lock a b c
> and a dn c are locked, but b was already locked, what happens? Same
> applies for unlock.
>
> What I suggest is to model the response after the response for the log
> command. For example:
> Server sends one command response per path, followed by "done", followed
> by a final command response. If a fatal error happens, there may be less
> command responses than paths in the request. (You can fix up the prose to
> be more consistent with the protocol spec.)
> This makes it possible for the client to differentiate between non-fatal
> locking errors and a fatal error that stops processing early.
Thanks so much for the analysis, Peter.
Sameer, want to have a go at it?
> > + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, subpool, "c(?r)",
> > + &cmd->path, &cmd->current_rev));
> > +
>
> cmd->path is allocated in subpool. Luckily, it isn't used outside of this
> loop iteration, but why store it in the struct then?
Heh, we had the pool-vs-subpool discussion during review, but I didn't
think to ask "why store it in the struct?".
> > + 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;
> > + }
> > +
> Why store all the paths/revs to lock instead of just locking them while
> you parse the tuple? If you do the protocol change I proposed above (or
> something similar), you could just write responses in the same loop as
> well. I think this is more about code clarity than memory usage.
But it would improve pool usage too, which is good.
> > + 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,
>
> That 0 should be FALSE, since it is a boolean.
Excellent point.
> > + 0, /* No expiration time. */
> > + cmd->current_rev,
> > + force, pool));
>
> Compare this with the corresponding code in svn_ra_local__lock. That code
> will continue on particular errors (controlled by SVN_ERR_IS_LOCK_ERROR)).
> We need to do the same here. (And there is a 0 that should be FALSE there
> as well. Grrrrr!)
>
> Also, you use pool to call svn_repos_fs_lock. You should use subpool and
> then just svn_lock_dup() the returned lock if you need to store it. (This
> becomes irrelevant if you make this into just one loop.)
I didn't think of that either; thanks.
I haven't quoted the rest, but they are excellent points -- this was
an amazingly thorough review, thank you Peter.
Sameer: ball can be in your court or mine, let me know your preference.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 8 23:47:06 2005