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.
Index: subversion/mod_dav_svn/version.c
===================================================================
--- subversion/mod_dav_svn/version.c (revision 10007)
+++ subversion/mod_dav_svn/version.c (working copy)
+dav_error *dav_svn__get_locations_report(const dav_resource *resource,
+ const apr_xml_doc *doc,
+ ap_filter_t *output)
+{
+ svn_error_t *serr;
+ dav_error *derr = NULL;
+ apr_status_t apr_err;
+ apr_bucket_brigade *bb;
+
+ /* The parameters to do the operation on. */
+ const char *relative_path = NULL;
+ const char *abs_path;
+ svn_revnum_t peg_revision = SVN_INVALID_REVNUM;
+ apr_array_header_t *location_revisions;
+
+ /* XML Parsing Variables */
+ int ns;
+ apr_xml_elem *child;
+
+ apr_hash_t *fs_locations;
+
+ location_revisions = apr_array_make(resource->pool, 0,
+ sizeof(svn_revnum_t));
+
+ /* Sanity check. */
+ ns = dav_svn_find_ns(doc->namespaces, SVN_XML_NAMESPACE);
+ if (ns == -1)
+ {
+ return dav_new_error(resource->pool, HTTP_BAD_REQUEST, 0,
+ "The request does not contain the 'svn:' "
+ "namespace, so it is not going to have certain "
+ "required elements.");
+ }
+
+ /* Gather the parameters. */
+ for (child = doc->root->first_child; child != NULL; child = child->next)
+ {
+ /* If this element isn't one of ours, then skip it. */
+ if (child->ns == ns)
+ {
+ if (strcmp(child->name, "peg-revision") == 0)
+ /* ### assume no white space, no child elems, etc */
+ peg_revision = SVN_STR_TO_REV(child->first_cdata.first->text);
+ else if (strcmp(child->name, "location-revision") == 0)
+ {
+ /* ### assume no white space, no child elems, etc */
+ svn_revnum_t revision
+ = SVN_STR_TO_REV(child->first_cdata.first->text);
+ APR_ARRAY_PUSH(location_revisions, svn_revnum_t) = revision;
+ }
+ else if (strcmp(child->name, "path") == 0)
+ {
+ if (child->first_cdata.first)
+ relative_path = apr_pstrdup(resource->pool,
+ child->first_cdata.first->text);
+ else
+ relative_path = "";
+ }
+ }
+ }
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.
+
+ /* 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?
+ /* 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
semantics)... and throw an HTTP_INTERNAL_ERROR.
Index: subversion/libsvn_ra_dav/fetch.c
===================================================================
--- subversion/libsvn_ra_dav/fetch.c (revision 10007)
+++ subversion/libsvn_ra_dav/fetch.c (working copy)
@@ -26,6 +26,7 @@
#include <apr_strings.h>
#include <apr_md5.h>
#include <apr_portable.h>
+#include <apr_xml.h>
#include <ne_socket.h>
#include <ne_basic.h>
@@ -1097,7 +1098,145 @@
return SVN_NO_ERROR;
}
+typedef struct {
+ svn_ra_session_t *ras;
+ apr_hash_t *hash;
+ apr_pool_t *pool;
+} get_locations_baton_t;
+/*
+ * Plan for processing the XML. The XML will be of the form:
+ *
+ * <S:get-locations-report xmlns...>
+ * <S:location rev="..." path="..."/>
+ * ...
+ * </S:get-locations-report>
+ *
+ * We extract what we want at the start of <S:location>. */
+/* Elements used in a get-locations response */
+static const svn_ra_dav__xml_elm_t gloc_report_elements[] =
+{
+ { SVN_XML_NAMESPACE, "get-locations-report", ELEM_get_locations_report, 0 },
+ { SVN_XML_NAMESPACE, "location", ELEM_location, 0 },
+ { NULL }
+};
+
+static const char *get_attr(const char **atts, const char *which);
+
+/* This implements the `ne_xml_startelem_cb' prototype. */
+static int gloc_start_element(void *userdata, int parent_state, const char *ns,
+ const char *ln, const char **atts)
+{
+ get_locations_baton_t *baton = userdata;
+ const svn_ra_dav__xml_elm_t *elm;
+
+ elm = svn_ra_dav__lookup_xml_elem(gloc_report_elements, ns, ln);
+
+ /* Just skip unknown elements. */
+ if (!elm)
+ return NE_XML_DECLINE;
+
+ if (parent_state == ELEM_get_locations_report
+ && elm->id == ELEM_location)
+ {
+ svn_revnum_t rev = SVN_INVALID_REVNUM;
+ const char *path;
+ const char *r;
+
+ r = get_attr(atts, "rev");
+ if (r)
+ rev = SVN_STR_TO_REV(r);
+
+ path = get_attr(atts, "path");
+
+ 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?)
+ }
+
+ return elm->id;
+}
+
+svn_error_t *
+svn_ra_dav__get_locations(void *session_baton,
+ apr_hash_t **locations,
+ const char *relative_path,
+ svn_revnum_t peg_revision,
+ apr_array_header_t *location_revisions,
+ apr_pool_t *pool)
+{
+ svn_ra_session_t *ras = session_baton;
+ svn_stringbuf_t *request_body;
+ svn_error_t *err;
+ get_locations_baton_t request_baton;
+ const char *relative_path_quoted;
+ svn_string_t bc_url, bc_relative;
+ const char *final_bc_url;
+ int i;
+ int status_code = 0;
+
+ *locations = apr_hash_make (pool);
+
+ request_body = svn_stringbuf_create("", pool);
+ svn_stringbuf_appendcstr(request_body,
+ "<?xml version=\"1.0\" encoding=\"utf-8\"?>" DEBUG_CR
+ "<S:get-locations xmlns:S=\"" SVN_XML_NAMESPACE
+ "\" xmlns:D=\"DAV:\">" DEBUG_CR);
+
+ svn_stringbuf_appendcstr(request_body, "<S:path>");
+ /* We need to escape the path XML-wise. */
+ relative_path_quoted = apr_xml_quote_string(pool, relative_path, 0);
+ svn_stringbuf_appendcstr(request_body, relative_path_quoted);
+ svn_stringbuf_appendcstr(request_body, "</S:path>" DEBUG_CR);
+ svn_stringbuf_appendcstr(request_body,
+ apr_psprintf(pool,
+ "<S:peg-revision>%ld"
+ "</S:peg-revision>" DEBUG_CR,
+ peg_revision));
+
+ for (i = 0; i < location_revisions->nelts; ++i)
+ {
+ svn_revnum_t rev = APR_ARRAY_IDX(location_revisions, i, svn_revnum_t);
+ svn_stringbuf_appendcstr(request_body,
+ apr_psprintf(pool,
+ "<S:location-revision>%ld"
+ "</S:location-revision>" DEBUG_CR,
+ rev));
+ }
+
+ svn_stringbuf_appendcstr (request_body, "</S:get-locations>");
+
+ request_baton.ras = ras;
+ request_baton.hash = *locations;
+ request_baton.pool = pool;
+
+ /* 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.
+ 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. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 17 18:34:54 2004