svn_ra_dav__parsed_request error handling

While reviewing /branches/1.2.x-get-locks I came across
svn_ra_dav__parsed_request error handling like this:

  err = svn_ra_dav__parsed_request(... &status_code ...);

  /* 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,
                            _("Server does not support locking features"));

  if (err && err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
    return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, err,
                            _("Server does not support locking features"));

Note that status_code is checked before err. That could be a problem
since parsed_request can return an error without setting status code.
The current code works because status_code is set to zero before
svn_ra_dav__parsed_request is called so status_code is always valid.
However relying on the caller like that is not reliable and indeed
reporter_finish_report gets it wrong, although it doesn't use
status_code either so it's not a problem at the moment.

I don't think it's feasible for callers to examine err to determine
whether status_code is valid (or is it?), so I'd suggest that
parsed_request be responsible for always setting status_code. That
leaves the question what value to use. Should it use zero or some
special "non-error" value, -1 prehaps? If we decide to continue to
make the caller responsible then svn_ra_dav__parsed_request should
document that requirement.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org