You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2013/07/09 15:27:21 UTC
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).
Bert Huijben wrote on Tue, Jul 09, 2013 at 14:20:27 +0200:
>
>
> > -----Original Message-----
> > From: danielsh@apache.org [mailto:danielsh@apache.org]
> > Sent: dinsdag 9 juli 2013 04:38
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1501049 - in /subversion/trunk/subversion:
> > include/svn_error_codes.h libsvn_ra_serf/util_error.c
> >
> > Author: danielsh
> > Date: Tue Jul 9 02:37:50 2013
> > New Revision: 1501049
> >
> > URL: http://svn.apache.org/r1501049
> > Log:
> > ra_serf: Do not return a serf apr_status_t.
> >
> > For me, this manifested as the following error chain (with the patch already
> > applied; before the patch, 120171 was the only code in the chain):
> >
> > % $svn info https://svn-us.apache.org/repos/asf
> > subversion/svn/info-cmd.c:663,
> > subversion/libsvn_client/info.c:300,
> > subversion/libsvn_client/ra.c:516,
> > subversion/libsvn_client/ra.c:393,
> > subversion/libsvn_ra/ra_loader.c:482:
> > (apr_err=SVN_ERR_RA_SERF_WRAPPED_ERROR)
> > svn: E230003: Unable to connect to a repository at URL 'https://svn-
> > us.apache.org/repos/asf'
> > subversion/libsvn_ra_serf/serf.c:506,
> > subversion/libsvn_ra_serf/options.c:508,
> > subversion/libsvn_ra_serf/util.c:817,
> > subversion/libsvn_ra_serf/util.c:784:
> > (apr_err=SVN_ERR_RA_SERF_WRAPPED_ERROR)
> > svn: E230003: Error running context: An error occurred during SSL
> > communication
> > subversion/libsvn_ra_serf/util.c:784: (apr_err=120171)
> > svn: E120171: APR does not understand this error code
> >
> > In that, 120171 is SERF_ERROR_SSL_COMM_FAILED, which is not a useful
> > value for
> > svn_error_t->apr_err (API users can't do anything with it).
>
> So without the patch a user sees:
>
> $ svn info https://svn.apache.org:80/
> svn: E120171: Unable to connect to a repository at URL 'https://svn.apache.org:80'
> svn: E120171: Error running context: An error occurred during SSL communication
>
> And with the patch
> $ svn info https://svn.apache.org:80/
> 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
>
> (Connecting as HTTPS to any HTTP host will trigger this error)
>
> As AnkhSVN developer I would say this is a regression that should be fixed. And as a normal 'svn' user I would reason the same way.
>
>
> I prefer to see the clear error codes going through, and not adding errors of the error handling.
>
> That your enum value generator patch doesn't work for you is your and maybe 'svn'-s problem, but certainly not worth hiding proper full status codes (such as from the OS, Apr, Serf) from clients.
>
> Bert
>
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
On Tue, Jul 09, 2013 at 05:12:23PM +0300, 'Daniel Shahaf' wrote:
> Bert Huijben wrote on Tue, Jul 09, 2013 at 15:34:56 +0200:
> > The tens of thousands of Windows defined error codes are not mapped by
> > Subversion or apr either, but they are certainly useful for applications. We
> > never documented that all of them must be mapped as enum value. apr_status
> > is an int for a reason.
> >
>
> False comparison. apr_err has a set of values specifically reserved for
> OS errors. An application can do APR_IS_OS_ERROR() followed by
> APR_FROM_OS_ERROR() followed by detailed inspection to its heart's
> contnet. Point being: inclusion of OS errors in the apr_status_t value
> is part of APR's API contract --- but including serf errors in
> err->apr_err is not part of our API.
BTW, the analogy continues: if we want to put 120171 in err->apr_err, we
should provide something in our API that lets our API consumers work with
it. For example:
#define SVN_ERROR_IS_SERF_ERROR(status) ((status) >= 120100 && (status) < 121000))
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Jul 09, 2013 at 16:47:36 +0200:
> Api users like those explicit error codes... and looking up the chain is not
> without errors; loses information etc.
Not in this case. Given ERR, you would only look in err->child->apr_err
(not in err->child->...->child->apr_err), and only if err->apr_err
is SVN_ERR_RA_SERF_WRAPPED_ERROR. The svn_error_compose_create()
problem doesn't happen here.
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Jul 09, 2013 at 16:47:36 +0200:
> Api users like those explicit error codes... and looking up the chain is not
> without errors; loses information etc.
Not in this case. Given ERR, you would only look in err->child->apr_err
(not in err->child->...->child->apr_err), and only if err->apr_err
is SVN_ERR_RA_SERF_WRAPPED_ERROR. The svn_error_compose_create()
problem doesn't happen here.
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Jul 09, 2013 at 20:02:48 +0200:
>
>
> > -----Original Message-----
> > From: 'Daniel Shahaf' [mailto:d.s@daniel.shahaf.name]
> > Sent: dinsdag 9 juli 2013 18:39
> > To: dev@subversion.apache.org
> > Cc: commits@subversion.apache.org
> > Subject: Re: svn commit: r1501049 - in /subversion/trunk/subversion:
> > include/svn_error_codes.h libsvn_ra_serf/util_error.c
> >
> > Daniel Shahaf wrote on Tue, Jul 09, 2013 at 16:20:56 +0000:
> > > 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.
> >
> > Looking at your comment:
> >
> > The reason for the -1 is the re-use of a specific error
> code
> > that is automatically unwrapped in some ra_serf code,
> to avoid
> > handling codes like APR_EOF as non fatal)
> >
> > Weere does ra_serf *unwrap* SVN_ERR_RA_SERF_WRAPPED_ERROR?
> >
> > % grep 'SVN_ERR.*SERF' subversion/*rf/*.[hc]
> > subversion/libsvn_ra_serf/options.c:
> > SVN_ERR_ASSERT(SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> > subversion/libsvn_ra_serf/options.c:
> > SVN_ERR_ASSERT(!SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> > subversion/libsvn_ra_serf/serf.c: SVN_ERR_ASSERT(!
> > SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> > subversion/libsvn_ra_serf/update.c: return
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util.c: return
> > svn_error_create(SVN_ERR_RA_SERF_SSL_CERT_UNTRUSTED, NULL, NULL);
> > subversion/libsvn_ra_serf/util.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util_error.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > % grep 'SVN_ERR.*SERF' subversion/*{ra,client} | wc -l
>
> Slightly more context:
> I see 5 usages:
> 1:
> /* Some errors would be handled by serf; make sure they really make
> the update fail by wrapping it in a different error. */
> if (!SERF_BUCKET_READ_ERROR(err->apr_err))
> return svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 2:
> if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 3:
> if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 4:
> if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 5:
> if (serf_err_msg)
> {
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> err_msg = serf_err_msg;
> }
>
> Which one is not specifically mapping specific error types?
>
I don't understand the question. All 5 uses you paste above use
SVN_ERR_RA_SERF_WRAPPED_ERROR in exactly the same manner: as a wrapper
error code that is propagated directly to libsvn_ra's caller.
What problem do you see in usage 5?
> Note that all of these cases handle specific APR error codes as specific for
> SERF.
> (except the last one of course)
>
> Bert
>
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, Jul 09, 2013 at 20:02:48 +0200:
>
>
> > -----Original Message-----
> > From: 'Daniel Shahaf' [mailto:d.s@daniel.shahaf.name]
> > Sent: dinsdag 9 juli 2013 18:39
> > To: dev@subversion.apache.org
> > Cc: commits@subversion.apache.org
> > Subject: Re: svn commit: r1501049 - in /subversion/trunk/subversion:
> > include/svn_error_codes.h libsvn_ra_serf/util_error.c
> >
> > Daniel Shahaf wrote on Tue, Jul 09, 2013 at 16:20:56 +0000:
> > > 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.
> >
> > Looking at your comment:
> >
> > The reason for the -1 is the re-use of a specific error
> code
> > that is automatically unwrapped in some ra_serf code,
> to avoid
> > handling codes like APR_EOF as non fatal)
> >
> > Weere does ra_serf *unwrap* SVN_ERR_RA_SERF_WRAPPED_ERROR?
> >
> > % grep 'SVN_ERR.*SERF' subversion/*rf/*.[hc]
> > subversion/libsvn_ra_serf/options.c:
> > SVN_ERR_ASSERT(SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> > subversion/libsvn_ra_serf/options.c:
> > SVN_ERR_ASSERT(!SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> > subversion/libsvn_ra_serf/serf.c: SVN_ERR_ASSERT(!
> > SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> > subversion/libsvn_ra_serf/update.c: return
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util.c: return
> > svn_error_create(SVN_ERR_RA_SERF_SSL_CERT_UNTRUSTED, NULL, NULL);
> > subversion/libsvn_ra_serf/util.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > subversion/libsvn_ra_serf/util_error.c: err =
> > svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> > % grep 'SVN_ERR.*SERF' subversion/*{ra,client} | wc -l
>
> Slightly more context:
> I see 5 usages:
> 1:
> /* Some errors would be handled by serf; make sure they really make
> the update fail by wrapping it in a different error. */
> if (!SERF_BUCKET_READ_ERROR(err->apr_err))
> return svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 2:
> if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 3:
> if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 4:
> if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
>
> 5:
> if (serf_err_msg)
> {
> err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> err_msg = serf_err_msg;
> }
>
> Which one is not specifically mapping specific error types?
>
I don't understand the question. All 5 uses you paste above use
SVN_ERR_RA_SERF_WRAPPED_ERROR in exactly the same manner: as a wrapper
error code that is propagated directly to libsvn_ra's caller.
What problem do you see in usage 5?
> Note that all of these cases handle specific APR error codes as specific for
> SERF.
> (except the last one of course)
>
> Bert
>
RE: svn commit: r1501049 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: 'Daniel Shahaf' [mailto:d.s@daniel.shahaf.name]
> Sent: dinsdag 9 juli 2013 18:39
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1501049 - in /subversion/trunk/subversion:
> include/svn_error_codes.h libsvn_ra_serf/util_error.c
>
> Daniel Shahaf wrote on Tue, Jul 09, 2013 at 16:20:56 +0000:
> > 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.
>
> Looking at your comment:
>
> The reason for the -1 is the re-use of a specific error
code
> that is automatically unwrapped in some ra_serf code,
to avoid
> handling codes like APR_EOF as non fatal)
>
> Weere does ra_serf *unwrap* SVN_ERR_RA_SERF_WRAPPED_ERROR?
>
> % grep 'SVN_ERR.*SERF' subversion/*rf/*.[hc]
> subversion/libsvn_ra_serf/options.c:
> SVN_ERR_ASSERT(SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> subversion/libsvn_ra_serf/options.c:
> SVN_ERR_ASSERT(!SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> subversion/libsvn_ra_serf/serf.c: SVN_ERR_ASSERT(!
> SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> subversion/libsvn_ra_serf/update.c: return
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util.c: return
> svn_error_create(SVN_ERR_RA_SERF_SSL_CERT_UNTRUSTED, NULL, NULL);
> subversion/libsvn_ra_serf/util.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util_error.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> % grep 'SVN_ERR.*SERF' subversion/*{ra,client} | wc -l
Slightly more context:
I see 5 usages:
1:
/* Some errors would be handled by serf; make sure they really make
the update fail by wrapping it in a different error. */
if (!SERF_BUCKET_READ_ERROR(err->apr_err))
return svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
2:
if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
3:
if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
4:
if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
5:
if (serf_err_msg)
{
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
err_msg = serf_err_msg;
}
Which one is not specifically mapping specific error types?
Note that all of these cases handle specific APR error codes as specific for
SERF.
(except the last one of course)
Bert
RE: svn commit: r1501049 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: 'Daniel Shahaf' [mailto:d.s@daniel.shahaf.name]
> Sent: dinsdag 9 juli 2013 18:39
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1501049 - in /subversion/trunk/subversion:
> include/svn_error_codes.h libsvn_ra_serf/util_error.c
>
> Daniel Shahaf wrote on Tue, Jul 09, 2013 at 16:20:56 +0000:
> > 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.
>
> Looking at your comment:
>
> The reason for the -1 is the re-use of a specific error
code
> that is automatically unwrapped in some ra_serf code,
to avoid
> handling codes like APR_EOF as non fatal)
>
> Weere does ra_serf *unwrap* SVN_ERR_RA_SERF_WRAPPED_ERROR?
>
> % grep 'SVN_ERR.*SERF' subversion/*rf/*.[hc]
> subversion/libsvn_ra_serf/options.c:
> SVN_ERR_ASSERT(SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> subversion/libsvn_ra_serf/options.c:
> SVN_ERR_ASSERT(!SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> subversion/libsvn_ra_serf/serf.c: SVN_ERR_ASSERT(!
> SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
> subversion/libsvn_ra_serf/update.c: return
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util.c: return
> svn_error_create(SVN_ERR_RA_SERF_SSL_CERT_UNTRUSTED, NULL, NULL);
> subversion/libsvn_ra_serf/util.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> subversion/libsvn_ra_serf/util_error.c: err =
> svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
> % grep 'SVN_ERR.*SERF' subversion/*{ra,client} | wc -l
Slightly more context:
I see 5 usages:
1:
/* Some errors would be handled by serf; make sure they really make
the update fail by wrapping it in a different error. */
if (!SERF_BUCKET_READ_ERROR(err->apr_err))
return svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
2:
if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
3:
if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
4:
if (err && !SERF_BUCKET_READ_ERROR(err->apr_err))
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
5:
if (serf_err_msg)
{
err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
err_msg = serf_err_msg;
}
Which one is not specifically mapping specific error types?
Note that all of these cases handle specific APR error codes as specific for
SERF.
(except the last one of course)
Bert
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Jul 09, 2013 at 16:20:56 +0000:
> 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.
Looking at your comment:
The reason for the -1 is the re-use of a specific error code
that is automatically unwrapped in some ra_serf code, to avoid
handling codes like APR_EOF as non fatal)
Weere does ra_serf *unwrap* SVN_ERR_RA_SERF_WRAPPED_ERROR?
% grep 'SVN_ERR.*SERF' subversion/*rf/*.[hc]
subversion/libsvn_ra_serf/options.c: SVN_ERR_ASSERT(SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
subversion/libsvn_ra_serf/options.c: SVN_ERR_ASSERT(!SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
subversion/libsvn_ra_serf/serf.c: SVN_ERR_ASSERT(! SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
subversion/libsvn_ra_serf/update.c: return svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util.c: return svn_error_create(SVN_ERR_RA_SERF_SSL_CERT_UNTRUSTED, NULL, NULL);
subversion/libsvn_ra_serf/util.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util_error.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
% grep 'SVN_ERR.*SERF' subversion/*{ra,client} | wc -l
0
%
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Jul 09, 2013 at 16:20:56 +0000:
> 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.
Looking at your comment:
The reason for the -1 is the re-use of a specific error code
that is automatically unwrapped in some ra_serf code, to avoid
handling codes like APR_EOF as non fatal)
Weere does ra_serf *unwrap* SVN_ERR_RA_SERF_WRAPPED_ERROR?
% grep 'SVN_ERR.*SERF' subversion/*rf/*.[hc]
subversion/libsvn_ra_serf/options.c: SVN_ERR_ASSERT(SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
subversion/libsvn_ra_serf/options.c: SVN_ERR_ASSERT(!SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
subversion/libsvn_ra_serf/serf.c: SVN_ERR_ASSERT(! SVN_RA_SERF__HAVE_HTTPV2_SUPPORT(session));
subversion/libsvn_ra_serf/update.c: return svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util.c: return svn_error_create(SVN_ERR_RA_SERF_SSL_CERT_UNTRUSTED, NULL, NULL);
subversion/libsvn_ra_serf/util.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
subversion/libsvn_ra_serf/util_error.c: err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err, NULL);
% grep 'SVN_ERR.*SERF' subversion/*{ra,client} | wc -l
0
%
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
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
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
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
RE: svn commit: r1501049 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----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.
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
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Daniel Shahaf <da...@apache.org>.
On Tue, Jul 09, 2013 at 05:12:23PM +0300, 'Daniel Shahaf' wrote:
> Bert Huijben wrote on Tue, Jul 09, 2013 at 15:34:56 +0200:
> > The tens of thousands of Windows defined error codes are not mapped by
> > Subversion or apr either, but they are certainly useful for applications. We
> > never documented that all of them must be mapped as enum value. apr_status
> > is an int for a reason.
> >
>
> False comparison. apr_err has a set of values specifically reserved for
> OS errors. An application can do APR_IS_OS_ERROR() followed by
> APR_FROM_OS_ERROR() followed by detailed inspection to its heart's
> contnet. Point being: inclusion of OS errors in the apr_status_t value
> is part of APR's API contract --- but including serf errors in
> err->apr_err is not part of our API.
BTW, the analogy continues: if we want to put 120171 in err->apr_err, we
should provide something in our API that lets our API consumers work with
it. For example:
#define SVN_ERROR_IS_SERF_ERROR(status) ((status) >= 120100 && (status) < 121000))
RE: svn commit: r1501049 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----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.
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
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
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);
return err;
}
]]]
changes the stack trace to:
svn: E230003: Unable to connect to a repository at URL 'https://svn.apache.org:80'
svn: E230003: While handling serf response:
svn: E120171: Error running context: An error occurred during SSL communication
>
> The tens of thousands of Windows defined error codes are not mapped by
> Subversion or apr either, but they are certainly useful for applications. We
> never documented that all of them must be mapped as enum value. apr_status
> is an int for a reason.
>
False comparison. apr_err has a set of values specifically reserved for
OS errors. An application can do APR_IS_OS_ERROR() followed by
APR_FROM_OS_ERROR() followed by detailed inspection to its heart's
contnet. Point being: inclusion of OS errors in the apr_status_t value
is part of APR's API contract --- but including serf errors in
err->apr_err is not part of our API.
> Why do you want to break existing code in TortoiseSVN, AnkhSVN, Subclipse,
> etc. etc.?
As I already said, the only way applications can depend on the previous
error chain is if they do:
if (err->apr_err == 120171)
...
or if they do:
#include <serf.h>
if (err->apr_err == SERF_ERROR_SSL_COMM_FAILED)
...
Applications that use svn_error_find_cause() are unaffected.
Re: svn commit: r1501049 - in /subversion/trunk/subversion:
include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
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);
return err;
}
]]]
changes the stack trace to:
svn: E230003: Unable to connect to a repository at URL 'https://svn.apache.org:80'
svn: E230003: While handling serf response:
svn: E120171: Error running context: An error occurred during SSL communication
>
> The tens of thousands of Windows defined error codes are not mapped by
> Subversion or apr either, but they are certainly useful for applications. We
> never documented that all of them must be mapped as enum value. apr_status
> is an int for a reason.
>
False comparison. apr_err has a set of values specifically reserved for
OS errors. An application can do APR_IS_OS_ERROR() followed by
APR_FROM_OS_ERROR() followed by detailed inspection to its heart's
contnet. Point being: inclusion of OS errors in the apr_status_t value
is part of APR's API contract --- but including serf errors in
err->apr_err is not part of our API.
> Why do you want to break existing code in TortoiseSVN, AnkhSVN, Subclipse,
> etc. etc.?
As I already said, the only way applications can depend on the previous
error chain is if they do:
if (err->apr_err == 120171)
...
or if they do:
#include <serf.h>
if (err->apr_err == SERF_ERROR_SSL_COMM_FAILED)
...
Applications that use svn_error_find_cause() are unaffected.
RE: svn commit: r1501049 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----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.
The tens of thousands of Windows defined error codes are not mapped by
Subversion or apr either, but they are certainly useful for applications. We
never documented that all of them must be mapped as enum value. apr_status
is an int for a reason.
And at least one other public use of the currently undefined values is that
applications can use them themselves and are free to return them from
callbacks for specific error cases.
Do you want to map all of these to SVN_ERR_DOESNT_UNDERSTAND_THIS too?
We shouldn't be in the business of hiding things for bindings that currently
use this behavior.
Are you a binding developer or binding user?
Why do you want to break existing code in TortoiseSVN, AnkhSVN, Subclipse,
etc. etc.?
End users want clear *messages*
Applications want clear *codes*
Your patch breaks both cases, by making the message harder to understand and
hiding the code further in the error path.
Bert
RE: svn commit: r1501049 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_ra_serf/util_error.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----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.
The tens of thousands of Windows defined error codes are not mapped by
Subversion or apr either, but they are certainly useful for applications. We
never documented that all of them must be mapped as enum value. apr_status
is an int for a reason.
And at least one other public use of the currently undefined values is that
applications can use them themselves and are free to return them from
callbacks for specific error cases.
Do you want to map all of these to SVN_ERR_DOESNT_UNDERSTAND_THIS too?
We shouldn't be in the business of hiding things for bindings that currently
use this behavior.
Are you a binding developer or binding user?
Why do you want to break existing code in TortoiseSVN, AnkhSVN, Subclipse,
etc. etc.?
End users want clear *messages*
Applications want clear *codes*
Your patch breaks both cases, by making the message harder to understand and
hiding the code further in the error path.
Bert