(I've tweaked the subject line to distinguish this part of the thread
(and this patch) from the first part (which had its own patch).)
On Mon, 2010-01-11, Ivan Zhakov wrote:
> On Mon, Jan 11, 2010 at 1:29 PM, Philip Martin
> <philip.mar...@wandisco.com> wrote:
> > Ivan Zhakov <i...@visualsvn.com> writes:
> >
> >> - return ne_xml_parse(pwb->parser, data, len);
> >> + parser_status = ne_xml_parse(pwb->parser, data, len);
> >> + if (parser_status)
> >> + {
> >> + /* Pass XML parser error. */
> >> + ne_set_error(pwb->req->ne_sess, "%s",
> >> ne_xml_get_error(pwb->parser));
> >> + }
> >> +
> >> + return parser_status;
> >
> > There's another call to ne_xml_parse in that file, in
> > parse_spool_file. Should we make the same change there?
>
> Good point, but XML parser neon is wrapped in parsed_request()
> function after calling parse_spool_file(). I updated my patch to
> return the same error message as parsed_request():
> [[[
> svn: The OPTIONS request returned invalid XML in the response: XML parse
> error at line 1: no element found (https://svn.apache.org/repos/)
> ]]]
> I like my original error message, but consistency is more important IMHO.
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?
- Julian
