On Tue, 2008-06-24 at 15:25 -0400, Karl Fogel wrote:
> Julian Foad <julianfoad_at_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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-24 22:38:57 CEST