Re: svn commit: r38337 - trunk/subversion/libsvn_client

After reviewing r38328, I'm really not liking this pattern. It seems
to be very error-prone. You're destroying the context in cases where
you probably shouldn't. Via empirical evidence, mistakes seem to be
pretty easy.

In general, why aren't we just creating a wc_ctx when the client
context is created? They are *cheap* to create (the state pool is the
most expensive). It only gets expensive when the context and its
contained DB are used for an operation and an SDB is opened.

Cheers,
-g

On Sun, Jul 5, 2009 at 15:26, Greg Stein<gstein_at_gmail.com> wrote:
> This seems like a strange pattern. Why don't we stash that wc_context
> into the client context? Seems better to lazy-create the wc_context,
> than to have pairs of create/destroy. ??
>
> On Sun, Jul 5, 2009 at 02:28, Hyrum K. Wright<hyrum_at_hyrumwright.org> wrote:
>> Author: hwright
>> Date: Sat Jul  4 17:28:19 2009
>> New Revision: 38337
>>
>> Log:
>> Attempt to fix the windows tests by ensuring we destroy the wc_context
>> prior to exiting svn_client_commit4().
>>
>> * subversion/libsvn_client/commit.c
>>  (svn_client_commit4): Destroy the wc_context.  Also, create the wc_context
>>    a bit later (after all the early outs).
>>
>> Modified:
>>   trunk/subversion/libsvn_client/commit.c
>>
>> Modified: trunk/subversion/libsvn_client/commit.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/commit.c?pathrev=38337&r1=38336&r2=38337
>> ==============================================================================
>> --- trunk/subversion/libsvn_client/commit.c     Sat Jul  4 17:26:21 2009        (r38336)
>> +++ trunk/subversion/libsvn_client/commit.c     Sat Jul  4 17:28:19 2009        (r38337)
>> @@ -1338,12 +1338,6 @@ svn_client_commit4(svn_commit_info_t **c
>>                                       depth == svn_depth_infinity,
>>                                       pool, pool));
>>
>> -  /* Get a wc context. */
>> -  if (!ctx->wc_ctx)
>> -    SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
>> -  else
>> -    wc_ctx = ctx->wc_ctx;
>> -
>>   /* No targets means nothing to commit, so just return. */
>>   if (! base_dir)
>>     goto cleanup;
>> @@ -1522,6 +1516,12 @@ svn_client_commit4(svn_commit_info_t **c
>>       svn_pool_destroy(subpool);
>>     }
>>
>> +  /* Get a wc context. */
>> +  if (!ctx->wc_ctx)
>> +    SVN_ERR(svn_wc_context_create(&wc_ctx, NULL /* config */, pool, pool));
>> +  else
>> +    wc_ctx = ctx->wc_ctx;
>> +
>>   SVN_ERR(svn_wc_adm_open3(&base_dir_access, NULL, base_dir,
>>                            TRUE,  /* Write lock */
>>                            lock_base_dir_recursive ? -1 : 0, /* lock levels */
>> @@ -1702,6 +1702,9 @@ svn_client_commit4(svn_commit_info_t **c
>>                                          pool);
>>     }
>>
>> +  if (!ctx->wc_ctx)
>> +    SVN_ERR(svn_wc_context_destroy(wc_ctx));
>> +
>>   /* Sleep to ensure timestamp integrity. */
>>   svn_io_sleep_for_timestamps(base_dir, pool);
>>
>> ------------------------------------------------------
>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=2368048
>>
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2368159
Received on 2009-07-05 19:00:28 CEST