Hi Ivan,
Just thought I would send you an email and check if you had managed to review Julian's comments?
And as always - please don;t be shy about asking for any assistance you might need.
Gavin "Beau" Baumanis
On 14/01/2010, at 2:04 AM, Julian Foad 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?
>
> - Julian
>
>
Received on 2010-01-20 13:36:51 CET