Re: svn commit: r22599 - in branches/multiple-moves/subversion: include libsvn_client svn

[Malcolm Rowe <ma...@farside.org.uk>]

Hi,

A few random comments:

On Thu, Dec 07, 2006 at 10:53:17AM -0800, hwright@tigris.org wrote:
> --- branches/multiple-moves/subversion/include/svn_client.h	(original)
> +++ branches/multiple-moves/subversion/include/svn_client.h	Thu Dec  7 10:53:16 2006
> @@ -1898,28 +1898,32 @@
>                      apr_pool_t *pool);
>  
>  
> -/** Copy @a src_path to @a dst_path.
> +/** Copy @a src_paths to @a dst_path.
>   *
> + * If multiple @a src_paths are given, @a dst_path must be a directory,
> + * and @a src_paths will be copied as children of @a dst_path.
> + *
> + * @a src_paths must be files or directories under version control, or
> + * URLs of a versioned item in the repository.  If @a src_paths has multiple
> + * items, they must @a src_revision is used to choose the revision from which
> + * to copy the @a src_path.

Something's missing from that sentence :-)

Incidentally, what happens if src_paths is an empty array?  Is it an
error or a no-op?

> + *
> + * The parent of @a dst_path must already exist.  If @a src_paths has only
> + * one item, and @a copy_as_child is TRUE, @a dst_path must be a non-existent
> + * WC path or URL.  If @a src_paths has only one item, @a copy_as_child is
> + * FALSE, and @a dst_path already exists, fail with @c SVN_ERR_ENTRY_EXISTS
> + * if @a dst_path is a working copy path and @c SVN_ERR_FS_ALREADY_EXISTS if
> + * @a dst_path is an URL.

It might be good to provide a simple brief description at the start of
this paragraph of what copy_as_child is meant to represent.

What's the significance of copy_as_child if src_paths has more than one
item?

Pre-existing comments use 'a URL' rather than 'an URL'.

> [...]
> +
> +/**
> + * Similar to svn_client_copy4(), with the difference that if @dst_path exists,
> + * fail with @c SVN_ERR_ENTRY_EXISTS if @a dst_path is a working copy path and
> + * @c SVN_ERR_FS_ALREADY_EXISTS.  Also, only take a single @a src_path and
> + * @a dst_path.
> + *

I don't believe that SVN_ERR_FS_ALREADY_EXISTS is a verb :-)


Some of the above comments also apply to the move() interface.

> Modified: branches/multiple-moves/subversion/libsvn_client/copy.c
> URL: http://svn.collab.net/viewvc/svn/branches/multiple-moves/subversion/libsvn_client/copy.c?pathrev=22599&r1=22598&r2=22599
> ==============================================================================
> --- branches/multiple-moves/subversion/libsvn_client/copy.c	(original)
> +++ branches/multiple-moves/subversion/libsvn_client/copy.c	Thu Dec  7 10:53:16 2006
> @@ -1363,6 +1363,45 @@
>  
>  
>  /* Public Interfaces */
> +svn_error_t *
> +svn_client_copy4(svn_commit_info_t **commit_info_p,
> +                 apr_array_header_t *src_paths,
> +                 const svn_opt_revision_t *src_revision,
> +                 const char *dst_path,
> +                 svn_boolean_t copy_as_child,
> +                 svn_client_ctx_t *ctx,
> +                 apr_pool_t *pool)
> +{
> +  svn_error_t *err;
> +
> +  err = setup_copy(commit_info_p,
> +                   src_paths, src_revision, dst_path,
> +                   FALSE /* is_move */,
> +                   TRUE /* force, set to avoid deletion check */,
> +                   ctx,
> +                   pool);
> +
> +  /* If the destination exists, try to copy the sources as children of the
> +     destination. */
> +  if (copy_as_child && err && (src_paths->nelts == 1)
> +        && (err->apr_err == SVN_ERR_ENTRY_EXISTS
> +            || err->apr_err == SVN_ERR_FS_ALREADY_EXISTS))

I think we normally indent run-on lines in a conditional flush-left.  (We
might also put the operator at the end of the line, but I can't remember
for sure).

> +svn_client_move5(svn_commit_info_t **commit_info_p,
> +                 apr_array_header_t *src_paths,
> +                 const char *dst_path,
> +                 svn_boolean_t force,
> +                 svn_boolean_t move_as_child,
> +                 svn_client_ctx_t *ctx,
> +                 apr_pool_t *pool)
> +{
> +  const svn_opt_revision_t src_revision
> +    = { svn_opt_revision_unspecified, { 0 } };

Is this really const?  You pass a pointer to it below.  Could it be
static as well? (and would you therefore not need the initialiser?)

> +  svn_error_t *err;
> +  
> +  err = setup_copy(commit_info_p, src_paths, &src_revision, dst_path,
> +                   TRUE /* is_move */,
> +                   force,
> +                   ctx,
> +                   pool);
> +

Regards,
Malcolm