Re: svn commit: r38686 - in trunk/subversion: include libsvn_ra_svn svnsync

[Stefan Sperling <st...@elego.de>]

On Tue, Aug 11, 2009 at 01:36:51PM -0700, Ben Collins-Sussman wrote:
> Author: sussman
> Date: Tue Aug 11 13:36:50 2009
> New Revision: 38686
> 
> Log:
> Fix two serious bugs in libsvn_ra_svn and svnsync.

I only have stylistic nits.

> Modified: trunk/subversion/include/svn_error_codes.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_error_codes.h?pathrev=38686&r1=38685&r2=38686
> ==============================================================================
> --- trunk/subversion/include/svn_error_codes.h	Tue Aug 11 13:27:24 2009	(r38685)
> +++ trunk/subversion/include/svn_error_codes.h	Tue Aug 11 13:36:50 2009	(r38686)
> @@ -929,11 +929,17 @@ SVN_ERROR_START
>               SVN_ERR_RA_SVN_CATEGORY_START + 6,
>               "Client/server version mismatch")
>  
> -  /** @since New in 1.5. */
> +    /** @since New in 1.5. */

Indentation here looks off.

>    SVN_ERRDEF(SVN_ERR_RA_SVN_NO_MECHANISMS,
>               SVN_ERR_RA_SVN_CATEGORY_START + 7,
>               "Cannot negotiate authentication mechanism")
>  
> +   /** @since New in 1.6.5 / 1.7  */

Same here.

> +  SVN_ERRDEF(SVN_ERR_RA_SVN_EDIT_ABORTED,
> +             SVN_ERR_RA_SVN_CATEGORY_START + 8,
> +             "Editor drive was aborted")
> +
> +
>    /* libsvn_ra_serf errors */
>    /** @since New in 1.5. */
>    SVN_ERRDEF(SVN_ERR_RA_SERF_SSPI_INITIALISATION_FAILED,
> 
> Modified: trunk/subversion/libsvn_ra_svn/client.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_svn/client.c?pathrev=38686&r1=38685&r2=38686
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/client.c	Tue Aug 11 13:27:24 2009	(r38685)
> +++ trunk/subversion/libsvn_ra_svn/client.c	Tue Aug 11 13:36:50 2009	(r38686)
> @@ -2253,6 +2253,7 @@ ra_svn_replay_range(svn_ra_session_t *se
>    svn_ra_svn__session_baton_t *sess = session->priv;
>    apr_pool_t *iterpool;
>    svn_revnum_t rev;
> +  svn_boolean_t drive_aborted = FALSE;
>  
>    SVN_ERR(svn_ra_svn_write_cmd(sess->conn, pool, "replay-range", "rrrb",
>                                 start_revision, end_revision,
> @@ -2288,7 +2289,14 @@ ra_svn_replay_range(svn_ra_session_t *se
>                              iterpool));
>        SVN_ERR(svn_ra_svn_drive_editor2(sess->conn, iterpool,
>                                         editor, edit_baton,
> -                                       NULL, TRUE));
> +                                       &drive_aborted, TRUE));
> +      /* If drive_editor2() aborted the commit, do NOT try to call
> +         revfinish_func and commit the transaction! */
> +      if (drive_aborted) {

I like brace-on-same-line better, too, but HACKING disagrees.

> +        svn_pool_destroy(iterpool);
> +        return svn_error_create(SVN_ERR_RA_SVN_EDIT_ABORTED, NULL,
> +                                _("Commit error during replay_range()"));

We rarely mention function names in error messages, especially
in those marked for translation. I can already see the german version
"Commit Fehler während wiederhole_intervall()" and while this looks funny
it's not user-friendly.

How about: _("Commit error while replaying revision range")); ?

Stefan

> +      }
>        SVN_ERR(revfinish_func(rev, replay_baton,
>                               editor, edit_baton,
>                               rev_props,

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2382677