> -----Original Message-----
> From: danielsh_at_apache.org [mailto:danielsh_at_apache.org]
> Sent: dinsdag 9 juli 2013 18:41
> To: commits_at_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
Received on 2013-07-09 20:13:02 CEST