[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Tue, 24 Jun 2008 17:19:57 -0400

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.

> @@ -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.

> @@ -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...

> @@ -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
Received on 2008-06-24 23:20:41 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.