Re: svn commit: r1186791 - /subversion/branches/1.7.x/STATUS

On Thu, Oct 20, 2011 at 8:18 AM, <ivan_at_apache.org> wrote:
> Author: ivan
> Date: Thu Oct 20 13:18:02 2011
> New Revision: 1186791
>
> URL: http://svn.apache.org/viewvc?rev=1186791&view=rev
> Log:
> * STATUS: Cast a -0 vote for the r1185746 change.
>
> Modified:
>    subversion/branches/1.7.x/STATUS
>
> Modified: subversion/branches/1.7.x/STATUS
> URL: http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?rev=1186791&r1=1186790&r2=1186791&view=diff
> ==============================================================================
> --- subversion/branches/1.7.x/STATUS (original)
> +++ subversion/branches/1.7.x/STATUS Thu Oct 20 13:18:02 2011
> @@ -28,6 +28,8 @@ Candidate changes:
>    Fix up some erroneous "Could not frob some targets because..." warnings.
>    Votes:
>      +1: stsp, rhuijben
> +     -0: ivan (breaking ABI even for private function is not good thing for
> +               patch release)

I don't understand how this is breaking ABI. The only function
signature that changes in r1185746 is this:

[[[
Index: subversion/svn/cl.h
===================================================================
--- subversion/svn/cl.h (revision 1185745)
+++ subversion/svn/cl.h (revision 1185746)
@@ -294,15 +294,14 @@
  * invoked on an unversioned, nonexistent, or otherwise innocuously
  * errorful resource. Meant to be wrapped with SVN_ERR().
  *
- * If ERR is null, return SVN_NO_ERROR, setting *SUCCESS to TRUE
- * if SUCCESS is not NULL.
+ * If ERR is null, return SVN_NO_ERROR.
  *
  * Else if ERR->apr_err is one of the error codes supplied in varargs,
  * then handle ERR as a warning (unless QUIET is true), clear ERR, and
- * return SVN_NO_ERROR, setting *SUCCESS to FALSE if SUCCESS is not
- * NULL.
+ * return SVN_NO_ERROR, and push the value of ERR->apr_err into the
+ * ERRORS_SEEN array, if ERRORS_SEEN is not NULL.
  *
- * Else return ERR, setting *SUCCESS to FALSE if SUCCESS is not NULL.
+ * Else return ERR.
  *
  * Typically, error codes like SVN_ERR_UNVERSIONED_RESOURCE,
  * SVN_ERR_ENTRY_NOT_FOUND, etc, are supplied in varargs. Don't
@@ -310,7 +309,7 @@
  */
 svn_error_t *
 svn_cl__try(svn_error_t *err,
- svn_boolean_t *success,
+ apr_array_header_t *errors_seen,
             svn_boolean_t quiet,
             ...);

]]]

That's a binary-private function, completely within the command line
client. It doesn't cross any library boundaries. We make these types
of changes frequently in patch releases, which makes me a little
confused as to the reasoning behind your -0.

Full disclosure: although the code is a bit too verbose for my liking,
I was going to +1 the change before your -0 came in. :)

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/