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