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);
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.
Received on 2013-07-09 16:13:03 CEST