> -----Original Message-----
> From: Branko Čibej [mailto:brane_at_wandisco.com]
> Sent: dinsdag 9 juli 2013 22:48
> To: dev_at_subversion.apache.org
> Subject: 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.
Agree:
But I think we should do that by setting the right message before an svn_error_t * exits libsvn_ra_serf, not by wrapping the error again or by introducing yet another item in the error chain.
But note that the original patch this discussion started with, did exactly the opposite: It decoupled the message and the error code, which made svn produce the warning that it didn't understand the error.
So: how about agree-ing that introducing this intermediate, untranslatable error was not the right thing to do?
Note also the replies from Lieven and Ivan that said that the return path in ra_serf should be verified to make sure the right text is provided.
>
>
> > 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.
We don't wrap them in many cases.
I think about 1 in every 3 error reports I get for AnkhSVN is an OS error wrapped via APR. And these values for different kinds of file-sharing problems and detailed winsock errors certainly aren't mapped by APR values.
(Although APR_EGENERAL is rising in the popularity contest for 1.8.0 :( )
I also return error codes outside the OS / Subversion / Serf range in SharpSvn to have some of my own conditions get through at the other side of wrapping.
And as far as I can tell this is what the apr status (with its ranges) values were designed for.
I would welcome a patch to svn_strerror that improves the default error for serf specific error codes, or a patch that translates all serf error codes to something within a subversion error range.
But I don't want to see the tens of thousands of defined errors in the Windows error header file being mapped to a single unknown Subversion error code for 'I don't understand'. It is better to return the proper error string and the value, so API users that do understand the code can use it.
Bert
>
> -- Brane
>
> --
> Branko Čibej | Director of Subversion
> WANdisco // Non-Stop Data
> e. brane_at_wandisco.com
Received on 2013-07-09 23:01:32 CEST