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

On Wed, 24 Jan 2007, Hyrum K. Wright wrote:

> Daniel Rall wrote:
> > On Tue, 23 Jan 2007, Hyrum K. Wright wrote:
> ...
> >> My main concern is that we have a unbounded number of copy_pair_t
> >> structures which get allocated in setup_copy() from its main pool. They
> >> are created before we do existence checking, and would not be destroyed
> >> in the case of an error. We also have other resources, such as ra
> >> sessions, which don't get cleaned up properly. Those could impact the
> >> server, as well as the client.
> >>
> >> In the typical error case, we don't worry about it, because we assume
> >> the pool is going to get destroyed quickly anyway. That assumption
> >> isn't valid in this case.
> >
> > Sounds like some additional pool juggling wouldn't be out of line,
> > then. Either that, or restructure the code to handle the
> > copy_as_child parameter deeper in the call stack.
>
> Or just use a subpool for calls to setup_copy(). We could then
> duplicate *commit_info_t before returning from the client API, and
> safely destroy the subpool. This approach seems a lot less invasive to me.

Yeah, I like the sound of that. Something like this? We might also
be able to finagle use of the sub-pool in move5() for allocations of
some of the temporary data fed to setup_copy() (though how to do that
with the pool clearing wasn't immediately obivous to me).

Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 23212)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -1538,17 +1538,19 @@
                  apr_pool_t *pool)
 {
   svn_error_t *err;
+ svn_commit_info_t *commit_info;
+ apr_pool_t *subpool = svn_pool_create(pool);
 
   if (sources->nelts > 1 && !copy_as_child)
     return svn_error_create(SVN_ERR_CLIENT_MULTIPLE_SOURCES_DISALLOWED,
                             NULL, NULL);
 
- err = setup_copy(commit_info_p,
+ err = setup_copy(&commit_info,
                    sources, dst_path,
                    FALSE /* is_move */,
                    TRUE /* force, set to avoid deletion check */,
                    ctx,
- pool);
+ subpool);
 
   /* If the destination exists, try to copy the sources as children of the
      destination. */
@@ -1561,18 +1563,20 @@
       const char *src_basename;
 
       svn_error_clear(err);
+ svn_pool_clear(subpool);
 
       src_basename = svn_path_basename(src_path, pool);
-
- err = setup_copy(commit_info_p,
+ err = setup_copy(&commit_info,
                        sources,
                        svn_path_join(dst_path, src_basename, pool),
                        FALSE /* is_move */,
                        TRUE /* force, set to avoid deletion check */,
                        ctx,
- pool);
+ subpool);
     }
 
+ *commit_info_p = svn_commit_info_dup(commit_info, pool);
+ svn_pool_destroy(subpool);
   return err;
 }
 
@@ -1663,10 +1667,12 @@
                  svn_client_ctx_t *ctx,
                  apr_pool_t *pool)
 {
+ svn_commit_info_t *commit_info;
   const svn_opt_revision_t head_revision
     = { svn_opt_revision_head, { 0 } };
   svn_error_t *err;
   int i;
+ apr_pool_t *subpool = svn_pool_create(pool);
   apr_array_header_t *sources = apr_array_make(pool, src_paths->nelts,
                                   sizeof(const svn_client_copy_source_t *));
 
@@ -1687,11 +1693,11 @@
       APR_ARRAY_PUSH(sources, svn_client_copy_source_t *) = copy_source;
     }
 
- err = setup_copy(commit_info_p, sources, dst_path,
+ err = setup_copy(&commit_info, sources, dst_path,
                    TRUE /* is_move */,
                    force,
                    ctx,
- pool);
+ subpool);
 
   /* If the destination exists, try to move the sources as children of the
      destination. */
@@ -1703,17 +1709,19 @@
       const char *src_basename;
 
       svn_error_clear(err);
+ svn_pool_clear(subpool);
 
       src_basename = svn_path_basename(src_path, pool);
-
- err = setup_copy(commit_info_p, sources,
+ err = setup_copy(&commit_info, sources,
                        svn_path_join(dst_path, src_basename, pool),
                        TRUE /* is_move */,
                        force,
                        ctx,
- pool);
+ subpool);
     }
   
+ *commit_info_p = svn_commit_info_dup(commit_info, pool);
+ svn_pool_destroy(subpool);
   return err;
 }