Re: [PATCH] Allow replace on repo-to-repo copy

Justin,
Thanks for the patch! I've included some comments inline below.

Justin Patrin wrote:
> In order to support more easily scriptable tagging (among other
> things), I've created a patch to allow replacement of the destination
> in a repo to repo copy.
>
> Assume that the repo is set up at http://svn.example.com/svn and there
> are the normal trunk and tags directories.
>
> An initial:
> svn copy http://svn.example.com/svn/trunk
> http://svn.example.com/svn/tags/tag1
> Will do a copy to tag1. A second call:
> svn copy http://svn.example.com/svn/trunk
> http://svn.example.com/svn/tags/tag1
> will essentially do this:
> svn copy http://svn.example.com/svn/trunk
> http://svn.example.com/svn/tags/tag1/trunk
> and make a copy of /trunk at /tags/tag1/trunk. This is generally not
> what a person means when specifying an already existing tag.

Ah, but tags are just copies, almost always of directories. When given
a source and a destination, if the destination is a directory, the
semantics of the copy command are to copy the source into the
destination. This is case with every cli tool which I know of, as well
as the svn commandline client. (Other clients may not want that
functionality at the API level, which is why we have the copy_as_child
flag.)

> You can
> do a delete of tag1 and then do another repo to repo copy in order to
> replace the tag, but this makes 2 revisions (causing 2 entries in the
> log and 2 e-mails if you use commit e-mail hooks). You can get around
> this by doing a checkout of the entire repo, deleting the tag in the
> checkout, then copying from trunk to tag1 and checking in. This will
> replace the tag in a single transaction.
>
> My patch allows you to add --replace to a copy command to override the
> default behavior and do a replacement in a single transation.
> svn copy --replace http://svn.example.com/svn/trunk
> http://svn.example.com/svn/tags/tag1

I will agree that the svn commandline client lacks repository replace
functionality, but I don't think that the copy command is the correct
place to add it. At this point, I'm not sure where is ('svn replace'?)

Have you looked at using mucc? It can be found in the source tree at
contrib/client-side/mucc.c. mucc is a client which can combine multiple
repository actions into a single commit. I haven't tested it for your
use case, but you may want to.

> I have included 2 patches, one for trunk and one for 1.4.2. My testing
> shows that by default svn works the same as before and with --replace
> it deletes and copies as expected.

In general, you only need to include a patch against trunk. If it is a
bug fix, it will get ported from trunk to the appropriate release
branch. As this is both a feature and an API change, any such addition
would get released with the next major release (currently 1.5.0).

> I have not checked closely for possible memory leakage or other logic
> errors but I think this patch is nearly ready for comittal, assuming
> other clients are fixed accordingly. Adding an additional FALSE to the
> end of calls to svn_client_copy4() in trunk and svn_client_copy3() in
> 1.4.2 should be enough to cause any other clients to compile and work
> as normal.

Because of my concerns about adding this to the copy command, have
haven't reviewed the substance of your patch. One other suggestion,
though, is that your patches stand a much better chance of being
committed if you include a log message of the format described in
HACKING[1]. Thanks again, and I'm interested to see how mucc works for you.

[1] http://subversion.tigris.org/hacking.html#log-messages

-Hyrum

> ------------------------------------------------------------------------
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 23227)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2101,7 +2101,8 @@
> const char *dst_path,
> svn_boolean_t copy_as_child,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool);
> + apr_pool_t *pool,
> + svn_boolean_t replace);
>
> /**
> * Similar to svn_client_copy4(), with only one @a src_path and
> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 23227)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -426,7 +426,8 @@
> const apr_array_header_t *copy_pairs,
> svn_client_ctx_t *ctx,
> svn_boolean_t is_move,
> - apr_pool_t *pool)
> + apr_pool_t *pool,
> + svn_boolean_t replace)
> {
> apr_array_header_t *paths = apr_array_make(pool, 2 * copy_pairs->nelts,
> sizeof(const char *));
> @@ -548,6 +549,9 @@
> /* Fetch the youngest revision. */
> SVN_ERR(svn_ra_get_latest_revnum(ra_session, &youngest, pool));
>
> + svn_client_commit_item3_t *item;
> + apr_array_header_t *commit_items
> + = apr_array_make(pool, 2 * copy_pairs->nelts, sizeof(item));
> for (i = 0; i < copy_pairs->nelts; i++)
> {
> svn_client__copy_pair_t *pair = APR_ARRAY_IDX(copy_pairs, i,
> @@ -612,9 +616,18 @@
> pool));
> if (dst_kind != svn_node_none)
> {
> - /* We disallow the overwriting of existing paths. */
> - return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> - _("Path '%s' already exists"), dst_rel);
> + if (replace) {
> + item = apr_pcalloc(pool, sizeof(*item));
> + item->url = svn_path_join(top_url, dst_rel, pool);
> + item->state_flags = SVN_CLIENT_COMMIT_ITEM_DELETE;
> + APR_ARRAY_PUSH(commit_items, svn_client_commit_item3_t *) = item;
> + apr_hash_set(action_hash, dst_rel, APR_HASH_KEY_STRING,
> + info);
> + } else {
> + /* We disallow the overwriting of existing paths. */
> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> + _("Path '%s' already exists"), dst_rel);
> + }
> }
>
> info->src_url = pair->src;
> @@ -625,10 +638,7 @@
> /* Create a new commit item and add it to the array. */
> if (SVN_CLIENT__HAS_LOG_MSG_FUNC(ctx))
> {
> - svn_client_commit_item3_t *item;
> const char *tmp_file;
> - apr_array_header_t *commit_items
> - = apr_array_make(pool, 2 * copy_pairs->nelts, sizeof(item));
>
> for (i = 0; i < path_infos->nelts; i++)
> {
> @@ -1294,7 +1304,8 @@
> svn_boolean_t is_move,
> svn_boolean_t force,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> + apr_pool_t *pool,
> + svn_boolean_t replace)
> {
> apr_array_header_t *copy_pairs = apr_array_make(pool, sources->nelts,
> sizeof(struct copy_pair *));
> @@ -1520,7 +1531,7 @@
> else
> {
> SVN_ERR(repos_to_repos_copy(commit_info_p, copy_pairs,
> - ctx, is_move, pool));
> + ctx, is_move, pool, replace));
> }
>
> return SVN_NO_ERROR;
> @@ -1535,7 +1546,8 @@
> const char *dst_path,
> svn_boolean_t copy_as_child,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> + apr_pool_t *pool,
> + svn_boolean_t replace)
> {
> svn_error_t *err;
> svn_commit_info_t *commit_info = NULL;
> @@ -1550,7 +1562,8 @@
> FALSE /* is_move */,
> TRUE /* force, set to avoid deletion check */,
> ctx,
> - subpool);
> + subpool,
> + replace);
>
> /* If the destination exists, try to copy the sources as children of the
> destination. */
> @@ -1573,7 +1586,8 @@
> FALSE /* is_move */,
> TRUE /* force, set to avoid deletion check */,
> ctx,
> - subpool);
> + subpool,
> + replace);
> }
>
> if (commit_info)
> @@ -1609,7 +1623,8 @@
> dst_path,
> FALSE,
> ctx,
> - pool);
> + pool,
> + FALSE);
> }
>
>
> @@ -1702,7 +1717,8 @@
> TRUE /* is_move */,
> force,
> ctx,
> - subpool);
> + subpool,
> + FALSE);
>
> /* If the destination exists, try to move the sources as children of the
> destination. */
> @@ -1723,7 +1739,8 @@
> TRUE /* is_move */,
> force,
> ctx,
> - subpool);
> + subpool,
> + FALSE);
> }
>
> if (commit_info)
> @@ -1840,7 +1857,8 @@
> TRUE /* is_move */,
> force,
> ctx,
> - pool);
> + pool,
> + FALSE);
> /* These structs have the same layout for the common fields. */
> *commit_info_p = (svn_client_commit_info_t *) commit_info;
> return err;
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 23227)
> +++ subversion/svn/cl.h (working copy)
> @@ -81,7 +81,8 @@
> svn_cl__targets_opt,
> svn_cl__version_opt,
> svn_cl__xml_opt,
> - svn_cl__keep_local_opt
> + svn_cl__keep_local_opt,
> + svn_cl__replace_opt
> } svn_cl__longopt_t;
>
>
> @@ -151,7 +152,7 @@
> const char *changelist; /* operate on this changelist */
> svn_boolean_t keep_changelist; /* don't remove changelist after commit */
> svn_boolean_t keep_local; /* delete path only from repository */
> -
> + svn_boolean_t replace; /* replace destination on repo-to-repo copy */
> } svn_cl__opt_state_t;
>
>
> Index: subversion/svn/copy-cmd.c
> ===================================================================
> --- subversion/svn/copy-cmd.c (revision 23227)
> +++ subversion/svn/copy-cmd.c (working copy)
> @@ -131,7 +131,7 @@
> NULL, ctx->config, pool));
>
> err = svn_client_copy4(&commit_info, sources,
> - dst_path, TRUE, ctx, pool);
> + dst_path, TRUE, ctx, pool, opt_state->replace);
>
> if (ctx->log_msg_func3)
> SVN_ERR(svn_cl__cleanup_log_msg(ctx->log_msg_baton3, err));
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 23227)
> +++ subversion/svn/main.c (working copy)
> @@ -192,6 +192,8 @@
> N_("don't delete changelist after commit")},
> {"keep-local", svn_cl__keep_local_opt, 0,
> N_("keep path in working copy")},
> + {"replace", svn_cl__replace_opt, 0,
> + N_("replace destination if it exists on a repo-to-repo copy")},
> {0, 0, 0, 0}
> };
>
> @@ -330,7 +332,7 @@
> " URL -> URL: complete server-side copy; used to branch & tag\n"
> " All the SRCs must be of the same type.\n"),
> {'r', 'q',
> - SVN_CL__LOG_MSG_OPTIONS, SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt} },
> + SVN_CL__LOG_MSG_OPTIONS, SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt, svn_cl__replace_opt} },
>
> { "delete", svn_cl__delete, {"del", "remove", "rm"}, N_
> ("Remove files and directories from version control.\n"
> @@ -1271,6 +1273,9 @@
> case svn_cl__keep_local_opt:
> opt_state.keep_local = TRUE;
> break;
> + case svn_cl__replace_opt:
> + opt_state.replace = TRUE;
> + break;
> default:
> /* Hmmm. Perhaps this would be a good place to squirrel away
> opts that commands like svn diff might need. Hmmm indeed. */
>
>
> ------------------------------------------------------------------------
>
> Index: subversion-1.4.2/subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion-1.4.2.orig/subversion/libsvn_client/copy.c
> +++ subversion-1.4.2/subversion/libsvn_client/copy.c
> @@ -271,7 +271,8 @@ repos_to_repos_copy(svn_commit_info_t **
> const char *dst_url,
> svn_client_ctx_t *ctx,
> svn_boolean_t is_move,
> - apr_pool_t *pool)
> + apr_pool_t *pool,
> + svn_boolean_t replace)
> {
> apr_array_header_t *paths = apr_array_make(pool, 2, sizeof(const char *));
> const char *top_url, *src_rel, *dst_rel, *message, *repos_root;
> @@ -392,20 +393,27 @@ repos_to_repos_copy(svn_commit_info_t **
>
> /* Figure out the basename that will result from this operation. */
> SVN_ERR(svn_ra_check_path(ra_session, dst_rel, youngest, &dst_kind, pool));
> + svn_client_commit_item2_t *item;
> + apr_array_header_t *commit_items
> + = apr_array_make(pool, 2, sizeof(item));
> if (dst_kind != svn_node_none)
> {
> - /* We disallow the overwriting of existing paths. */
> - return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> - _("Path '%s' already exists"), dst_rel);
> + if (replace) {
> + item = apr_pcalloc(pool, sizeof(*item));
> + item->url = svn_path_join(top_url, dst_rel, pool);
> + item->state_flags = SVN_CLIENT_COMMIT_ITEM_DELETE;
> + APR_ARRAY_PUSH(commit_items, svn_client_commit_item2_t *) = item;
> + } else {
> + /* We disallow the overwriting of existing paths. */
> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> + _("Path '%s' already exists"), dst_rel);
> + }
> }
>
> /* Create a new commit item and add it to the array. */
> if (ctx->log_msg_func || ctx->log_msg_func2)
> {
> - svn_client_commit_item2_t *item;
> const char *tmp_file;
> - apr_array_header_t *commit_items
> - = apr_array_make(pool, 2, sizeof(item));
>
> item = apr_pcalloc(pool, sizeof(*item));
> item->url = svn_path_join(top_url, dst_rel, pool);
> @@ -942,7 +950,8 @@ setup_copy(svn_commit_info_t **commit_in
> svn_boolean_t is_move,
> svn_boolean_t force,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> + apr_pool_t *pool,
> + svn_boolean_t replace)
> {
> svn_boolean_t src_is_url, dst_is_url;
>
> @@ -1036,7 +1045,7 @@ setup_copy(svn_commit_info_t **commit_in
> else
> {
> SVN_ERR(repos_to_repos_copy(commit_info_p, src_path, src_revision,
> - dst_path, ctx, is_move, pool));
> + dst_path, ctx, is_move, pool, replace));
> }
>
> return SVN_NO_ERROR;
> @@ -1052,14 +1061,16 @@ svn_client_copy3(svn_commit_info_t **com
> const svn_opt_revision_t *src_revision,
> const char *dst_path,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> + apr_pool_t *pool,
> + svn_boolean_t replace)
> {
> return setup_copy(commit_info_p,
> src_path, src_revision, dst_path,
> FALSE /* is_move */,
> TRUE /* force, set to avoid deletion check */,
> ctx,
> - pool);
> + pool,
> + replace);
> }
>
>
> @@ -1074,7 +1085,7 @@ svn_client_copy2(svn_commit_info_t **com
> svn_error_t *err;
>
> err = svn_client_copy3(commit_info_p, src_path, src_revision,
> - dst_path, ctx, pool);
> + dst_path, ctx, pool, FALSE);
>
> /* If the target exists, try to copy the source as a child of the target.
> This will obviously fail if target is not a directory, but that's exactly
> @@ -1088,7 +1099,7 @@ svn_client_copy2(svn_commit_info_t **com
>
> return svn_client_copy3(commit_info_p, src_path, src_revision,
> svn_path_join(dst_path, src_basename, pool),
> - ctx, pool);
> + ctx, pool, FALSE);
> }
>
> return err;
> @@ -1128,7 +1139,8 @@ svn_client_move4(svn_commit_info_t **com
> TRUE /* is_move */,
> force,
> ctx,
> - pool);
> + pool,
> + FALSE);
> }
>
> svn_error_t *
> @@ -1210,7 +1222,8 @@ svn_client_move(svn_client_commit_info_t
> TRUE /* is_move */,
> force,
> ctx,
> - pool);
> + pool,
> + FALSE);
> /* These structs have the same layout for the common fields. */
> *commit_info_p = (svn_client_commit_info_t *) commit_info;
> return err;
> Index: subversion-1.4.2/subversion/svn/cl.h
> ===================================================================
> --- subversion-1.4.2.orig/subversion/svn/cl.h
> +++ subversion-1.4.2/subversion/svn/cl.h
> @@ -77,7 +77,8 @@ typedef enum {
> svn_cl__summarize,
> svn_cl__targets_opt,
> svn_cl__version_opt,
> - svn_cl__xml_opt
> + svn_cl__xml_opt,
> + svn_cl__replace_opt
> } svn_cl__longopt_t;
>
>
> @@ -143,6 +144,7 @@ typedef struct svn_cl__opt_state_t
> svn_boolean_t no_autoprops; /* disable automatic properties */
> const char *native_eol; /* override system standard eol marker */
> svn_boolean_t summarize; /* create a summary of a diff */
> + svn_boolean_t replace; /* replace destination on repo-to-repo copy */
> } svn_cl__opt_state_t;
>
>
> Index: subversion-1.4.2/subversion/svn/main.c
> ===================================================================
> --- subversion-1.4.2.orig/subversion/svn/main.c
> +++ subversion-1.4.2/subversion/svn/main.c
> @@ -184,6 +184,8 @@ const apr_getopt_option_t svn_cl__option
> N_("don't unlock the targets")},
> {"summarize", svn_cl__summarize, 0,
> N_("show a summary of the results")},
> + {"replace", svn_cl__replace_opt, 0,
> + N_("replace destination if it exists on a repo-to-repo copy")},
> {0, 0, 0, 0}
> };
>
> @@ -299,7 +301,7 @@ const svn_opt_subcommand_desc2_t svn_cl_
> " URL -> WC: check out URL into WC, schedule for addition\n"
> " URL -> URL: complete server-side copy; used to branch & tag\n"),
> {'r', 'q',
> - SVN_CL__LOG_MSG_OPTIONS, SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt} },
> + SVN_CL__LOG_MSG_OPTIONS, SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt, svn_cl__replace_opt} },
>
> { "delete", svn_cl__delete, {"del", "remove", "rm"}, N_
> ("Remove files and directories from version control.\n"
> @@ -1196,6 +1198,9 @@ main(int argc, const char *argv[])
> case svn_cl__summarize:
> opt_state.summarize = TRUE;
> break;
> + case svn_cl__replace_opt:
> + opt_state.replace = TRUE;
> + break;
> default:
> /* Hmmm. Perhaps this would be a good place to squirrel away
> opts that commands like svn diff might need. Hmmm indeed. */
> Index: subversion-1.4.2/subversion/include/svn_client.h
> ===================================================================
> --- subversion-1.4.2.orig/subversion/include/svn_client.h
> +++ subversion-1.4.2/subversion/include/svn_client.h
> @@ -1816,7 +1816,8 @@ svn_client_copy3(svn_commit_info_t **com
> const svn_opt_revision_t *src_revision,
> const char *dst_path,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool);
> + apr_pool_t *pool,
> + svn_boolean_t replace);
>
>
> /** Similar to svn_client_copy3(), with the difference that if @a dst_path
> Index: subversion-1.4.2/subversion/svn/copy-cmd.c
> ===================================================================
> --- subversion-1.4.2.orig/subversion/svn/copy-cmd.c
> +++ subversion-1.4.2/subversion/svn/copy-cmd.c
> @@ -114,7 +114,7 @@ svn_cl__copy(apr_getopt_t *os,
>
> err = svn_client_copy3(&commit_info, src_path,
> &(opt_state->start_revision),
> - dst_path, ctx, pool);
> + dst_path, ctx, pool, opt_state->replace);
>
> /* If dst_path already exists, try to copy src_path as a child of it. */
> if (err && (err->apr_err == SVN_ERR_ENTRY_EXISTS
> @@ -127,7 +127,7 @@ svn_cl__copy(apr_getopt_t *os,
> err = svn_client_copy3(&commit_info, src_path,
> &(opt_state->start_revision),
> svn_path_join(dst_path, src_basename, pool),
> - ctx, pool);
> + ctx, pool, FALSE);
> }
>
> if (ctx->log_msg_func2)
>
>
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org