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

On Fri, 01 Sep 2006, rooneg@tigris.org wrote:
...
> --- trunk/subversion/libsvn_ra_dav/fetch.c (original)
> +++ trunk/subversion/libsvn_ra_dav/fetch.c Fri Sep 1 13:14:27 2006
...
> -/* This implements the `ne_xml_startelem_cb' prototype. */
> -static int getlocks_start_element(void *userdata, int parent_state,
> - const char *ns, const char *ln,
> - const char **atts)
> +/* This implements the `svn_ra_dav__startelm_cb_t' prototype. */
> +static svn_error_t *
> +getlocks_start_element(int *elem, void *userdata, int parent_state,
> + const char *ns, const char *ln, const char **atts)
> {
> get_locks_baton_t *baton = userdata;
> const svn_ra_dav__xml_elm_t *elm;
...
> if (elm->id == ELEM_lock)
> {
> if (parent_state != ELEM_get_locks_report)
> - return NE_XML_ABORT;
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + _("Got lock element in the wrong location"));
> else
> /* allocate a new svn_lock_t in the permanent pool */
> baton->current_lock = svn_lock_create(baton->pool);
> @@ -1579,7 +1584,8 @@
> const char *encoding;
>
> if (parent_state != ELEM_lock)
> - return NE_XML_ABORT;
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + _("Got element at an unexpected location"));

Any particular reason why one error message complains about the lock
element "wrong location", while the other complains about an element
at an "unexpected location"?
...
> -/* This implements the `ne_xml_endelm_cb' prototype. */
> -static int getlocks_end_element(void *userdata, int state,
> - const char *ns, const char *ln)
> +/* This implements the `svn_ra_dav__endelm_cb_t' prototype. */
> +static svn_error_t *
> +getlocks_end_element(void *userdata, int state,
> + const char *ns, const char *ln)
> {
> get_locks_baton_t *baton = userdata;
> const svn_ra_dav__xml_elm_t *elm;
> - svn_error_t *err;
>
> elm = svn_ra_dav__lookup_xml_elem(getlocks_report_elements, ns, ln);
>
> /* Just skip unknown elements. */
> if (elm == NULL)
> - return NE_XML_DECLINE;
> + return SVN_NO_ERROR;;

This seems like a subtle change in behavior in the information
returned by this function, and a difference in the API (lacking an
"int *elem" parameter). I guess the loss of "declined" vs. "no error"
doesn't matter in this case?

...
> @@ -1638,11 +1647,8 @@
> || (! baton->current_lock->token)
> || (! baton->current_lock->owner)
> || (! baton->current_lock->creation_date))
> - {
> - baton->err = svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> - _("Incomplete lock data returned"));
> - return NE_XML_ABORT;
> - }
> + SVN_ERR(svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + _("Incomplete lock data returned")));

Ditto.

...
> - /* unrecognized encoding! */
> - return NE_XML_ABORT;
> + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA,
> + NULL,
> + _("Got unrecognized encoding '%s'"),
> + baton->encoding);

Ditto.

(In case any other reviewers are curious, we discontinued use of the
baton->err field because it was removed from the baton structure.)

...
> rc = validate_element(NULL, parent_state, elm->id);
>
> if (rc != SVN_RA_DAV__XML_VALID)
> - return (rc == SVN_RA_DAV__XML_DECLINE) ? NE_XML_DECLINE : NE_XML_ABORT;
> + {
> + if (rc == SVN_RA_DAV__XML_DECLINE)
> + {
> + *elem = NE_XML_DECLINE;
> + return SVN_NO_ERROR;
> + }
> + else
> + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + _("Got unexpected element '%s:%s'"),

It's nice that we supply the unexpected element's name here. Why not
do that above also? (And do we need to check the element's name for
null before using it?)

> --- trunk/subversion/libsvn_ra_dav/file_revs.c (original)
> +++ trunk/subversion/libsvn_ra_dav/file_revs.c Fri Sep 1 13:14:27 2006
...
> switch (parent_state)
> {
> case ELEM_root:
> if (elm->id != ELEM_file_revs_report)
> - return NE_XML_ABORT;
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + _("Got unexpected element"));
> break;

As above -- dropping some error context, and not supplying the element
name in the error message. More of the same throughout this file.

...
> switch (elm->id)
> {
> case ELEM_rev_prop:
> case ELEM_set_prop:
> att = svn_xml_get_attr_value("name", atts);
> if (!att)
> - return NE_XML_ABORT;
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + _("Element is missing name attr"));

Out of context, this error message would probably be a bit tough to
digest. It would help to quote "name". More of the same below.

> --- trunk/subversion/libsvn_ra_dav/ra_dav.h (original)
> +++ trunk/subversion/libsvn_ra_dav/ra_dav.h Fri Sep 1 13:14:27 2006
...
> +/* Our equivalent of ne_xml_startelm_cb, the difference being that it
> + * returns errors in a svn_error_t, and returns the element type via
> + * ELEM. To ignore the element *ELEM should be set to NE_XML_DECLINE
> + * and SVN_NO_ERROR should be returned.
> + */
> +typedef svn_error_t * (*svn_ra_dav__startelm_cb_t)(int *elem,
> + void *baton,
> + int parent,
> + const char *nspace,
> + const char *name,
> + const char **atts);
> +
> +/* Our equivalent of ne_xml_cdata_cb, the difference being that it returns
> + * errors in a svn_error_t.
> + */
> +typedef svn_error_t * (*svn_ra_dav__cdata_cb_t)(void *baton,
> + int state,
> + const char *cdata,
> + size_t len);

As mentioned above, why do some APIs allow return of the Neon error
code through the int *elem parameter, and some do not?

> --- trunk/subversion/libsvn_ra_dav/replay.c (original)
> +++ trunk/subversion/libsvn_ra_dav/replay.c Fri Sep 1 13:14:27 2006

I have similar questions/comments as for replay.c as for the other .c
files in ra_dav.

> --- trunk/subversion/libsvn_ra_dav/util.c (original)
> +++ trunk/subversion/libsvn_ra_dav/util.c Fri Sep 1 13:14:27 2006
> @@ -555,6 +555,70 @@
> }
>
>
> +typedef struct {
> + svn_error_t *err;
> +
> + void *baton;
> +
> + svn_ra_dav__startelm_cb_t startelm_cb;
> + svn_ra_dav__cdata_cb_t cdata_cb;
> + svn_ra_dav__endelm_cb_t endelm_cb;
> +} parser_wrapper_baton_t;

This Neon wrapper data structure should be documented per the info
from the change log message for this commit. Good doc here would
largely obviate the need to document the implementing functions below.