Re: svn commit: r22712 - trunk/subversion/libsvn_ra_dav

On Thu, Dec 14, 2006 at 12:38:10PM -0800, dionisos@tigris.org wrote:
> Merge r22555:22710 from ra_dav-refactoring branch, closing.

Nice! A few comments below (though I only skimmed the diff, and I'm not
a DAV expert).

> --- trunk/subversion/libsvn_ra_dav/fetch.c (original)
> +++ trunk/subversion/libsvn_ra_dav/fetch.c Thu Dec 14 12:38:10 2006
> @@ -1893,8 +1850,7 @@
> */
>
> /* This implements the `svn_ra_dav__xml_validate_cb' prototype. */
> -static int validate_element(void *userdata,
> - svn_ra_dav__xml_elmid parent,
> +static int validate_element(svn_ra_dav__xml_elmid parent,
> svn_ra_dav__xml_elmid child)
> {
> /* We're being very strict with the validity of XML elements here. If

validate_element() could do with some doc-comments (as could the
versions in props.c and merge.c). The current one here refers to a
non-existent callback signature as well :-)

> --- trunk/subversion/libsvn_ra_dav/props.c (original)
> +++ trunk/subversion/libsvn_ra_dav/props.c Thu Dec 14 12:38:10 2006

in svn_ra_dav__do_proppatch():

> - /* WebDAV spec says that if any part of a PROPPATCH fails, the
> - entire 'commit' is rejected. */
> - err = svn_error_create
> - (SVN_ERR_RA_DAV_PROPPATCH_FAILED, NULL,
> - _("At least one property change failed; repository is unchanged"));
> - }
> + /* Finish up the headers. */
> + if (! extra_headers)
> + extra_headers = apr_hash_make(pool);
> + apr_hash_set(extra_headers, "Content-Type", APR_HASH_KEY_STRING,
> + "text/xml; charset=UTF-8");
> +
> + err = svn_ra_dav__simple_request(NULL, ras, "PROPPATCH", url,
> + extra_headers, body->data,
> + 200, 207, pool);
> + if (err)
> + svn_error_compose
> + (err,
> + svn_error_create
> + (SVN_ERR_RA_DAV_PROPPATCH_FAILED, NULL,
> + _("At least one property change failed; repository is unchanged")));

Is this right? Previously we'd return SVN_ERR_RA_DAV_PROPPATCH_FAILED,
now we return an error wrapping SVN_ERR_RA_DAV_PROPPATCH_FAILED.

> --- trunk/subversion/libsvn_ra_dav/ra_dav.h (original)
> +++ trunk/subversion/libsvn_ra_dav/ra_dav.h Thu Dec 14 12:38:10 2006

svn_ra_dav__get_baseline_info() et seq. still say "Given a Neon session
SESS", but SESS has now changed type so that it's no longer a Neon
session.

> @@ -598,13 +580,17 @@
> const char *name);
>
>
> -/* Create an xml parser for use with REQ.
> +/* Create an xml parser and registers a response body reader with REQ.
> *
> * Register a pool cleanup on the pool of REQ to clean up any allocated
> * Neon resources
> + *
> + * Pass NULL for ACCPT when you don't want the returned parser
> + * to be attached to REQ.
> */
> ne_xml_parser *
> svn_ra_dav__xml_parser_create(svn_ra_dav__request_t *req,
> + ne_accept_response accpt,
> svn_ra_dav__startelm_cb_t startelm_cb,
> svn_ra_dav__cdata_cb_t cdata_cb,
> svn_ra_dav__endelm_cb_t endelm_cb,

This is a little under-documented: I assume that the new XML parser is
what's registered as REQ's reponse body reader, but I'm not clear what
ACCPT does if it's non-NULL. (The callbacks could also do with a
mention, though they're fairly obvious).

> @@ -855,10 +838,45 @@
> svn_error_t *
> svn_ra_dav__request_dispatch(int *code_p,
> svn_ra_dav__request_t *request,
> + apr_hash_t *extra_headers,
> + const char *body,
> int okay_1,
> int okay_2,
> apr_pool_t *pool);

The doc-comments for this function still refer to 'a neon REQUEST and
SESSION', and don't mention the extra two arguments. (extra_header is
pretty obvious, but 'body' isn't).

Regards,
Malcolm