[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix the awful "200 OK" error message when can't connect to repository

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Fri, 22 Jan 2010 15:28:41 +0300

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

This is an archived mail posted to the Subversion Dev mailing list.