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

On Tue, 23 Jan 2007, Hyrum K. Wright wrote:

> On Mon, 22 Jan 2007, Daniel Rall wrote:
> > On Sun, 21 Jan 2007, Vlad Georgescu wrote:
...
> > > hwright@tigris.org wrote:
> > > [...]
> > > > + err = setup_copy(commit_info_p,
> > > > + src_paths, src_revision,
> > > > + svn_path_join(dst_path, src_basename, > pool),
> > > > + FALSE /* is_move */,
> > > > + TRUE /* force, set to avoid deletion check */,
> > > > + ctx,
> > > > + subpool);
> > > > }
> > > >
> > > > + svn_pool_destroy(subpool);
> > > > +
> > >
> > > Looks like you're destroying the pool in which *commit_info_p was
> > > allocated. (I compile APR with --enable-pool-debug, which makes it much
> > > more likely that these kinds of problems will cause a segfault later
> on).
> > >
> > > [...]
> > > > + err = setup_copy(commit_info_p, src_paths, &src_revision,
> > > > + svn_path_join(dst_path, src_basename, > pool),
> > > > + TRUE /* is_move */,
> > > > + force,
> > > > + ctx,
> > > > + subpool);
> > > > }
> > > >
> > > > + svn_pool_destroy(subpool);
> > > > +
> > > > return err;
> > > > }
> > >
> > > Same here.
> >
> > Thanks Vlad, taken care of.
>
> Thanks for doing that, Dan.

You're welcome. I figure we could hash out a better solution after
this minimal fix was applied to trunk.

> It still bugs me that we use the pool passed to setup_copy() to
> allocate unbounded data structures.

Yeah, we use an iterpool allocated from the pool passed to
setup_copy() for disposable allocations with a loop, but the size of
the data returned in *commit_info_p is also unbounded. In the case
where we ignore the first contents of *commit_info_p -- when we're
okay with copying/moving the sources under the destination (as
children) -- is it typically already fully populated? Or do we detect
the "error" condition (the presence of the destination) before
populating it?

> Would it be possible/appropriate to use a subpool within
> setup_copy() itself? We could use if for most of the operations,
> just making sure that we don't allocate commit_info_p with the
> subpool.
>
> Or does in that way madness lie?

If we're actually populating *commit_info_p, then ignoring it and
re-populating it, IMO it would be okay to pass a scratch pool to
setup_copy() (as it's not part of the public API, only part of the
implementation).

I also didn't notice us setting *commit_info_p anywhere when there are
no commitables (WC -> WC and REPOS -> WC ops, where no commits occur).
Do we need something like this patch?

Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 23190)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -1503,6 +1503,7 @@
   /* Now, call the right handler for the operation. */
   if ((! srcs_are_urls) && (! dst_is_url))
     {
+ *commit_info_p = NULL;
       SVN_ERR(wc_to_wc_copy(copy_pairs, is_move,
                             ctx, pool));
     }
@@ -1513,6 +1514,7 @@
     }
   else if ((srcs_are_urls) && (! dst_is_url))
     {
+ *commit_info_p = NULL;
       SVN_ERR(repos_to_wc_copy(copy_pairs, ctx, pool));
     }
   else