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?
> 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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-24 21:26:27 CEST