On Tue, Jul 09, 2013 at 04:47:36PM +0200, Bert Huijben wrote:
>
>
> > -----Original Message-----
> > From: 'Daniel Shahaf' [mailto:d.s_at_daniel.shahaf.name]
> > Sent: dinsdag 9 juli 2013 16:12
> > To: Bert Huijben
> > Cc: dev_at_subversion.apache.org; commits_at_subversion.apache.org
> > Subject: Re: svn commit: r1501049 - in /subversion/trunk/subversion:
> > include/svn_error_codes.h libsvn_ra_serf/util_error.c
> >
> > Bert Huijben wrote on Tue, Jul 09, 2013 at 15:34:56 +0200:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
> > > > Sent: dinsdag 9 juli 2013 15:27
> > > > To: Bert Huijben
> > > > Cc: dev_at_subversion.apache.org; commits_at_subversion.apache.org
> > > > Subject: Re: svn commit: r1501049 - in /subversion/trunk/subversion:
> > > > include/svn_error_codes.h libsvn_ra_serf/util_error.c
> > > >
> > > > Can you please give me a little credit? The problem here is not that
> > > > 120171 doesn't get stringified in maintainer mode.
> > > >
> > > > The problem here is that it is a number which _does not have_ a
> symbolic
> > > > name in APR's or Subversion's API, so if an API user wants to code
> > > > against it they need to either hard-code the number or #include
> <serf.h>
> > > > in their builds --- and last I checked, we didn't require API users to
> > > > do either of those. Compare this to sqlite: we wrap all SQLite error
> > > > integers to SVN_ERR_SQLITE_ERROR, except one or two which we
> > decided
> > > > to
> > > > be important so we get them their own svn error numbers.
> > > >
> > > > And it's not hiding any codes, they are still in the chain as
> > > > err->child->apr_err (or svn_error_find_cause() if you prefer that).
> > >
> > > Users don't care about the enum mapping. All they see is an error like:
> > >
> > > svn: E230003: Unable to connect to a repository at URL
> > > 'https://svn.apache.org:80'
> > > svn: E230003: Error running context: An error occurred during SSL
> > > communication
> > > svn: E120171: APR does not understand this error code
> > >
> > > ^^^ I don't think this new line you added in r1501049 doesn't help any
> user.
> > >
> >
> > Yes, that's a bug. I'm not sure why you went to the effort of vetoing
> > the patch and yelling at me in order to point it out.
> >
> > This patch:
> >
> > [[[
> > Index: subversion/libsvn_ra_serf/util_error.c
> > ==========================================================
> > =========
> > --- subversion/libsvn_ra_serf/util_error.c (revision 1501266)
> > +++ subversion/libsvn_ra_serf/util_error.c (working copy)
> > @@ -61,7 +61,6 @@ svn_ra_serf__wrap_err(apr_status_t status,
> >
> > if (serf_err_msg)
> > {
> > - err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err,
> > NULL);
> > err_msg = serf_err_msg;
> > }
> > else
> > @@ -96,6 +95,10 @@ svn_ra_serf__wrap_err(apr_status_t status,
> > err->message = msg;
> > }
> > }
> > +
> > + /* Make the outer-most error code be a Subversion/APR one. */
> > + if (serf_err_msg)
> > + err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err,
> > NULL);
>
> I still don't see any good reason to do this.
>
> SVN_ERR_RA_SERF_WRAPPED_ERROR didn't exist in Subversion 1.0, and this makes
> it just as worse as the original error. An 1.0 binding user wouldn't be able
> to use this error for the same reasoning.
>
When you meet someone who still uses the 1.0 bindings API please introduce me
to them. (Also, neither 120171 nor SVN_ERR_RA_SERF_WRAPPED_ERROR existed in
1.0, so a 1.0 API user won't care which of them we present to him.)
> We can't declare the only valid errors to be the errors in svn_error_codes.h
> and we never will declare that to be the only valid error codes.
>
Huh?
When I get a non-SVN_NO_ERROR svn_error_t, I expect the APR_ERR member thereof
to be either a Subversion-defined error code or an APR-defined one (where
APR-defined ones include OS errors via APR_FROM_OS_ERROR()).
> Every component can declare new integer values within its own ranges and
> Subversion should be transparent with error codes: return them up the chain.
>
Those error codes are useless. When Ankhsvn gets 120171 as the error code,
what can you do with it? It is in the APR_OS_START_USERERR range, but it is
not in any SVN_ERROR_IN_CATEGORY(SVN_ERR_CATEGORY_*), so all you know is "Some
APR-using dependency of Subversion returned it".
Again: nothing, unless you include serf.h in your Ankhsvn build. (And what
will you do when we link another APR-using dependency that has another meaning
for 120171? The SVN_ERR_RA_SERF_* error serves to disambiguate: "my
->child->apr_err is to be interpreted using serf_error_string()".)
> If 'svn' doesn't like this, that should be fixed in 'svn'.
>
> Api users like those explicit error codes... and looking up the chain is not
> without errors; loses information etc.
ADDING codes to the chain does not LOSE information.
Sorry, I still haven't heard a valid reason for your veto, other than "API
users who expect 120171 to be in the top-most error of the chain will be
broken" --- and, again, I submit the top-most apr_err must be one defined
either by APR or by *the Subversion meanings of* the APR_OS_START_USERERR
range.
Can you point me to a line number in any API user's code that actually depends
on that 120171 value? Does Ankhsvn have some version of the
SVN_ERROR_IS_SERF_ERROR() I posted elsethread?
I accept that some API users may depend on SVN_ERR_RA_SERF_WRAPPED_ERROR. Do
you think it is a problem to assign that new meaning to it in 1.8.x? I reused
it for the same reasons you re-used an existing error code in r1498851, if you
think a new error code is needed on trunk I'm happy to add one.
All that said, I'll go ahead and commit + nominate my previous patch. Worst
case, if consensus goes your way I'll revert it.
Daniel
Received on 2013-07-09 18:21:04 CEST