Re: [PATCH] Move multiple targets

On Mon, 07 Nov 2005, Julian Foad wrote:
...
> Thanks for the patch. I would like this enhancement.

Me too, thanks Hyrum.

--- subversion/clients/cmdline/move-cmd.c (revision 17231)
+++ subversion/clients/cmdline/move-cmd.c (working copy)
...
+ /* If we have two arguments, we know that the first one must be the
+ souce and the second must been the destination, so proceed with
+ the move on that assumption. Otherwise, make sure that the last
+ argument is a directory, and then move everything into it. */
+ if (targets->nelts == 2)
+ {
+ src_path = ((const char **) (targets->elts))[0];
+ dst_path = ((const char **) (targets->elts))[1];
+
+ err = svn_client_move3 (&commit_info, src_path, dst_path,
+ opt_state->force, ctx, pool);

dst_path is no longer used outside of this block, and thus can be
scoped to this block:

           const char *dst_path = ((const char **) (targets->elts))[1];
           src_path = ((const char **) (targets->elts))[0];

+ }
+ else

Perhaps some sort of comment would be useful here, something like:

/* more than 2 elements, indicating multiple source paths */

+ {
+ dir_path = ((const char **) (targets->elts))[targets->nelts - 1];
+
+ SVN_ERR (svn_io_check_path (dir_path, &kind, pool));
+
+ if (kind != svn_node_dir)
+ {
+ return svn_error_create
+ (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+ _("Last argument must be a directory when moving multiple files"));

This error string says "files", but couldn't the source paths be to
directories, too? How about an alternate term like "paths" (as you
use in the help below) or "targets" instead of "files"?

...
+ src_path = ((const char **) (targets->elts))[i];
+ err = svn_client_move3 (&commit_info, src_path, dir_path,
+ opt_state->force, ctx, subpool);

Here's what Julian mentioned needs to be an atomic operation.

...
Index: subversion/clients/cmdline/main.c
===================================================================
--- subversion/clients/cmdline/main.c (revision 17231)
+++ subversion/clients/cmdline/main.c (working copy)
@@ -496,13 +496,16 @@
 
   { "move", svn_cl__move, {"mv", "rename", "ren"},
     N_("Move and/or rename something in working copy or repository.\n"
- "usage: move SRC DST\n"
+ "usage: 1. move SRC DST\n"
+ " 2. move SRC... DIR\n"

It's not obvious to me that style #2 can take multiple source
arguments. How about something like:

                2. move SRC [SRC2 SRC3 ...] DIR\n"

        "\n"
        " Note: this subcommand is equivalent to a 'copy' and 'delete'.\n"
        "\n"
- " SRC and DST can both be working copy (WC) paths or URLs:\n"
+ " 1. Renames SRC to DST. SRC and DST can both be working copy (WC) \n"
+ " paths or URLs:\n"
        " WC -> WC: move and schedule for addition (with history)\n"
- " URL -> URL: complete server-side rename.\n"),
+ " URL -> URL: complete server-side rename.\n"
+ " 2. Moves the path(s) specified by SRC into DIR.\n"),

It should be made clear that SRC and DIR can still be either a WC path
or URL (I assume URLs are still a supported mode for a multi-SRC move
or copy).

There's also some trailing whitespace on this line which needn't be here.

-- 
Daniel Rall
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org