[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1033320 - in /subversion/trunk/subversion: include/svn_error_codes.h svn/cl.h svn/export-cmd.c svn/notify.c svn/switch-cmd.c svn/update-cmd.c tests/cmdline/externals_tests.py

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Wed, 10 Nov 2010 10:21:38 +0000

On Wed, 2010-11-10, cmpilato_at_apache.org wrote:
> Author: cmpilato
> Date: Wed Nov 10 01:44:35 2010
> New Revision: 1033320
>
> URL: http://svn.apache.org/viewvc?rev=1033320&view=rev
> Log:
> Fix issue #3622 ("svn should exit with exit code 1 if updating
> externals fails").
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_CL_ERROR_PROCESSING_EXTERNALS): New error code.
>
> * subversion/svn/cl.h
> (struct svn_cl__check_externals_failed_notify_baton): New baton.
> (svn_cl__check_externals_failed_notify_wrapper): New function.
>
> * subversion/svn/notify.c
> (svn_cl__check_externals_failed_notify_wrapper): New function.
>
> * subversion/svn/update-cmd.c
> (svn_cl__update): Use the new wrapper baton and function to track
> externals processing failures, and return an error after all else
> is finished if such failures occured.
>
> * subversion/svn/export-cmd.c
> (svn_cl__export): Use the new wrapper baton and function to track
> externals processing failures, and return an error after all else
> is finished if such failures occured.
>
> * subversion/svn/switch-cmd.c
> (svn_cl__switch): Use the new wrapper baton and function to track
> externals processing failures, and return an error after all else
> is finished if such failures occured.
>
> * subversion/tests/cmdline/externals_tests.py
> (old_style_externals_ignore_peg_reg,
> can_place_file_external_into_dir_external): Change expected exit code.

Hi Mike. Useful fix.

I see that as well as reporting errors in externals, svn_wc_notify_t can
also report errors in locking:

  /** Points to an error describing the reason for the failure when @c
   * action is one of the following: #svn_wc_notify_failed_lock,
   * #svn_wc_notify_failed_unlock, #svn_wc_notify_failed_external.
   * Is @c NULL otherwise. */
  svn_error_t *err;

Without investigating further, it seems to me that we will want to do
something similar for locking errors too. With this in mind, it might
be worth generalizing the wrapper stuff to "had an error" rather than
"had an externals error". (It can check nb->err != SVN_NO_ERROR rather
than nb->status==...)

I'm wondering about the wrapper function approach - did you feel it's
important to separate this error detection from the notification
function? I wonder if it would be simpler overall to get svn's existing
notifier code to track the presence of errors for you, instead. The
basic approach would be: add the flag

  svn_boolean_t had_error;

to the private "struct notify_baton"; add a getter such as

  svn_boolean_t svn_cl__notifier_had_error(void *baton);

that gets this flag; and make notify() do

  if (n->err)
    nb->had_error = TRUE;

Then the various subcommands would only need to call this function to
read the "had_error" indication. What do you think?

> Modified: subversion/trunk/subversion/include/svn_error_codes.h
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_error_codes.h (original)
> +++ subversion/trunk/subversion/include/svn_error_codes.h Wed Nov 10 01:44:35 2010
> @@ -1391,6 +1391,10 @@ SVN_ERROR_START
> SVN_ERR_CL_CATEGORY_START + 10,
> "No external merge tool available")
>
> + SVN_ERRDEF(SVN_ERR_CL_ERROR_PROCESSING_EXTERNALS,
> + SVN_ERR_CL_CATEGORY_START + 11,
> + "Failed processing one or more externals definitions")
> +
> /* malfunctions such as assertion failures */
>
> SVN_ERRDEF(SVN_ERR_ASSERTION_FAIL,
>
> Modified: subversion/trunk/subversion/svn/cl.h
> ==============================================================================
> --- subversion/trunk/subversion/svn/cl.h (original)
> +++ subversion/trunk/subversion/svn/cl.h Wed Nov 10 01:44:35 2010
> @@ -569,6 +569,21 @@ svn_cl__notifier_mark_checkout(void *bat
> svn_error_t *
> svn_cl__notifier_mark_export(void *baton);
>
> +/* Baton for use with svn_cl__check_externals_failed_notify_wrapper(). */
> +struct svn_cl__check_externals_failed_notify_baton
> +{
> + void *wrapped_baton; /* The "real" notify_func2. */
> + svn_wc_notify_func2_t wrapped_func; /* The "real" notify_func2 baton. */

Those two are swapped relative to their comments.

> + svn_boolean_t had_externals_error; /* Did something fail in an external? */
> +};
> +
> +/* Notification function wrapper (implements `svn_wc_notify_func2_t').
> + Use with an svn_cl__check_externals_failed_notify_baton BATON. */
> +void
> +svn_cl__check_externals_failed_notify_wrapper(void *baton,
> + const svn_wc_notify_t *n,
> + apr_pool_t *pool);
> +
> /* Print conflict stats accumulated in NOTIFY_BATON.
> * Return any error encountered during printing.
> * Do all allocations in POOL.*/
>
> Modified: subversion/trunk/subversion/svn/export-cmd.c
> ==============================================================================
> --- subversion/trunk/subversion/svn/export-cmd.c (original)
> +++ subversion/trunk/subversion/svn/export-cmd.c Wed Nov 10 01:44:35 2010
> @@ -52,6 +52,7 @@ svn_cl__export(apr_getopt_t *os,
> svn_error_t *err;
> svn_opt_revision_t peg_revision;
> const char *truefrom;
> + struct svn_cl__check_externals_failed_notify_baton nwb;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -95,6 +96,12 @@ svn_cl__export(apr_getopt_t *os,
> if (opt_state->depth == svn_depth_unknown)
> opt_state->depth = svn_depth_infinity;
>
> + nwb.wrapped_func = ctx->notify_func2;
> + nwb.wrapped_baton = ctx->notify_baton2;
> + nwb.had_externals_error = FALSE;
> + ctx->notify_func2 = svn_cl__check_externals_failed_notify_wrapper;
> + ctx->notify_baton2 = &nwb;
> +
> /* Do the export. */
> err = svn_client_export5(NULL, truefrom, to, &peg_revision,
> &(opt_state->start_revision),
> @@ -106,5 +113,10 @@ svn_cl__export(apr_getopt_t *os,
> _("Destination directory exists; please remove "
> "the directory or use --force to overwrite"));
>
> + if (nwb.had_externals_error)
> + return svn_error_create(SVN_ERR_CL_ERROR_PROCESSING_EXTERNALS, NULL,
> + _("Failure occured processing one or more "
> + "externals definitions"));
> +
> return svn_error_return(err);
> }
>
> Modified: subversion/trunk/subversion/svn/notify.c
> ==============================================================================
> --- subversion/trunk/subversion/svn/notify.c (original)
> +++ subversion/trunk/subversion/svn/notify.c Wed Nov 10 01:44:35 2010
> @@ -952,3 +952,18 @@ svn_cl__notifier_mark_export(void *baton
> nb->is_export = TRUE;
> return SVN_NO_ERROR;
> }
> +
> +void
> +svn_cl__check_externals_failed_notify_wrapper(void *baton,
> + const svn_wc_notify_t *n,
> + apr_pool_t *pool)
> +{
> + struct svn_cl__check_externals_failed_notify_baton *nwb = baton;
> +
> + if (n->action == svn_wc_notify_failed_external)
> + nwb->had_externals_error = TRUE;
> +
> + if (nwb->wrapped_func)
> + nwb->wrapped_func(nwb->wrapped_baton, n, pool);
> +}

All the rest looks fine.

- Julian
Received on 2010-11-10 11:22:22 CET

This is an archived mail posted to the Subversion Dev mailing list.