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

On 03/15/2011 12:34 AM, Daniel Shahaf wrote:
> 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

[...]

>> 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 <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand