On Tue, Jul 09, 2013 at 04:47:36PM +0200, Bert Huijben wrote: > > > > -----Original Message----- > > From: 'Daniel Shahaf' [mailto:d...@daniel.shahaf.name] > > Sent: dinsdag 9 juli 2013 16:12 > > To: Bert Huijben > > Cc: dev@subversion.apache.org; comm...@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...@daniel.shahaf.name] > > > > Sent: dinsdag 9 juli 2013 15:27 > > > > To: Bert Huijben > > > > Cc: dev@subversion.apache.org; comm...@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
