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

On Wed, 20 Dec 2006, Hyrum K. Wright wrote:

> Kamesh Jayachandran wrote:
> > Daniel Rall wrote:
> >> On Wed, 20 Dec 2006, Kamesh Jayachandran wrote:
> >>
> >>
> >>> dlr@tigris.org wrote:
> >>>
> >>>> Modified:
> >>>> trunk/subversion/libsvn_client/copy.c
> >>>>
> >>>> Modified: trunk/subversion/libsvn_client/copy.c
> >>>> URL:
> >>>> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22765&r1=22764&r2=22765
> >>>>
> >>>> ==============================================================================
> >>>>
> >>>> --- trunk/subversion/libsvn_client/copy.c (original)
> >>>> +++ trunk/subversion/libsvn_client/copy.c Wed Dec 20 06:06:12 2006
> >>>>
> >>>> /* Check that all of our SRCs exist, and all the DSTs don't. */
> >>>> for (i = 0; i < copy_pairs->nelts; i++)
> >>>> @@ -221,7 +220,7 @@
> >>>> ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
> >>>> svn_node_kind_t dst_kind, dst_parent_kind;
> >>>>
> >>>> - svn_pool_clear(subpool);
> >>>> + svn_pool_clear(iterpool);
> >>>>
> >>>> /* Verify that SRC_PATH exists. */
> >>>> SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
> >>>>
> >>> Why not this svn_io_check_path from iterpool?
> >>>
> >>
> >> As I mentioned in a separate email, this operation allocates memory
> >> for our input parameters. I want to avoid altering their values, then
> >> destroying the memory holding their new values. While I don't believe
> >> that those values are used now, this is a safe practice which leaves
> >> the code without booby-traps (and thus easier to modify).
> >>
> >> - Dan
> >>
> > Given the call it can only modify '&pair->src_kind' as 'pair->src_kind'
> > is an enum it does not need any allocation, so no need to worry abt pool
> > destruction.
>
> I think Kamesh is right on this one. The use of the pool parameter
> isn't documented in svn_io.h, and looking at the internals, pool is only
> used for allocations inside svn_io_check_path, not for anything that
> gets returned. pair->src_kind isn't allocated, just set to an enum value.
>
> Using iterpool here is safe.

Ah, you're right, I wasn't paying close enough attention to the data
type.

Index: copy.c
===================================================================
--- copy.c (revision 22765)
+++ copy.c (working copy)
@@ -223,7 +223,7 @@
       svn_pool_clear(iterpool);
 
       /* Verify that SRC_PATH exists. */
- SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
+ SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, iterpool));
       if (pair->src_kind == svn_node_none)
         return svn_error_createf(SVN_ERR_NODE_UNKNOWN_KIND, NULL,
                                  _("Path '%s' does not exist"),
@@ -238,7 +238,7 @@
                                  _("Path '%s' already exists"),
                                  svn_path_local_style(pair->dst, pool));
 
- svn_path_split(pair->dst, &pair->dst_parent, &pair->base_name, pool);
+ svn_path_split(pair->dst, &pair->dst_parent, &pair->base_name, iterpool);
 
       /* Make sure the destination parent is a directory and produce a clear
          error message if it is not. */

Any others in the patch?