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

On Tue, Jun 24, 2008 at 2:19 PM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> glasser_at_tigris.org writes:
>> Log:
>> Fixes to pool usage in libsvn_client copy code. Specifically, prevent
>> unbounded memory use when wc-to-wc copying or moving multiple files,
>> by passing an iterpool to propagate_mergeinfo_within_wc.
>>
>> Found by: Matt McHenry <mmchenry_at_carnegielearning.com>
>
> Aha! I knew it! :-)
>
> Comments below...
>
>> --- trunk/subversion/libsvn_client/copy.c (r31867)
>> +++ trunk/subversion/libsvn_client/copy.c (r31868)
>> @@ -371,7 +371,7 @@ do_wc_to_wc_copies(const apr_array_heade
>> if (src_access)
>> {
>> err = propagate_mergeinfo_within_wc(pair, src_access, dst_access,
>> - ctx, pool);
>> + ctx, iterpool);
>> if (err)
>> break;
>
> +1, but is there any reason not to do the same to the
>
> svn_path_split(pair->src, &src_parent, NULL, pool);
>
> ...call earlier in that same loop? Well, I just did it in r31870.

Sounds good.

>> @@ -463,7 +463,7 @@ do_wc_to_wc_moves(const apr_array_header
>> break;
>>
>> err = propagate_mergeinfo_within_wc(pair, src_access, dst_access,
>> - ctx, pool);
>> + ctx, iterpool);
>> if (err)
>> break;
>
> Heh, in this case the svn_path_split() above is already using iterpool.
>
> However, I'm still not sure why dst_access is treated differently
> between those two functions. I've left it alone in r31870, but it bears
> a second look.

Not sure. Hyrum?

>> @@ -1773,7 +1773,7 @@ setup_copy(svn_commit_info_t **commit_in
>> iterpool));
>> src_basename = svn_path_basename(pair->src, iterpool);
>> if (srcs_are_urls && ! dst_is_url)
>> - src_basename = svn_path_uri_decode(src_basename, pool);
>> + src_basename = svn_path_uri_decode(src_basename, iterpool);
>>
>> /* Check to see if all the sources are urls or all working copy
>> * paths. */
>
> *nod* But I really wonder if that function shouldn't use a subpool
> where it currently uses pool, too...

Eh, setup_copy's pool is the subpool from its caller (eg,
svn_client_copy4), and the caller doesn't allocate much itself. So
that's not really necessary.

--dave

>> @@ -2015,11 +2015,11 @@ svn_client_copy4(svn_commit_info_t **com
>>
>> src_basename = svn_path_basename(src_path, subpool);
>> if (svn_path_is_url(src_path) && ! svn_path_is_url(dst_path))
>> - src_basename = svn_path_uri_decode(src_basename, pool);
>> + src_basename = svn_path_uri_decode(src_basename, subpool);
>
> ...like this one does.
>
>> err = setup_copy(&commit_info,
>> sources,
>> - svn_path_join(dst_path, src_basename, pool),
>> + svn_path_join(dst_path, src_basename, subpool),
>> FALSE /* is_move */,
>> TRUE /* force, set to avoid deletion check */,
>> make_parents,
>> @@ -2033,7 +2033,7 @@ svn_client_copy4(svn_commit_info_t **com
>> if (commit_info)
>> *commit_info_p = svn_commit_info_dup(commit_info, pool);
>> else
>> - *commit_info_p = commit_info;
>> + *commit_info_p = NULL;
>> }
>
> Yes, much more readable.
>
> -K
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>
>

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org