On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> (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.martin_at_wandisco.com> wrote:
>> > Ivan Zhakov <ivan_at_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?
>
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.
--
Ivan Zhakov
VisualSVN Team
Received on 2010-01-22 13:29:16 CET