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

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