You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2010/01/09 22:56:32 UTC
Re: [PATCH] Augment the awful "200 OK" error message when can't
connect to repository
On Fri, Jan 8, 2010 at 4:22 PM, Julian Foad <ju...@btopenworld.com> wrote:
> What happens if I accidentally specify a URL that's not a repository?
> (Recent trunk build, Neon.)
>
>> $ svn checkout https://svn.apache.org/repos/
>> svn: OPTIONS of 'https://svn.apache.org/repos': 200 OK (https://svn.apache.org)
>
> This is a truly awful error message, appearing alone as it does. Google
> reveals that many users have spent hours trying to track down the cause
> of such errors. They appear in all repository operations, not just
> checkout. For example,
> <http://forum.joomla.org/viewtopic.php?f=406&t=205587> (during import;
> unresolved) and
> <http://stackoverflow.com/questions/1025377/an-svn-error-200-ok-when-checking-out-from-my-online-repo> (during checkout; SSH credentials not provided).
>
>
Hi Julian,
Fully agree with you that this message is really confusing.
> What do we need to do? Two things, I think:
>
> 1. That low-level error report needs to be better. Any low-level
> function that regards a "200 OK" response as an error must not just
> create an error object saying "200 OK" alone but must wrap or replace
> that with an error message that indicates what it was trying to do and
> why an "OK" response is wrong.
>
Please consider attached patch to turn 200 OK error message to more
descriptive one:
svn: OPTIONS of 'https://svn.apache.org/repos/': XML parse error at
line 1: no element
found (https://svn.apache.org)
[[[
Improve RA neon error message when non XML response received.
* subversion/libsvn_ra_neon/util.c
(wrapper_reader_cb): Wrap neon XML parser error.
]]]
PS: Tests are still running.
--
Ivan Zhakov
VisualSVN Team
Re: [PATCH] Fix the awful "200 OK" error message when can't connect
to repository
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, Jan 20, 2010 at 3:20 PM, Gavin Beau Baumanis
<ga...@thespidernet.com> wrote:
> 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.
This email is already in my task queue, so don't worry I discuss it
with Julian and commit it myself.
--
Ivan Zhakov
VisualSVN Team
Re: [PATCH] Fix the awful "200 OK" error message when can't connect to repository
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
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
>> <ph...@wandisco.com> wrote:
>>> Ivan Zhakov <iv...@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
>
>
Re: [PATCH] Fix the awful "200 OK" error message when can't
connect to repository
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2010-01-26, Ivan Zhakov wrote:
> > 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.
>
> Thanks!
> I committed small follow up in r903153 to use SVN_RA_NEON__REQ_ERR
> macro instead of assigning req->err to prevent potential error
> leakage.
Thank you.
> > 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?
>
> I'm not sure but I'd like prefer to extract neon error ASAP. Passing
> global state like 'last error' between call levels looks unreliable
> for me.
Well, I don't know, so I'll leave it as it is.
- Julian
Re: [PATCH] Fix the awful "200 OK" error message when can't connect
to repository
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, Jan 25, 2010 at 6:21 PM, Julian Foad <ju...@btopenworld.com> wrote:
> On Fri, 2010-01-22, Ivan Zhakov wrote:
>> On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <ju...@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.
>
Thanks!
I committed small follow up in r903153 to use SVN_RA_NEON__REQ_ERR
macro instead of assigning req->err to prevent potential error
leakage.
> 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?
>
I'm not sure but I'd like prefer to extract neon error ASAP. Passing
global state like 'last error' between call levels looks unreliable
for me.
--
Ivan Zhakov
VisualSVN Team
Re: [PATCH] Fix the awful "200 OK" error message when can't
connect to repository
Posted by Julian Foad <ju...@btopenworld.com>.
On Fri, 2010-01-22, Ivan Zhakov wrote:
> On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <ju...@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
Re: [PATCH] Fix the awful "200 OK" error message when can't connect
to repository
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <ju...@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
>> <ph...@wandisco.com> wrote:
>> > Ivan Zhakov <iv...@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
Re: [PATCH] Fix the awful "200 OK" error message when can't
connect to repository
Posted by Julian Foad <ju...@btopenworld.com>.
(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
> <ph...@wandisco.com> wrote:
> > Ivan Zhakov <iv...@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
Re: [PATCH] Augment the awful "200 OK" error message when can't
connect to repository
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, Jan 11, 2010 at 1:29 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@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 a
t line 1: no element found (https://svn.apache.org/repos/)
]]]
I like my original error message, but consistency is more important IMHO.
--
Ivan Zhakov
VisualSVN Team
Re: [PATCH] Augment the awful "200 OK" error message when can't
connect to repository
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, Jan 11, 2010 at 1:29 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@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 a
t line 1: no element found (https://svn.apache.org/repos/)
]]]
I like my original error message, but consistency is more important IMHO.
--
Ivan Zhakov
VisualSVN Team
Re: [PATCH] Augment the awful "200 OK" error message when can't connect to repository
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@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?
--
Philip