You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/07/05 13:26:56 UTC
Re: svn commit: r38337 - trunk/subversion/libsvn_client
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<hy...@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=2368120
Re: svn commit: r38337 - trunk/subversion/libsvn_client
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
I've revisited the wc context creation/destruction issue in r38345.
Hopefully it addresses these concerns (as well as the other stuff
discussed in IRC).
-Hyrum
On Jul 5, 2009, at 12:00 PM, Greg Stein wrote:
> 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<gs...@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<hy...@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
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2368405
Re: svn commit: r38337 - trunk/subversion/libsvn_client
Posted by Greg Stein <gs...@gmail.com>.
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<gs...@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<hy...@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