You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Shlomi Fish <sh...@iglu.org.il> on 2004/04/24 07:20:18 UTC
[PATCH] Get-locations - Finished (more or less)
Hi all!
This time this patch adds the ra_svn/svnserve support. I could really use your
help in reviewing the patch and eliminating potential bugs or things that
weren't done optimally.
Check:
http://www.iglu.org.il/shlomif/svn-patches/peg-revision-tng-rev13.patch
Here's the log:
<<<
Adds an libsvn_fs layer function to efficiently trace the file historical
(and to a limited extent) location based on its peg revision+URL. Also adds
a callack that implements it to the RA vtable and bindings to it from
libsvn_ra_local, libsvn_ra_dav and libsvn_ra_svn. It also implements
the server-side handlers.
* build.conf
Compile ra-get-past-location, which is a command line client for this
functionality to facilitate testing.
* include/svn_fs.h
Added the declaration of svn_fs_trace_node_locations().
* include/svn_ra.h
Added the get_locations binding to the bottom of svn_ra_plugin_t.
* libsvn_fs/tree.c
Added the definition of svn_fs_trace_node_locations with two
accessory functions.
* libsvn_ra_local/ra_plugin.c
Added the definition of svn_ra_local__get_locations, a binding
to the RA vtable callback that handles the operation.
Added it to the end of the libsvn_ra_local vtable.
* subversion/mod_dav_svn/log.c
Added the definition of dav_svn__get_locations_report() which handles
the <S:get-locations> report and response. It has a helper function
send_get_locations_report().
* subversion/mod_dav_svn/version.c
(dav_svn_deliver_report): Added an appropriate case directive to
handle the get-locations report.
* subversion/mod_dav_svn/dav_svn.h
Added the declaration of dav_svn__get_locations_report().
* tests/libsvn_ra_local/get-past-location.c
(new file): a simple standalone command line client that queries for
the historical URLs at other revisions of a certain URL at a peg revision.
* tests/clients/cmdline/past_loc_tests.py
(new file): a test suite to test the past revision functionality.
* subversion/libsvn_ra_svn/client.c
Added the ra_svn_get_locations() handler, and added it to the plugin
vtable.
* subversion/libsvn_ra_dav/ra_dav.h
Added the new elements to the enum.
Added the declaration of svn_ra_dav__get_locations().
Added the declaration of svn_ra_dav__get_repos_root(). It is being
made global because I need to use it outside its file.
* subversion/libsvn_ra_dav/session.c
Made svn_ra_dav__get_repos_root() global instead of static.
Added svn_ra_dav__get_locations to the ra_dav plugin's vtable.
* subversion/libsvn_ra_dav/fetch.c
Added the definition of svn_ra_dav__get_locations() and helper functions
, struct declarations and comments. This function implements the
get_locations() handler() for ra_dav.
* subversion/svnserve/serve.c
Added the get_locations() handler for the get-locations command.
Added it to the main_command[] map.
>>>
(also here:
http://www.iglu.org.il/shlomif/svn-patches/peg-revision-tng-rev13.log.txt
)
To do:
1. Get feedback on the places marked with "TODO" (in all caps) in the patch.
2. If there's a need, change the behaviour of the ra_dav handler to inform the
upper layer that the server does not support this operation and it should
trace the locations using the log instead. I can use sussman's input on this.
3. Remove the remaining cruft (cmds.gdb file, etc.) from the patch.
4. Forward-port to the latest revision. (currently against 9245).
5. Verify that all the tests run correctly with this patch.
Enjoy!
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
Re: [PATCH] Get-locations - Finished (more or less)
Posted by Greg Hudson <gh...@MIT.EDU>.
Oh, one more thing I forgot, from serve.c:
+ /* Now, write the parameters to the stream. */
+
+ if (fs_locations)
This check does no good. svn_fs_trace_node_locations never returns
successfully with fs_locations set to NULL.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Get-locations - Finished (more or less)
Posted by Shlomi Fish <sh...@iglu.org.il>.
On Sunday 25 April 2004 23:18, Greg Hudson wrote:
> On Sun, 2004-04-25 at 03:52, Shlomi Fish wrote:
> > 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.
>
> I would support having a consistent style across the whole project, but
> consensus seems to be against that so far. Certainly, the right answer
> is not to have mixed styles within a single file.
>
OK, even though I also encountered places where one file had inconsistent
style.
> > > 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?
>
> No, relative to the URL used to open the RA session.
>
Well, in that case we may face a problem. Some of the paths returned can be
well outside that URL ("../../../" etc) and some of them can even be out of
the bounds of the working copy. (if it was checked out from a directory of
the repository).
Determining if the paths belong in the same repository would be somewhat
harder in this case, as would translating them to this format from the
absolute paths within the repository.
> > Well, when cmpilato and I formulated the specification for the
> > get_locations() handler, we specifically said we are going to input and
> > output URLs.
>
> Were you guys specifically talking about the RA interface, or were you
> talking about some other interface?
>
Well, the specific E-mail is this:
http://www.contactor.se/~dast/svn/archive-2004-04/0541.shtml
cmpilato there says that "The return hash maps those same location revisions
to the location paths.". I don't exactly know what he meant by "paths". I may
have actually misinterpreted him.
In any case, the ra_dav protocol does path URLs, and so should probably the
ra_svn one.
> It seems highly inconsistent for every single ra-lib function (including
> similar ones like check_path) to take a path parameter, while this one
> new function takes a URL parameter. If you guys have designed this
> inconsistency into the RA interface, then I think you guys have designed
> it wrong, although I'm willing to listen to arguments to the contrary.
>
OK.
> > > 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.
>
> It's not common. But it's a point of C correctness which we follow in
> our code. (There are other points of C correctness we don't follow; for
> instance, we use apr_pcalloc and expect it to initialize pointer
> parameters to NULL, which is not valid C. I argued against this years
> ago and lost.)
>
I recall the C FAQ saying that on some obscure platforms, the NULL pointer is
not all zero bits.
http://www.eskimo.com/~scs/C-faq/q5.17.html
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
Re: [PATCH] Get-locations - Finished (more or less)
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sun, 2004-04-25 at 03:52, Shlomi Fish wrote:
> 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.
I would support having a consistent style across the whole project, but
consensus seems to be against that so far. Certainly, the right answer
is not to have mixed styles within a single file.
> > 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?
No, relative to the URL used to open the RA session.
> Well, when cmpilato and I formulated the specification for the get_locations()
> handler, we specifically said we are going to input and output URLs.
Were you guys specifically talking about the RA interface, or were you
talking about some other interface?
It seems highly inconsistent for every single ra-lib function (including
similar ones like check_path) to take a path parameter, while this one
new function takes a URL parameter. If you guys have designed this
inconsistency into the RA interface, then I think you guys have designed
it wrong, although I'm willing to listen to arguments to the contrary.
> 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.
This sort of thing is going to happen. We can't make every developer
think of every problem at the earliest possible moment. So we have to
choose between making contributors go to more work, or doing things
wrong a lot. I prefer the former.
> > 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?
You should use what you already have. I thought I was clear about that.
> > 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.
It's not common. But it's a point of C correctness which we follow in
our code. (There are other points of C correctness we don't follow; for
instance, we use apr_pcalloc and expect it to initialize pointer
parameters to NULL, which is not valid C. I argued against this years
ago and lost.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Get-locations - Finished (more or less)
Posted by Greg Hudson <gh...@MIT.EDU>.
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