Gavin, you're a volunteer, offering a service we very much appreciate but
which we realize is offered only out of your free and good will. We've
already established the minimum expectations of someone serving in the Patch
Manager role. So IMO, if you *want* to contribute to threads such as this
one (between committers) with a *poke* or additional information or
whatever, please do so.
(In this particular case, there's nothing to do. I fixed the code review
issues Julian mentioned, and shot down his concerns about the wrapper
function usage.)
On 11/24/2010 07:00 AM, Gavin Beau Baumanis wrote:
> 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
>>
>>
>>
>
--
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on 2010-11-24 14:58:11 CET