> -----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.
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.
Every component can declare new integer values within its own ranges and
Subversion should be transparent with error codes: return them up the chain.
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.
Bert
Received on 2013-07-09 16:48:41 CEST