subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
Subject RE: svn commit: r1501371 - /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
Date Tue, 09 Jul 2013 18:11:57 GMT


> -----Original Message-----
> From: danielsh@apache.org [mailto:danielsh@apache.org]
> Sent: dinsdag 9 juli 2013 18:41
> To: commits@subversion.apache.org
> Subject: svn commit: r1501371 -
> /subversion/trunk/subversion/libsvn_ra_serf/util_error.c
> 
> Author: danielsh
> Date: Tue Jul  9 16:41:22 2013
> New Revision: 1501371
> 
> URL: http://svn.apache.org/r1501371
> Log:
> Fix bug in r1501049.
> 
> Found by: rhuijben
> 
> * subversion/libsvn_ra_serf/util_error.c
>   (svn_ra_serf__wrap_err): Wrap ERR later since the incumbent pointer value
> is
>     needed to set its ->message member correctly; it was set to "APR does not
>     understand this error code"  at line
>     54:  err = svn_error_create(status, NULL, NULL);
>     because STATUS was neither a Subversion error code nor an APR one, and
> is
>     being overriden in lines 89:96.
> 
> Modified:
>     subversion/trunk/subversion/libsvn_ra_serf/util_error.c
> 
> Modified: subversion/trunk/subversion/libsvn_ra_serf/util_error.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/
> util_error.c?rev=1501371&r1=1501370&r2=1501371&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_ra_serf/util_error.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/util_error.c Tue Jul  9
> 16:41:22 2013
> @@ -61,7 +61,6 @@ svn_ra_serf__wrap_err(apr_status_t statu
> 
>        if (serf_err_msg)
>          {
> -          err = svn_error_create(SVN_ERR_RA_SERF_WRAPPED_ERROR, err,
> NULL);
>            err_msg = serf_err_msg;
>          }
>        else
> @@ -97,5 +96,9 @@ svn_ra_serf__wrap_err(apr_status_t statu
>          }
>      }
> 
> +  /* 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);

Same as for all previous patches in line:

How is this going to help?

We always use the pattern

if (err && err->apr_err = 1234567)
to check for values

And in a very few cases we check the root cause, but then we do lose more than a bit of information
as that breaks over combining errors.


To users there is absolutely no value in adding the knowledge that an integer maps to whatever
source. Api users that link specific components already know that and users want a meaningful
error.


Let's show that every error is caused by 'serf' and everybody wants to go back to 'neon'?
(or maybe 'curl')

I only hear your arguments and nothing for our users to gain here.


You are nominating 'a change' for 1.8. 
You should tell us why this change is important to our users and not a breaking change to
others.

Adding more text that users don't understand to an error is not a positive change and neither
is adding yet another integer to an error that users don't want to see.

And API users want to see the most interesting/unique value for every error, and wrapping
with generic codes is exactly working against that.

	Bert 


Mime
View raw message