Hi Everyone, I thought I would use this thread to solicit a few opinions about threads like this one and the Patch Manager's role with them.
In a previous thread Hyrum noted that because the discussion was between full-committers there wasn't necessarily a role for the PM to play within the scope of that topic / thread. I.e. there is a discussion happening about development work - perhaps there is even a patch submission (or commit) being discussed on list by developers who have commit privileges. So it would seem sensible that I shouldn't "bother" to keep a watch on this specific one either. But is it so clear-cut? Should I only advocate submissions by non-committers, or is it appropriate, if I note a discussion missing a conclusion to bring that back to the lists attention, too? I don't mind either way - though my job (while not particularly demanding in the first place), would be "easier" if I simply ignored threads where the "mantle" was taken up by full-committers. I'm happy to do the work - but at the same time I'm mindful of creating unnecessary noise on the list. Gavin "Beau" Baumanis On 10/11/2010, at 9:21 PM, Julian Foad wrote: > On Wed, 2010-11-10, [email protected] 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 > > >
