Re: [PATCH] HTTP reason phrases not always returned in ra_dav error messages

"Bill Tutt" <rassilon@lyra.org> writes:

> While trying to diagnose whether an error a proxy error or not I noticed
> not all ra_dav error messages were including the Neon error message
> correctly.
[...]
> * libsvn_ra_dav/util.c:
> (svn_ra_dav__parsed_request, svn_ra_dav__request_dispatch) Adjust
> the code knowing that Neon can return NE_OK while still having
> a failure status code.

Sure, this makes sense. All you're doing is changing

   if (A) error; if (B) error;

into

   if (A || B) error;

It doesn't change the logic, just improves the error-throwing detail.
Looks good.

>
> * libsvn_ra_dav/props.c:
> (svn_ra_dav__get_props) Use error helper function to ensure
> that the HTTP reason string gets included in the error context.

This may not be as safe. You're changing the logic here:

> - if (rv != NE_OK)
> + if (rv != NE_OK
> + || status_code != 200)
> {
> const char *msg = apr_psprintf(pool, "PROPFIND of %s", url);
> return svn_ra_dav__convert_error(sess, msg, rv);
> }
> -
> - if (404 == status_code)
> - return svn_error_createf(SVN_ERR_RA_DAV_PROPS_NOT_FOUND, NULL,
> - "Failed to fetch props for '%s'", url);

Although I guess simply combining the if statements here won't help
you. You need to be able to catch 5XX codes from your proxy failures.

Maybe we should change this section of __get_props to

   if (nv != NE_OK || status_code < 200 || status_code > 299)

?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 20 21:22:58 2002