On 1/25/07, Hyrum K. Wright <hyrum_wright@mail.utexas.edu> wrote:
> 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.
I agree and my patch does not change the default behavior of the clkient.
> (Other clients may not want that
> functionality at the API level, which is why we have the copy_as_child
> flag.)
This is true, but as I explain, my patch does nto change the default
behavior. You have to pass a new parameter in order for it to replace
instead of copy (or fail). copy_as_child is really the inverse of
replace and what they do it different. There may be some room for
combination of these 2 options but it would need to be a tri-state to
keep the same options. Here's how the options interact:
!copy_as_child && !replace: if dest exists the copy fails
copy_as_child && !replace: if dest exists it will be a child of dest
!copy_as_child && replace: if dest exists it will be replaced
copy_as_child && replace: if dest exists it will be replaced
Because of how the code was written, replacement will override
copy_as_child. However, passing false for replacs will cause
copy_as_child to work as it currently does.
>
> > 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'?)
Hmmm...it was so easy to add to copy, though. All of the correct
functionality was already in place except for a deletion of dest
before the copy.
>
> 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.
mucc does do what I want, but it requires a revision # for the copy
(easy to get, but an annoying limitation if you just want HEAD). I'd
also think that adding support for such a simple operation to
libsvn_client could be beneficial for other clients as they could then
do a replacement in a repo-to-repo copy by just passing true for the
replace parameter.
>
> > 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 understand, but I had a 1.4.2 patch as that is what I tested with
first and I figured I'd send it.
>
> > 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.
>
I'd forgotten that in my haste to get to bed last night. Here's the
commit message:
Add support to repo-to-repo copy for replacing the destination
* subversion/libsvn_client/copy.c
(svn_client_copy4, repos_to_repos_copy, setup_copy): Add a replace parameter
and supporting code to allow for replacement of dest on a repo-to-repo copy.
Current functionality is not changed if replace is FALSE.
(svn_client_copy3, svn_client_move5, svn_client_move): pass FALSE for replace,
keeps current logic.
* subversion/include/svn_client.h
(svn_client_copy4): update prototype
* subversion/svn/cl.h
(svn_cl__longopt_t): add svn_cl__replace_opt to long options
(svn_cl__opt_state_t): add replace switch
* subversion/svn/main.c
(svn_cl__options): add the replace option and description
(svn_cl__cmd_table): add the replace option to copy's option list
(main): set opt_state.replace if passed in
* subversion/svn/copy-cmd.c
(svn_cl__copy): pass the replace option to svn_client_copy4
> [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
>
>
>
>
>
--
Justin Patrin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jan 25 19:36:14 2007