subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <danie...@apache.org>
Subject Re: svn commit: r1501049 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_ra_serf/util_error.c
Date Tue, 09 Jul 2013 16:20:56 GMT
On Tue, Jul 09, 2013 at 04:47:36PM +0200, Bert Huijben wrote:
> 
> 
> > -----Original Message-----
> > From: 'Daniel Shahaf' [mailto:d.s@daniel.shahaf.name]
> > Sent: dinsdag 9 juli 2013 16:12
> > To: Bert Huijben
> > Cc: dev@subversion.apache.org; commits@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@daniel.shahaf.name]
> > > > Sent: dinsdag 9 juli 2013 15:27
> > > > To: Bert Huijben
> > > > Cc: dev@subversion.apache.org; commits@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

Mime
View raw message