You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2008/06/24 17:34:36 UTC
RA-Serf: misleading error message "Malformed URL"
A couple of improvement suggestions.
I noticed this in svn_ra_serf__get_lock():
> if (status_code == 404)
> {
> return svn_error_create(SVN_ERR_RA_ILLEGAL_URL, NULL,
> _("Malformed URL for repository"));
> }
If somebody hits this error, I expect they might be rather puzzled because their URL may not have been malformed, and they may not even have specified any URL at all on their original command. Better to (a) report the URL that failed, and (b) say more accurately what sort of error occurred.
And (c) doesn't this leak 'err' if 'err' is set and 'status_code' happens to contain 404 as well?
And immediately following that is this:
> if (err)
> {
> /* TODO Shh. We're telling a white lie for now. */
> return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, err,
> _("Server does not support locking features"));
> }
It would be better to be a bit more honest here as well, to make sure we don't waste the user's time when they go off trying to find out their server capabilities. Maybe say something like "A locking request failed for URL '%s'. Perhaps the server does not support locking features."
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: RA-Serf: misleading error message "Malformed URL"
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-06-24 at 15:25 -0400, Karl Fogel wrote:
> Julian Foad <ju...@btopenworld.com> writes:
> > A couple of improvement suggestions.
> >
> > I noticed this in svn_ra_serf__get_lock():
> >
> >> if (status_code == 404)
> >> {
> >> return svn_error_create(SVN_ERR_RA_ILLEGAL_URL, NULL,
> >> _("Malformed URL for repository"));
> >> }
> >
> > If somebody hits this error, I expect they might be rather puzzled
> > because their URL may not have been malformed, and they may not even
> > have specified any URL at all on their original command. Better to (a)
> > report the URL that failed, and (b) say more accurately what sort of
> > error occurred.
>
> +1, though what do we know about the error besides that it's a 404?
By "more accurately", I just meant we should say it's a "404" error
(preferably translated into English) rather than saying it's a
"malformed URL" error.
> > And (c) doesn't this leak 'err' if 'err' is set and 'status_code'
> > happens to contain 404 as well?
>
> E-yup.
>
> > And immediately following that is this:
> >
> >> if (err)
> >> {
> >> /* TODO Shh. We're telling a white lie for now. */
> >> return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, err,
> >> _("Server does not support locking features"));
> >> }
> >
> > It would be better to be a bit more honest here as well, to make sure
> > we don't waste the user's time when they go off trying to find out
> > their server capabilities. Maybe say something like "A locking request
> > failed for URL '%s'. Perhaps the server does not support locking
> > features."
>
> Much better, yes.
Great. Is there a Serf person here who'd like to do this? If not, just
ask and I'll have a go at it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: RA-Serf: misleading error message "Malformed URL"
Posted by Karl Fogel <kf...@red-bean.com>.
Julian Foad <ju...@btopenworld.com> writes:
> A couple of improvement suggestions.
>
> I noticed this in svn_ra_serf__get_lock():
>
>> if (status_code == 404)
>> {
>> return svn_error_create(SVN_ERR_RA_ILLEGAL_URL, NULL,
>> _("Malformed URL for repository"));
>> }
>
> If somebody hits this error, I expect they might be rather puzzled
> because their URL may not have been malformed, and they may not even
> have specified any URL at all on their original command. Better to (a)
> report the URL that failed, and (b) say more accurately what sort of
> error occurred.
+1, though what do we know about the error besides that it's a 404?
> And (c) doesn't this leak 'err' if 'err' is set and 'status_code'
> happens to contain 404 as well?
E-yup.
> And immediately following that is this:
>
>> if (err)
>> {
>> /* TODO Shh. We're telling a white lie for now. */
>> return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, err,
>> _("Server does not support locking features"));
>> }
>
> It would be better to be a bit more honest here as well, to make sure
> we don't waste the user's time when they go off trying to find out
> their server capabilities. Maybe say something like "A locking request
> failed for URL '%s'. Perhaps the server does not support locking
> features."
Much better, yes.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org