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, 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-24 13:01:02 CET