On Fri, 2010-01-22, Ivan Zhakov wrote:
> On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <julianf...@btopenworld.com>
> wrote:
> > On Mon, 2010-01-11, Ivan Zhakov wrote:
> > Your new patch now has a more descriptive error message, that is the
> > same as the one produced by the parse_spool_file() function, which is
> > good, but they are generated in different ways at different call levels,
> > which is bad. Your patch has:
> >
> >> + parser_status = ne_xml_parse(pwb->parser, data, len);
> >> + if (parser_status)
> >> + {
> >> + /* Pass XML parser error. */
> >> + pwb->req->err = svn_error_createf
> >> + (SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
> >> + _("The %s request returned invalid XML "
> >> + "in the response: %s (%s)"),
> >> + pwb->req->method,
> >> + ne_xml_get_error(pwb->parser),
> >> + pwb->req->url);
> >> + }
> >
> > whereas parse_request() calls parse_spool_file() which calls
> > ne_xml_parse(), and then it does:
> >
> >> /* was there an XML parse error somewhere? */
> >> msg = ne_xml_get_error(success_parser);
> >> if (msg != NULL && *msg != '\0')
> >> return svn_error_createf(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
> >> _("The %s request returned invalid XML "
> >> "in the response: %s (%s)"),
> >> method, msg, url);
> >
> > Can't we do them both in the same way, somehow?
> >
> I don't like code duplication too, but I didn't find good way how this
> code could be merged. So I propose to commit my patch since it makes
> error diagnostic much better it costs just 2-line duplication.
Thanks, Ivan.
I looked at this a bit more. First I factored out the error message
generation into a separate function.
I committed that version as r902836.
When I said "different call levels", I was thinking of the fact that the
first function, wrapper_reader_cb(), extracts the Neon parse error
message and turns it into a Subversion error immediately after calling
the ne_xml_parse(), whereas the second function, parsed_request(), calls
the parser in a complex way (many times) and then extracts any Neon
parse error message afterwards, when it is finished with the parser.
If wrapper_reader_cb() converts the parser error into a Subversion error
itself, it now has two ways of reporting an error: by returning the
parser status code (int) and/or by setting pwb->req->err. Generally it
is best to have just one way. I can't see a big problem with reporting
the error both ways but I'm looking for a better way.
I think the right place to call the (factored out) error detector
function is at the end of the request that uses the parser - so,
mirroring the end of parsed_request(), that would be at the end of
svn_ra_neon__request_dispatch() INSTEAD OF in wrapper_reader_cb().
However, I'm not completely sure of that.
Does that make any sense?
- Julian
