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.)
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.
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.
+ 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.
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);
+ /* 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.
+ 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.
+ /* 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.
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. 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.
+ fs_path = url+repos_url_len;
Spaces around binary operators.
+ 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;
+ /* All the paraemters are fine - let's perform the query against the
"parameters"
+ 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.
+ 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 *.
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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Apr 25 02:30:29 2004