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

[Malcolm Rowe <ma...@farside.org.uk>]

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