You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/03/15 05:34:11 UTC
Re: svn commit: r1079508 - in /subversion/trunk/subversion:
include/svn_client.h include/svn_wc.h libsvn_client/commit_util.c
libsvn_client/deprecated.c svn/notify.c
cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
> Author: cmpilato
> Date: Tue Mar 8 20:05:50 2011
> New Revision: 1079508
>
> URL: http://svn.apache.org/viewvc?rev=1079508&view=rev
> Log:
> For issue #3752 ("Warn about committing copied directory at depth
> 'empty'"), introduce new notification types for overwriting and
> non-overwriting copies. Current, the command-line client treats these
> just as it does their non-copy-ful equivalents.
>
> * subversion/include/svn_wc.h
> (svn_wc_notify_action_t): Add new 'svn_wc_notify_commit_copied' and
> 'svn_wc_notify_commit_copied_replaced' notification types.
>
> * subversion/include/svn_client.h
> (svn_client_commit5): Update docstring to indicate that this
> function will transmit the new notification types, too.
> (svn_client_commit4): Explicitly note that this interface does *not*
> use the new notification types.
>
> * subversion/libsvn_client/commit_util.c
> (do_item_commit): Check for a non-NULL copyfrom_url on the commit
> item and, if found, use the new notification types rather than
> mere "added" and "replaced" notifications.
>
> * subversion/libsvn_client/deprecated.c
> (struct downgrade_commit_copied_notify_baton,
> downgrade_commit_copied_notify_func): New notification wrapper
> function and baton.
> (svn_client_commit4): Use downgrade_commit_copied_notify_func()
> wrapper as necessary to avoid sending unrecognized notification
> types to the caller.
>
> * subversion/svn/notify.c
> (notify): Handle svn_wc_notify_commit_copied as
> svn_wc_notify_commit_added; svn_wc_notify_commit_copied_replaced
> as svn_wc_notify_commit_replaced.
>
> Modified:
> subversion/trunk/subversion/include/svn_client.h
> subversion/trunk/subversion/include/svn_wc.h
> subversion/trunk/subversion/libsvn_client/commit_util.c
> subversion/trunk/subversion/libsvn_client/deprecated.c
> subversion/trunk/subversion/svn/notify.c
>
> Modified: subversion/trunk/subversion/include/svn_client.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=1079508&r1=1079507&r2=1079508&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_client.h (original)
> +++ subversion/trunk/subversion/include/svn_client.h Tue Mar 8 20:05:50 2011
> @@ -1850,6 +1850,7 @@ svn_client_import(svn_client_commit_info
> * @a ctx->notify_baton2 as the commit progresses, with any of the following
> * actions: #svn_wc_notify_commit_modified, #svn_wc_notify_commit_added,
> * #svn_wc_notify_commit_deleted, #svn_wc_notify_commit_replaced,
> + * #svn_wc_notify_commit_copied, #svn_wc_notify_commit_copied_replaced,
> * #svn_wc_notify_commit_postfix_txdelta.
> *
> * If @a depth is #svn_depth_infinity, commit all changes to and
> @@ -1895,7 +1896,10 @@ svn_client_commit5(const apr_array_heade
>
> /**
> * Similar to svn_client_commit5(), but returns the @a commit_info_p directly,
> - * rather than through @a commit_callback.
> + * rather than through @a commit_callback. Does not make use of
> + * #svn_wc_notify_commit_copied or #svn_wc_notify_commit_copied_replaced
> + * (preferring #svn_wc_notify_commit_added and
> + * #svn_wc_notify_commit_replaced, respectively, instead).
> *
> * Also, if no error is returned and @a (*commit_info_p)->revision is set to
> * #SVN_INVALID_REVNUM, then the commit was a no-op; nothing needed to
>
> Modified: subversion/trunk/subversion/include/svn_wc.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_wc.h?rev=1079508&r1=1079507&r2=1079508&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_wc.h (original)
> +++ subversion/trunk/subversion/include/svn_wc.h Tue Mar 8 20:05:50 2011
> @@ -1144,6 +1144,16 @@ typedef enum svn_wc_notify_action_t
> * @since New in 1.7. */
> svn_wc_notify_patch_hunk_already_applied,
>
> + /** Committing a non-overwriting copy (path is the target of the
> + * copy, not the source).
> + * @since New in 1.7. */
> + svn_wc_notify_commit_copied,
> +
> + /** Committing an overwriting (replace) copy (path is the target of
> + * the copy, not the source).
> + * @since New in 1.7. */
> + svn_wc_notify_commit_copied_replaced,
> +
> /** The server has instructed the client to follow a URL
> * redirection.
> * @since New in 1.7. */
>
> Modified: subversion/trunk/subversion/libsvn_client/commit_util.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/commit_util.c?rev=1079508&r1=1079507&r2=1079508&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/commit_util.c (original)
> +++ subversion/trunk/subversion/libsvn_client/commit_util.c Tue Mar 8 20:05:50 2011
> @@ -1391,8 +1391,14 @@ do_item_commit(void **dir_baton,
> /* We don't print the "(bin)" notice for binary files when
> replacing, only when adding. So we don't bother to get
> the mime-type here. */
> - notify = svn_wc_create_notify(npath, svn_wc_notify_commit_replaced,
> - pool);
> + if (item->copyfrom_url)
> + notify = svn_wc_create_notify(npath,
> + svn_wc_notify_commit_copied_replaced,
> + pool);
> + else
> + notify = svn_wc_create_notify(npath, svn_wc_notify_commit_replaced,
> + pool);
> +
> }
> else if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
> {
> @@ -1401,8 +1407,13 @@ do_item_commit(void **dir_baton,
> }
> else if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD)
> {
> - notify = svn_wc_create_notify(npath, svn_wc_notify_commit_added,
> - pool);
> + if (item->copyfrom_url)
> + notify = svn_wc_create_notify(npath, svn_wc_notify_commit_copied,
> + pool);
> + else
> + notify = svn_wc_create_notify(npath, svn_wc_notify_commit_added,
> + pool);
> +
> if (item->kind == svn_node_file)
> {
> const svn_string_t *propval;
>
> Modified: subversion/trunk/subversion/libsvn_client/deprecated.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/deprecated.c?rev=1079508&r1=1079507&r2=1079508&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/deprecated.c (original)
> +++ subversion/trunk/subversion/libsvn_client/deprecated.c Tue Mar 8 20:05:50 2011
> @@ -425,6 +425,43 @@ svn_client_import(svn_client_commit_info
> return svn_error_return(err);
> }
>
> +
> +/* Wrapper notify_func2 function and baton for downgrading
> + svn_wc_notify_commit_copied and svn_wc_notify_commit_copied_replaced
> + to svn_wc_notify_commit_added and svn_wc_notify_commit_replaced,
> + respectively. */
> +struct downgrade_commit_copied_notify_baton
> +{
> + svn_wc_notify_func2_t orig_notify_func2;
> + void *orig_notify_baton2;
> +};
> +
> +static void
> +downgrade_commit_copied_notify_func(void *baton,
> + const svn_wc_notify_t *notify,
> + apr_pool_t *pool)
> +{
> + svn_wc_notify_t *my_notify = (svn_wc_notify_t *)notify;
> + struct downgrade_commit_copied_notify_baton *b = baton;
> +
> + if (notify->action == svn_wc_notify_commit_copied)
> + {
> + my_notify = svn_wc_dup_notify(notify, pool);
> + my_notify->action = svn_wc_notify_commit_added;
> + }
> + else if (notify->action == svn_wc_notify_commit_copied)
> + {
> + my_notify = svn_wc_dup_notify(notify, pool);
> + my_notify->action = svn_wc_notify_commit_added;
> + }
> +
> + /* Call the wrapped notification system with MY_NOTIFY, which is
> + either the original NOTIFY object, or a tweaked deep copy
> + thereof. */
> + b->orig_notify_func2(b->orig_notify_baton2, my_notify, pool);
> +}
> +
> +
> svn_error_t *
> svn_client_commit4(svn_commit_info_t **commit_info_p,
> const apr_array_header_t *targets,
> @@ -437,14 +474,33 @@ svn_client_commit4(svn_commit_info_t **c
> apr_pool_t *pool)
> {
> struct capture_baton_t cb;
> + struct downgrade_commit_copied_notify_baton notify_baton;
> + svn_error_t *err;
> +
> + notify_baton.orig_notify_func2 = ctx->notify_func2;
> + notify_baton.orig_notify_baton2 = ctx->notify_baton2;
>
> *commit_info_p = NULL;
> cb.info = commit_info_p;
> cb.pool = pool;
>
> - SVN_ERR(svn_client_commit5(targets, depth, keep_locks, keep_changelists,
> - changelists, revprop_table,
> - capture_commit_info, &cb, ctx, pool));
> + /* Swap out the notification system (if any) with a thin filtering
> + wrapper. */
> + if (ctx->notify_func2)
> + {
> + ctx->notify_func2 = downgrade_commit_copied_notify_func;
> + ctx->notify_baton2 = ¬ify_baton;
> + }
> +
> + err = svn_client_commit5(targets, depth, keep_locks, keep_changelists,
> + changelists, revprop_table,
> + capture_commit_info, &cb, ctx, pool);
> +
> + /* Ensure that the original notification system is in place. */
> + ctx->notify_func2 = notify_baton.orig_notify_func2;
> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
> +
This is actually a race condition, isn't it? (for clients that call the
deprecated API while using CTX->notify_func2 in another thread (in the
same or another API))
(Okay, so maybe we'll just let it live on. Presumably N other
deprecated wrappers do this too.)
> + SVN_ERR(err);
>
> if (! *commit_info_p)
> *commit_info_p = svn_create_commit_info(pool);
>
> Modified: subversion/trunk/subversion/svn/notify.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/notify.c?rev=1079508&r1=1079507&r2=1079508&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svn/notify.c (original)
> +++ subversion/trunk/subversion/svn/notify.c Tue Mar 8 20:05:50 2011
> @@ -668,6 +668,7 @@ notify(void *baton, const svn_wc_notify_
> break;
>
> case svn_wc_notify_commit_added:
> + case svn_wc_notify_commit_copied:
> if (n->mime_type && svn_mime_type_is_binary(n->mime_type))
> {
> if ((err = svn_cmdline_printf(pool,
> @@ -691,6 +692,7 @@ notify(void *baton, const svn_wc_notify_
> break;
>
> case svn_wc_notify_commit_replaced:
> + case svn_wc_notify_commit_copied_replaced:
> if ((err = svn_cmdline_printf(pool,
> _("Replacing %s\n"),
> path_local)))
>
>
RE: svn commit: r1079508 - in /subversion/trunk/subversion: include/svn_client.h include/svn_wc.h libsvn_client/commit_util.c libsvn_client/deprecated.c svn/notify.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: dinsdag 15 maart 2011 5:34
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1079508 - in /subversion/trunk/subversion:
> include/svn_client.h include/svn_wc.h libsvn_client/commit_util.c
> libsvn_client/deprecated.c svn/notify.c
>
> cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
> > Author: cmpilato
> > Date: Tue Mar 8 20:05:50 2011
> > New Revision: 1079508
> >
> > URL: http://svn.apache.org/viewvc?rev=1079508&view=rev
> > Log:
> > For issue #3752 ("Warn about committing copied directory at depth
> > 'empty'"), introduce new notification types for overwriting and
> > non-overwriting copies. Current, the command-line client treats these
> > just as it does their non-copy-ful equivalents.
> >
> > * subversion/include/svn_wc.h
> > (svn_wc_notify_action_t): Add new 'svn_wc_notify_commit_copied' and
> > 'svn_wc_notify_commit_copied_replaced' notification types.
> >
> > * subversion/include/svn_client.h
> > (svn_client_commit5): Update docstring to indicate that this
> > function will transmit the new notification types, too.
> > (svn_client_commit4): Explicitly note that this interface does *not*
> > use the new notification types.
> >
> > * subversion/libsvn_client/commit_util.c
> > (do_item_commit): Check for a non-NULL copyfrom_url on the commit
> > item and, if found, use the new notification types rather than
> > mere "added" and "replaced" notifications.
> >
> > * subversion/libsvn_client/deprecated.c
> > (struct downgrade_commit_copied_notify_baton,
> > downgrade_commit_copied_notify_func): New notification wrapper
> > function and baton.
> > (svn_client_commit4): Use downgrade_commit_copied_notify_func()
> > wrapper as necessary to avoid sending unrecognized notification
> > types to the caller.
> >
> > * subversion/svn/notify.c
> > (notify): Handle svn_wc_notify_commit_copied as
> > svn_wc_notify_commit_added;
> svn_wc_notify_commit_copied_replaced
> > as svn_wc_notify_commit_replaced.
> >
> > Modified:
> > subversion/trunk/subversion/include/svn_client.h
> > subversion/trunk/subversion/include/svn_wc.h
> > subversion/trunk/subversion/libsvn_client/commit_util.c
> > subversion/trunk/subversion/libsvn_client/deprecated.c
> > subversion/trunk/subversion/svn/notify.c
> > svn_error_t *
> > svn_client_commit4(svn_commit_info_t **commit_info_p,
> > const apr_array_header_t *targets,
> > @@ -437,14 +474,33 @@ svn_client_commit4(svn_commit_info_t **c
> > apr_pool_t *pool)
> > {
> > struct capture_baton_t cb;
> > + struct downgrade_commit_copied_notify_baton notify_baton;
> > + svn_error_t *err;
> > +
> > + notify_baton.orig_notify_func2 = ctx->notify_func2;
> > + notify_baton.orig_notify_baton2 = ctx->notify_baton2;
> >
> > *commit_info_p = NULL;
> > cb.info = commit_info_p;
> > cb.pool = pool;
> >
> > - SVN_ERR(svn_client_commit5(targets, depth, keep_locks,
> keep_changelists,
> > - changelists, revprop_table,
> > - capture_commit_info, &cb, ctx, pool));
> > + /* Swap out the notification system (if any) with a thin filtering
> > + wrapper. */
> > + if (ctx->notify_func2)
> > + {
> > + ctx->notify_func2 = downgrade_commit_copied_notify_func;
> > + ctx->notify_baton2 = ¬ify_baton;
> > + }
> > +
> > + err = svn_client_commit5(targets, depth, keep_locks,
keep_changelists,
> > + changelists, revprop_table,
> > + capture_commit_info, &cb, ctx, pool);
> > +
> > + /* Ensure that the original notification system is in place. */
> > + ctx->notify_func2 = notify_baton.orig_notify_func2;
> > + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
> > +
>
> This is actually a race condition, isn't it? (for clients that call the
> deprecated API while using CTX->notify_func2 in another thread (in the
> same or another API))
>
> (Okay, so maybe we'll just let it live on. Presumably N other
> deprecated wrappers do this too.)
Sharing svn_client_ctx_t between different threads at the same time is
already broken in 1.7.
The client context contains a svn_wc_context_t instance, which is not thread
safe.
(I never assumed that it was safe to share the context instance between
threads in the first place)
Bert
RE: Race condition in libsvn_client code?
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: dinsdag 15 maart 2011 14:52
> To: C. Michael Pilato
> Cc: Subversion Development
> Subject: Re: Race condition in libsvn_client code?
>
> "C. Michael Pilato" <cm...@collab.net> writes:
>
> > [Tweaking Subject: for (hopefully) additional visibility.]
> >
> > On 03/15/2011 09:15 AM, Daniel Shahaf wrote:
> >> C. Michael Pilato wrote on Tue, Mar 15, 2011 at 09:05:56 -0400:
> >>> On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
> >>>> cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
> >>>>> Author: cmpilato
> >>>>> Date: Tue Mar 8 20:05:50 2011
> >>>>> New Revision: 1079508
> >>>
> >>> [...]
> >>>
> >>>>> svn_client_commit4(svn_commit_info_t **commit_info_p,
> >>>>> const apr_array_header_t *targets,
> >>>
> >>> [...]
> >>>
> >>>>> + /* Ensure that the original notification system is in place. */
> >>>>> + ctx->notify_func2 = notify_baton.orig_notify_func2;
> >>>>> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
> >>>>> +
> >>>>
> >>>> This is actually a race condition, isn't it? (for clients that call
the
> >>>> deprecated API while using CTX->notify_func2 in another thread (in
> the
> >>>> same or another API))
> >>>>
> >>>> (Okay, so maybe we'll just let it live on. Presumably N other
> >>>> deprecated wrappers do this too.)
> >>>
> >>> I hadn't considered that. We do alot of pointer swaps like this up in
the
> >>> command-line client code itself, but that's a bit different than doing
so
> >>> down in the libsvn_client library as in this case. There is one prior
> >>> instance of us doing this kind of swap inside the libsvn_client
library: in
> >>> libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
> >>> precedent not to be our reason for allowing badness to persist in the
> codebase.
> >>>
> >>> Got suggestions?
> >>
> >> Would this work? ---
> >>
> >> svn_client_ctx_t ctx2 = *ctx;
> >> ctx2.notify_func2 = notify_baton.orig_notify_func2;
> >> ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
> >> svn_client_commit5(ctx=&ctx2);
>
> It has a different problem, if the client modifies the context in a
> callback those modifications will not persist.
I don't think any of our callbacks receives the current ctx instance.
But see my other mail: We store a svn_wc_context_t in the client context. So
svn_client_ctx_t can't be shared between functions running on different
threads anyway.
Bert
Re: Race condition in libsvn_client code?
Posted by Philip Martin <ph...@wandisco.com>.
"C. Michael Pilato" <cm...@collab.net> writes:
> [Tweaking Subject: for (hopefully) additional visibility.]
>
> On 03/15/2011 09:15 AM, Daniel Shahaf wrote:
>> C. Michael Pilato wrote on Tue, Mar 15, 2011 at 09:05:56 -0400:
>>> On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
>>>> cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
>>>>> Author: cmpilato
>>>>> Date: Tue Mar 8 20:05:50 2011
>>>>> New Revision: 1079508
>>>
>>> [...]
>>>
>>>>> svn_client_commit4(svn_commit_info_t **commit_info_p,
>>>>> const apr_array_header_t *targets,
>>>
>>> [...]
>>>
>>>>> + /* Ensure that the original notification system is in place. */
>>>>> + ctx->notify_func2 = notify_baton.orig_notify_func2;
>>>>> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
>>>>> +
>>>>
>>>> This is actually a race condition, isn't it? (for clients that call the
>>>> deprecated API while using CTX->notify_func2 in another thread (in the
>>>> same or another API))
>>>>
>>>> (Okay, so maybe we'll just let it live on. Presumably N other
>>>> deprecated wrappers do this too.)
>>>
>>> I hadn't considered that. We do alot of pointer swaps like this up in the
>>> command-line client code itself, but that's a bit different than doing so
>>> down in the libsvn_client library as in this case. There is one prior
>>> instance of us doing this kind of swap inside the libsvn_client library: in
>>> libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
>>> precedent not to be our reason for allowing badness to persist in the codebase.
>>>
>>> Got suggestions?
>>
>> Would this work? ---
>>
>> svn_client_ctx_t ctx2 = *ctx;
>> ctx2.notify_func2 = notify_baton.orig_notify_func2;
>> ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
>> svn_client_commit5(ctx=&ctx2);
It has a different problem, if the client modifies the context in a
callback those modifications will not persist.
--
Philip
Race condition in libsvn_client code? (Was: svn commit: r1079508
...)
Posted by "C. Michael Pilato" <cm...@collab.net>.
[Tweaking Subject: for (hopefully) additional visibility.]
On 03/15/2011 09:15 AM, Daniel Shahaf wrote:
> C. Michael Pilato wrote on Tue, Mar 15, 2011 at 09:05:56 -0400:
>> On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
>>> cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
>>>> Author: cmpilato
>>>> Date: Tue Mar 8 20:05:50 2011
>>>> New Revision: 1079508
>>
>> [...]
>>
>>>> svn_client_commit4(svn_commit_info_t **commit_info_p,
>>>> const apr_array_header_t *targets,
>>
>> [...]
>>
>>>> + /* Ensure that the original notification system is in place. */
>>>> + ctx->notify_func2 = notify_baton.orig_notify_func2;
>>>> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
>>>> +
>>>
>>> This is actually a race condition, isn't it? (for clients that call the
>>> deprecated API while using CTX->notify_func2 in another thread (in the
>>> same or another API))
>>>
>>> (Okay, so maybe we'll just let it live on. Presumably N other
>>> deprecated wrappers do this too.)
>>
>> I hadn't considered that. We do alot of pointer swaps like this up in the
>> command-line client code itself, but that's a bit different than doing so
>> down in the libsvn_client library as in this case. There is one prior
>> instance of us doing this kind of swap inside the libsvn_client library: in
>> libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
>> precedent not to be our reason for allowing badness to persist in the codebase.
>>
>> Got suggestions?
>
> Would this work? ---
>
> svn_client_ctx_t ctx2 = *ctx;
> ctx2.notify_func2 = notify_baton.orig_notify_func2;
> ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
> svn_client_commit5(ctx=&ctx2);
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r1079508 - in /subversion/trunk/subversion:
include/svn_client.h include/svn_wc.h libsvn_client/commit_util.c
libsvn_client/deprecated.c svn/notify.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Tue, Mar 15, 2011 at 09:05:56 -0400:
> On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
> > cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
> >> Author: cmpilato
> >> Date: Tue Mar 8 20:05:50 2011
> >> New Revision: 1079508
>
> [...]
>
> >> svn_client_commit4(svn_commit_info_t **commit_info_p,
> >> const apr_array_header_t *targets,
>
> [...]
>
> >> + /* Ensure that the original notification system is in place. */
> >> + ctx->notify_func2 = notify_baton.orig_notify_func2;
> >> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
> >> +
> >
> > This is actually a race condition, isn't it? (for clients that call the
> > deprecated API while using CTX->notify_func2 in another thread (in the
> > same or another API))
> >
> > (Okay, so maybe we'll just let it live on. Presumably N other
> > deprecated wrappers do this too.)
>
> I hadn't considered that. We do alot of pointer swaps like this up in the
> command-line client code itself, but that's a bit different than doing so
> down in the libsvn_client library as in this case. There is one prior
> instance of us doing this kind of swap inside the libsvn_client library: in
> libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
> precedent not to be our reason for allowing badness to persist in the codebase.
>
> Got suggestions?
Would this work? ---
svn_client_ctx_t ctx2 = *ctx;
ctx2.notify_func2 = notify_baton.orig_notify_func2;
ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
svn_client_commit5(ctx=&ctx2);
Re: svn commit: r1079508 - in /subversion/trunk/subversion: include/svn_client.h
include/svn_wc.h libsvn_client/commit_util.c libsvn_client/deprecated.c svn/notify.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
> cmpilato@apache.org wrote on Tue, Mar 08, 2011 at 20:05:51 -0000:
>> Author: cmpilato
>> Date: Tue Mar 8 20:05:50 2011
>> New Revision: 1079508
[...]
>> svn_client_commit4(svn_commit_info_t **commit_info_p,
>> const apr_array_header_t *targets,
[...]
>> + /* Ensure that the original notification system is in place. */
>> + ctx->notify_func2 = notify_baton.orig_notify_func2;
>> + ctx->notify_baton2 = notify_baton.orig_notify_baton2;
>> +
>
> This is actually a race condition, isn't it? (for clients that call the
> deprecated API while using CTX->notify_func2 in another thread (in the
> same or another API))
>
> (Okay, so maybe we'll just let it live on. Presumably N other
> deprecated wrappers do this too.)
I hadn't considered that. We do alot of pointer swaps like this up in the
command-line client code itself, but that's a bit different than doing so
down in the libsvn_client library as in this case. There is one prior
instance of us doing this kind of swap inside the libsvn_client library: in
libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
precedent not to be our reason for allowing badness to persist in the codebase.
Got suggestions?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand