On Sunday 25 April 2004 03:30, Greg Hudson wrote:
> On Sat, 2004-04-24 at 03:20, Shlomi Fish wrote:
> > http://www.iglu.org.il/shlomif/svn-patches/peg-revision-tng-rev13.patch
>
> This web server presents your patch with the MIME type
> application/octet-stream, which is inconvenient; also, someone might
> check through the list archives months from now and find that this is a
> stale link. Please send patches as plain-text attachments or as inline
> text. (Make sure your MUA doesn't word-wrap it if you do the latter.)
>
OK, I just sent a link to it, because I sent quite a few large patches like
that to the mailing list alreay, and did not want to clog it much more. I
will send an updated patch soon.
> Your patch is chock full of trailing whitespace. It certainly wouldn't
> be the first time we've committed trailing whitespace to the tree, but
> we don't normally do it such vast quantities. Please clean it up.
>
Done.
> Beyond that note, I only reviewed the ra_svn parts of the patch.
> Starting with client.c:
>
> +static svn_error_t * ra_svn_get_locations (void *session_baton,
>
> Please observe the style of the file you are modifying. It's clear that
> you copied code from other parts of the file and then changed its style;
> don't do that.
>
> Specifically: no space before open parens in function calls or function
> definitions. No space after the "*" of a pointer type. Comments should
> generally be complete, punctuated sentences, and if they fit on one
> line, they should only take up one line. No braces around a
> single-statement body of a control statement. No extra parentheses
> around a comparison within a boolean expression to clarify precedence.
> When wrapping an expression because it's too long, try to use as few
> lines as possible; don't use one line per argument just because you
> started wrapping it. Only one blank line after the end of a function
> definition, not two.
>
Hmmm... is there any reason the style of the ra_svn and svnserve components is
different than the rest of Subversion. This is highly confusing and I did not
know what to do in this case. Future contributors may face the same problem.
> + url = svn_path_uri_decode (url, pool);
> + repos_url_len = strlen (repos_url);
> + if (strncmp (url, repos_url, repos_url_len) != 0)
> + return svn_error_createf (SVN_ERR_RA_ILLEGAL_URL, NULL,
> + "'%s'\n"
> + "is not the same repository as\n"
> + "'%s'", url, repos_url);
>
> This client code does not seem appropriate. An ra_lib function is not
> supposed to take a URL argument to operate on; it is supposed to take a
> path argument relative to the root of the RA session.
>
You mean the root returned by the get_repos_root() callback? Well, when
cmpilato and I formulated the specification for the get_locations() handler,
we specifically said we are going to input and output URLs. If we are to
change it now, it will require quite a lot of work. It is doable and I am
willing to do it, but think that we should have thought about it earlier.
> Also, this error message really shouldn't have internal newlines. I
> don't know where you copied it from; this snippet from svnserve is
> better:
>
> return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
> "'%s' is not the same repository as '%s'",
> url, repos_url);
>
OK, done. It may appear in some other places in the code. I'll check and
correct it.
> + /* TODO: should it be svn_ra_svn_write_number()
> + * instead? */
> + SVN_ERR (svn_ra_svn_write_tuple(conn, pool, "!r!", past_revision));
>
> svn_ra_svn_write_number() would do the same thing, but consistently
> using write_tuple() should make it easier to understand.
>
So what should I use?
> + if (strncmp (ret_url, repos_url, repos_url_len) != 0)
> + {
> + return
> + svn_error_createf (SVN_ERR_RA_ILLEGAL_URL, NULL,
> + "'%s'\n"
> + "is not the same repository as\n"
> + "'%s'", ret_url, repos_url);
> + }
>
> This client code, which checks for an invalid URL sent by the
> repository, appears to use the wrong error code; the caller would tend
> to think that it supplied an illegal URL, rather than that the server
> screwed up and sent an illegal one. I suggest SVN_ERR_MALFORMED_DATA.
>
Done
> + /* Read the response.
> + * I'm just copying it from ra_svn_log() - I don't know why I have
> + * to call it --shlomif.
> + * */
> + SVN_ERR (svn_ra_svn_read_cmd_response (conn, pool, ""));
>
> So that the server has a chance to report an error.
>
Fixed the comment.
> On to serve.c:
>
> + /*
> + * Sanity check to see if the URL is under this
> + * repository. Should not really happen, unless we have
> + * a malicious client.
> + *
> + * TODO: check for a slash at url[conn_repos_url_len] -
> + * ???
> + * */
>
> See the get_fs_path helper function.
Done.
> But, as I noted before, this
> operation should be accepting a path parameter, like all the other
> operations (see check_path, for example), not a URL.
>
Well, it's something I'll put on discussion in the mailing list, because at
the moment there are two contradictory opinions here.
> + fs_path = url+repos_url_len;
>
> Spaces around binary operators.
>
Well, I already eliminated this line due to the use of get_fs_path().
> + elt = &((svn_ra_svn_item_t *) loc_revs_proto->elts)[i];
>
> + *((svn_revnum_t*) apr_array_push(location_revisions)) =
> past_revision;
>
> I realize this is just copied from log_cmd, but please change these to:
>
> elt = &APR_ARRAY_IDX(loc_revs_proto, i, svn_ra_svn_item_t);
>
> APR_ARRAY_PUSH(location_revisions, svn_revnum_t) = past_revision;
>
Done.
> + /* All the paraemters are fine - let's perform the query against the
>
> "parameters"
>
Right. Done.
> + SVN_ERR(svn_fs_trace_node_locations(b->fs,
> + &fs_locations,
> + fs_path,
> + peg_revision,
> + location_revisions,
> + pool));
>
> You have to be more careful with errors here. The code should look
> like:
>
> err = svn_fs_trace_node_locations(...);
> if (!err)
> {
> /* Write the results. */
> ...
> }
> write_err = svn_ra_svn_write_word(conn, pool, "done");
> if (write_err)
> {
> svn_error_clear(err);
> return write_err;
> }
> SVN_CMD_ERR(err);
> SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
>
> + /* Now, write the parameters to the stream. */
>
> We're not writing parameters. We're writing results. Or locations. Or
> something. But not parameters. And we're not writing to a stream;
> we're writing to a connection, or to a client.
>
Right. Done.
> + iter = apr_hash_first (pool, fs_locations);
> + while (iter)
>
> The normal idiom in our code is to use a for loop to traverse a hash
> table, not a while loop.
>
> + apr_hash_this (iter, (const void**)&key, &key_len, (void
> **)&past_fs_path);
>
> This is not correct C code; the representation of a void * could,
> conceivably, be different from the representation of an svn_revnum_t *.
Interesting. I never encountered such a situation. Oh well. If you say so.
> Use temporary variables of type const void * and void * to hold the key
> and value as returned by apr_hash_this. On the other hand, you can pass
> NULL for key_len.
>
Done. I'll have to fix it in the other places of the code as well.
Thanks for reviewing part of the patch!
Regards,
Shlomi Fish
---------------------------------------------------------------------
Shlomi Fish shlomif@iglu.org.il
Homepage: http://shlomif.il.eu.org/
Quidquid latine dictum sit, altum viditur.
[Whatever is said in Latin sounds profound.]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Apr 25 09:47:14 2004