Hi Kamesh, thanks for your review, below, I have some additional remarks.
On 12/20/06, Kamesh Jayachandran <kamesh@collab.net> wrote:
> hwright@tigris.org wrote:
> > Modified: trunk/subversion/libsvn_client/copy.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22762&r1=22761&r2=22762
> > ==============================================================================
> > --- trunk/subversion/libsvn_client/copy.c (original)
> > +++ trunk/subversion/libsvn_client/copy.c Tue Dec 19 20:38:50 2006
> > @@ -170,17 +205,86 @@
> > }
> >
> >
> > -struct path_driver_cb_baton
> > +static svn_error_t *
> > +wc_to_wc_copy(const apr_array_header_t *copy_pairs,
> > + svn_boolean_t is_move,
> > + svn_client_ctx_t *ctx,
> > + apr_pool_t *pool)
> > {
> > - const svn_delta_editor_t *editor;
> > - void *edit_baton;
> > - svn_node_kind_t src_kind;
> > + int i;
> > + apr_pool_t *subpool = svn_pool_create(pool);
> >
> Do you intend to use this subpool in both the for loops. But it does not
> seem so.
Also, there are 2 naming conventions: 'subpool' if the pool is used to
clear excessive resource useage in the function, for example,
allocation of (large) file buffers, open files or xml parsers. When
the pool is destroyed at the end of the function, those resources will
be freed.
The other is 'iterpool' which is used in iterations to allocate
resources on a per-iteration basis. These are cleared between
iterations. Sometimes there is an iterpool1 and iterpool2, which get
cleared alternatingly when resources are needed in the next iteration.
> If you don't want to use subpool in the first loop, why unneccarily
> clear it in the first loop.
But he does :-)
> > +
> > + /* Check that all of our SRCs exist, and all the DSTs don't. */
> > + for (i = 0; i < copy_pairs->nelts; i++)
> > + {
> > + svn_client__copy_pair_t *pair =
> > + ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
> > + svn_node_kind_t dst_kind, dst_parent_kind;
> > +
> > + svn_pool_clear(subpool);
> > +
> > + /* Verify that SRC_PATH exists. */
> > + SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
> > + if (pair->src_kind == svn_node_none)
> > + return svn_error_createf(SVN_ERR_NODE_UNKNOWN_KIND, NULL,
> > + _("Path '%s' does not exist"),
> > + svn_path_local_style(pair->src, pool));
> > +
> > + /* If DST_PATH does not exist, then its basename will become a new
> > + file or dir added to its parent (possibly an implicit '.').
> > + Else, just error out. */
> > + SVN_ERR(svn_io_check_path(pair->dst, &dst_kind, pool));
> > + if (dst_kind != svn_node_none)
> > + return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
> > + _("Path '%s' already exists"),
> > + svn_path_local_style(pair->dst, pool));
> > +
> > + svn_path_split(pair->dst, &pair->dst_parent, &pair->base_name, pool);
> > +
> > + /* Make sure the destination parent is a directory and produce a clear
> > + error message if it is not. */
> > + SVN_ERR(svn_io_check_path(pair->dst_parent, &dst_parent_kind, pool));
> > + if (dst_parent_kind != svn_node_dir)
> > + return svn_error_createf(SVN_ERR_WC_NOT_DIRECTORY, NULL,
> > + _("Path '%s' is not a directory"),
> > + svn_path_local_style(pair->dst_parent, pool));
> > + }
> > +
> > + for ( i = 0; i < copy_pairs->nelts; i++)
> > + {
> > + svn_client__copy_pair_t *pair =
> > + ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
> > + svn_pool_clear(subpool);
> > +
> > + /* Check for cancellation */
> > + if (ctx->cancel_func)
> > + SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
> > +
> > + SVN_ERR(wc_to_wc_copy_single(pair, is_move, ctx, subpool));
> > + }
> > +
> > + svn_pool_destroy(subpool);
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> >
> >
> > Modified: trunk/subversion/tests/cmdline/copy_tests.py
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/cmdline/copy_tests.py?pathrev=22762&r1=22761&r2=22762
> > ==============================================================================
> > --- trunk/subversion/tests/cmdline/copy_tests.py (original)
> > +++ trunk/subversion/tests/cmdline/copy_tests.py Tue Dec 19 20:38:50 2006
> > @@ -2813,6 +2813,347 @@
> > svntest.actions.run_and_verify_status(wc_dir, expected_status)
> >
> >
> > +#----------------------------------------------------------------------
> > +
> > +# Test moving multiple files within a wc.
> > +
> > +def move_multiple_wc(sbox):
> > + "svn mv multiple files to a common directory"
> > +
> >
> ....
> > + svntest.actions.run_and_verify_commit (wc_dir,
> >
>
> There should be no space between function name 'argument paren'
>
> > +# Test copying multiple files within a wc.
> > +
> > +def copy_multiple_wc(sbox):
> ........
>
>
> > + svntest.actions.run_and_verify_commit (wc_dir,
> >
>
> There should be no space between function name 'argument paren'.
>
> With regards
> Kamesh Jayachandran
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Dec 20 12:51:51 2006