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

> -----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.

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.

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).

I would reason like Raymond Chen (of the oldnewthing fame) here: every
change starts at -1000 points. And needs a good reason to be implemented as
new feature, and an even better reason to be backported as 'fix' to a
maintenance branch.

        Bert
Received on 2013-07-09 22:33:02 CEST