On Thu, 17 Jun 2004, Ben Collins-Sussman wrote:
> On Wed, 2004-06-16 at 14:11, Peter N. Lundblad wrote:
>
> > I don't think ra_dav got much review. I'm not a DAV expert at all, so
> > could someone with deeper knowledge of that protocol please take a look?
>
> Looks good overall, I just have a few suggestions.
>
Thanks for reviewing.
>
> Just a style nit: I think this would be more readable if we tested "if
> (child->ns != ns) {continue;}" at the beginning, rather than indent
> all the main logic.
>
Agree. Fixed.
> +
> + /* Now we should have the parameters ready - let's
> + check if they are all present. */
> + if (! (relative_path && SVN_IS_VALID_REVNUM(peg_revision)))
> + {
> + return dav_new_error(resource->pool, HTTP_BAD_REQUEST, 0,
> + "Not all parameters passed.");
> + }
> +
>
> Should we also test that the location_revisions array has at least 1 element?
> What happens if that array is empty? Should we demand at least one
> location revision in the original REPORT request?
>
I don't think there is any reason to have a special case for this corner
case. svn_repos_trace_node_locations should handle this case (maybe not in
the most efficient way, but who cares). Still, 0 revisions give 0
locations.
>
>
> + /* Append the relative paths to the base FS path to get an
> + absolute repository path. */
> + abs_path = svn_path_join(resource->info->repos_path, relative_path,
> + resource->pool);
> +
> + serr = svn_repos_trace_node_locations(resource->info->repos->fs,
> + &fs_locations, abs_path, peg_revision,
> + location_revisions, resource->pool);
> +
> + if (serr)
> + {
> + return dav_svn_convert_err(serr, HTTP_BAD_REQUEST, serr->message,
> + resource->pool);
> + }
>
> Hmmm, I wouldn't use HTTP_BAD_REQUEST. That usually implies some sort
> of syntax error in the client request. But the client request already
> parsed cleanly. If the node-tracing function failed, I would just
> call this a "server error" (something's wrong with the repository
Or the path doesn't exist at the peg revision or something.
> semantics)... and throw an HTTP_INTERNAL_ERROR.
>
Changed anyway since it seems more consistent.
>
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> + if (SVN_IS_VALID_REVNUM(rev) && path)
> + apr_hash_set(baton->hash,
> + apr_pmemdup(baton->pool, &rev, sizeof (rev)),
> + sizeof(rev), apr_pstrdup(baton->pool, path));
>
>
> Maybe we should have an 'else' here to throw an integer errorcode? If
> either of the 2 attributes is missing, that's definitely a syntax
> error in the response. (NE_XML_ABORT?)
You're right.
> + /* ras's URL may not exist in HEAD, and thus it's not safe to send
> + it as the main argument to the REPORT request; it might cause
> + dav_get_resource() to choke on the server. So instead, we pass a
> + baseline-collection URL, which we get from the peg revision. */
> + SVN_ERR(svn_ra_dav__get_baseline_info(NULL, &bc_url, &bc_relative, NULL,
> + ras->sess, ras->url, peg_revision,
> + ras->pool));
>
> We need to run the REPORT on some URI (which is ignored), and you're
> right, the URI may not exist in HEAD, which could cause mod_dav_svn to
> fail. But IIRC, __get_baseline_info() is a very expensive function,
> with potentially many network turnarounds. I think we should run this
> REPORT on the "VCC" URI, just as we do for many other types of report
> requests. The VCC URI is always guaranteed to exist, and can be
> obtained via svn_ra_dav__get_vcc(). This would keep our REPORT
> strategy pretty consistent across the board.
Maybe pretty consistent, but not very consistent:-) This code was stolen
from log.c. Maybe there is room for improvement there, or is there a
reason for using the BC there? BTW, I've noticed that get_logs requires
some round-trips... I'll try this out.
> >
> + final_bc_url = svn_path_url_add_component(bc_url.data, bc_relative.data,
> + ras->pool);
> +
> + err = svn_ra_dav__parsed_request(ras->sess, "REPORT",
> + final_bc_url,
> + request_body->data, NULL, NULL,
> + gloc_start_element, NULL, NULL,
> + &request_baton, NULL, &status_code,
> + pool);
> +
> + /* Map status 501: Method Not Implemented to our not implemented error.
> + 1.0.x servers and older don't support this report. */
> + if (status_code == 501)
> + return svn_error_create (SVN_ERR_RA_NOT_IMPLEMENTED, err,
> + _("get-locations REPORT not implemented"));
>
> Excellent, I love this error mapping. :-)
Good, since it also works. It passes the test suite against a 1.0 server
now. Except for one test where the output is different. I'll come back to
that in another thread.
I'll send the whole patch again, hopefully for the final round.
Thanks again,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 17 22:07:47 2004