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

> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
> Sent: dinsdag 15 maart 2011 5:34
> To: dev_at_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_at_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 = &notify_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
Received on 2011-03-15 14:54:47 CET