Re: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c

On 09.07.2013 22:32, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
>> Sent: dinsdag 9 juli 2013 22:16
>> To: Bert Huijben
>> Cc: dev_at_subversion.apache.org; commits_at_subversion.apache.org
>> Subject: Re: svn commit: r1501371 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
>>
>> Bert Huijben wrote on Tue, Jul 09, 2013 at 20:11:57 +0200:
>>> Same as for all previous patches in line:
>>>
>> "All previous patches"? There was one previous patch along these lines.
>>
>>> How is this going to help?
>> I already told you how: it is going to help because API users can't do
>> anything with the value 120171 that they presently receive. The
>> outermost error must be defined by APR or by Subversion. 120171 is
>> neither.
> *Why* ?
>
> There is no rule that apr_err must be set to something that is defined by
> APR or Subversion.

Not as such. But I hope we can agree that there's an implied rule that
apr_strerror and/or svn_strerror should return a valid error message,
not complain about not understanding the error code.

> Besides APR *defines* everything in a range to be user defined..
>
>
> This same reasoning applies to wrapping everything to APR_EGENERAL, which is
> just as bad.
>
> Every integer value is valid here... And allows checking for it.
>
> APR_EGENERAL (=1) doesn't allow checking for by api users. And bindings
> can't translate textual error messages back to text.

The actual Serf error code is still there, in the error chain, for
anyone who wants to bother looking for it. But it's really bad user
experience to see the message "APR can't understand this error" in
response to something that we /can/ be more specific about.

> So *why* do *you* want to change the existing error code to this wrapping
> code that is only used wrapping for non-bucket errors. (Which is a specific
> set of APR defined error codes that are handled specificly in serf).

Daniel said this a number of times: The Serf API (and, as a consequence,
Serf's error codes) are not part of our public API. Why then should we
leave those error codes unwrapped? We wrap everything else, including Win32.

-- Brane

-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com