Disallow a non-empty peg revision specifier on any 'svn' command-line argument where a peg revision is not wanted. The previous behaviour was to silently ignore any such peg revision specifier. That was done so that an empty specifier (for example, 'file@') could be used consistently on any argument in order to escape any earlier '@' sign in the argument. This is related to issue #3416 "Cannot add or commit 'dir/@file.txt'" and issue #3651 "svn copy does not eat peg revision within copy target path". * subversion/svn/cl.h, subversion/svn/util.c (svn_cl__eat_peg_revision): New function. (svn_cl__eat_peg_revisions): Raise an error if a non-empty peg revision is found. Document the new behaviour and remove the "TODO" note about this. * subversion/svn/copy-cmd.c (svn_cl__copy): Instead of calling svn_cl__eat_peg_revisions() on the whole 'targets' array, perform the equivalent check on just the relevant target, but also allow '@HEAD' to be specified if the target is a URL because issue #3651 is about that specific case and there is a test for it. Also eliminate two unnecessary variables. * subversion/svn/export-cmd.c (svn_cl__export): Use svn_cl__eat_peg_revision() instead of in-line code that discarded a peg revision from the destination path. --This line, and those below, will be ignored-- Index: subversion/svn/cl.h =================================================================== --- subversion/svn/cl.h (revision 1140544) +++ subversion/svn/cl.h (working copy) @@ -767,35 +767,18 @@ svn_cl__node_description(const svn_wc_co const char *wc_repos_root_URL, apr_pool_t *pool); -/* Return, in @a *true_targets_p, a copy of @a targets with peg revision - * specifiers snipped off the end of each element. - * - * ### JAF TODO: This function is not good because it does not allow the - * ### caller to detect if an invalid peg revision was specified. - * ### - * ### Callers should never have a need to silently *discard* all peg - * ### revisions, even if they are doing this *after* saving any peg - * ### revisions that might be of interest on certain arguments: I don't - * ### think it can ever be correct to silently ignore a peg revision that - * ### is specified, whether it makes semantic sense or not. - * ### - * ### Instead, callers should parse all the arguments and silently - * ### ignore an *empty* peg revision part (just an "@", which can be - * ### used to escape an earlier "@" in the argument) on any argument, - * ### even an argument on which a peg revision does not make sense, - * ### but should not silently ignore a non-empty peg when it does not - * ### make sense. - * ### - * ### Something like: - * ### For each (URL-like?) argument that doesn't accept a peg rev: - * ### Parse into peg-rev and true-path parts; - * ### If (peg rev != unspecified) - * ### Error("This arg doesn't accept a peg rev."). - * ### Use the true-path part. +svn_error_t * +svn_cl__eat_peg_revision(const char **true_target, + const char *target, + apr_pool_t *pool); + +/* Return, in @a *true_targets_p, a copy of @a targets with empty peg + * revision specifiers snipped off the end of each element. If any element + * has a non-empty peg revision specifier, throw an error. * * This function is useful for subcommands for which peg revisions - * do not make any sense. Such subcommands still need to allow peg - * revisions to be specified on the command line so that users of + * do not make any sense. Such subcommands still need to parse off any + * empty peg revision specifier on the command line so that users of * the command line client can consistently escape '@' characters * in filenames by appending an '@' character, regardless of the * subcommand being used. Index: subversion/svn/copy-cmd.c =================================================================== --- subversion/svn/copy-cmd.c (revision 1140544) +++ subversion/svn/copy-cmd.c (working copy) @@ -46,7 +46,7 @@ svn_cl__copy(apr_getopt_t *os, svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state; svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx; apr_array_header_t *targets, *sources; - const char *src_path, *dst_path; + const char *dst_path; svn_boolean_t srcs_are_urls, dst_is_url; svn_error_t *err; int i; @@ -64,28 +64,35 @@ svn_cl__copy(apr_getopt_t *os, { const char *target = APR_ARRAY_IDX(targets, i, const char *); svn_client_copy_source_t *source = apr_palloc(pool, sizeof(*source)); - const char *src; svn_opt_revision_t *peg_revision = apr_palloc(pool, sizeof(*peg_revision)); - SVN_ERR(svn_opt_parse_path(peg_revision, &src, target, pool)); - source->path = src; + SVN_ERR(svn_opt_parse_path(peg_revision, &source->path, target, pool)); source->revision = &(opt_state->start_revision); source->peg_revision = peg_revision; APR_ARRAY_PUSH(sources, svn_client_copy_source_t *) = source; } - SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, pool)); - /* Figure out which type of notification to use. (There is no need to check that the src paths are homogeneous; svn_client_copy6() through its subroutine try_copy() will return an error if they are not.) */ - src_path = APR_ARRAY_IDX(targets, 0, const char *); - srcs_are_urls = svn_path_is_url(src_path); - dst_path = APR_ARRAY_IDX(targets, targets->nelts - 1, const char *); - apr_array_pop(targets); + srcs_are_urls = svn_path_is_url(APR_ARRAY_IDX(targets, 0, const char *)); + + /* Find DST_PATH and check it has no peg (or is in the form 'URL@HEAD'). */ + { + const char *target = APR_ARRAY_IDX(targets, targets->nelts - 1, + const char *); + svn_opt_revision_t peg; + + SVN_ERR(svn_opt_parse_path(&peg, &dst_path, target, pool)); + if (! (peg.kind == svn_opt_revision_unspecified + || (peg.kind == svn_opt_revision_head && svn_path_is_url(target)))) + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, + _("Unexpected peg revision on '%s'"), + target); + } dst_is_url = svn_path_is_url(dst_path); if ((! srcs_are_urls) && (! dst_is_url)) Index: subversion/svn/export-cmd.c =================================================================== --- subversion/svn/export-cmd.c (revision 1140544) +++ subversion/svn/export-cmd.c (working copy) @@ -81,11 +81,9 @@ svn_cl__export(apr_getopt_t *os, } else { - to = APR_ARRAY_IDX(targets, 1, const char *); + const char *target = APR_ARRAY_IDX(targets, 1, const char *); - if (strcmp("", to) != 0) - /* svn_cl__eat_peg_revisions() but only on one target */ - SVN_ERR(svn_opt__split_arg_at_peg_revision(&to, NULL, to, pool)); + SVN_ERR(svn_cl__eat_peg_revision(&to, target, pool)); } SVN_ERR(svn_cl__check_target_is_local_path(to)); Index: subversion/svn/util.c =================================================================== --- subversion/svn/util.c (revision 1140544) +++ subversion/svn/util.c (working copy) @@ -1335,6 +1335,23 @@ svn_cl__node_description(const svn_wc_co } svn_error_t * +svn_cl__eat_peg_revision(const char **true_target, + const char *target, + apr_pool_t *pool) +{ + const char *peg; + + SVN_ERR(svn_opt__split_arg_at_peg_revision(true_target, &peg, + target, pool)); + if (peg[0] == '@' && peg[1] != '\0') + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL, + _("Unexpected peg revision on '%s'"), + target); + + return SVN_NO_ERROR; +} + +svn_error_t * svn_cl__eat_peg_revisions(apr_array_header_t **true_targets_p, const apr_array_header_t *targets, apr_pool_t *pool) @@ -1349,8 +1366,7 @@ svn_cl__eat_peg_revisions(apr_array_head const char *target = APR_ARRAY_IDX(targets, i, const char *); const char *true_target; - SVN_ERR(svn_opt__split_arg_at_peg_revision(&true_target, NULL, - target, pool)); + SVN_ERR(svn_cl__eat_peg_revision(&true_target, target, pool)); APR_ARRAY_PUSH(true_targets, const char *) = true_target; }