I believe we should make 'svn' reject any peg revision specifier on an
argument where it doesn't use one. This is a rather long email to
record some of the details behind my reasoning, and a question mainly
for Stefan Sperling about issue #3651 [3].
First some context.
$ svn cat ^/trunk/readme@123
# Makes sense.
$ svn mkdir ^/trunk/doc@123
# Nonsense: mkdir with a URL can only operate on the head. [2]
$ svn add foo@123
# Nonsense: the target must be an unversioned local path.
We decided [1] that we should allow the user to consistently escape any
target argument by appending an '@' sign, without having to know whether
a peg revision is relevant. This was implemented in 1.6.5 and trunk by
parsing off and discarding any peg revision specifier, using the
function 'svn_cl__eat_peg_revisions()'.
That current solution is great for allowing consistent escaping, but as
a side effect 'svn' silently ignores any peg revision it isn't going to
use, which I think is bad, as I have noted in the doc string of
svn_cl__eat_peg_revisions().
So, when does a peg not make sense?
(a) When the target is supposed to be an unversioned local path, such
as in 'svn add'. A peg never makes sense on an unversioned path. A
versioned local path is allowed with '--force', but it would still be
wrong to specify any peg revision on it because only the path on disk is
relevant to the 'add' operation.
(b) When the target necessarily refers to a path at the head of the
repository, such as in 'svn lock TARGET' or 'svn delete TARGET-URL'.
Logically, a peg of 'HEAD' is always valid in these commands.
Logically, these commands could accept a non-head peg where TARGET is
known to exist. Then, if someone else moves TARGET to another path
while we are composing and executing this command, Subversion could do
its best to follow the move or error out if it can't do so. But unless
and until 'svn' actually uses the peg revision in this way, it should
complain, not silently ignore it.
(c) When the target refers to a new path that will be created in a new
revision on top of the old head, such as in 'svn mkdir TARGET-URL' or
the destination of 'svn copy SOURCE DEST-URL'. In most of these cases,
the target does not exist in the repository at the time the command is
issued so there is no possible peg revision specifier. The target of a
copy may be specified as an existing directory, and in that case it
could accept a peg of 'HEAD' or even a non-head peg as described in (b).
My question is about this case:
$ svn copy SOURCE TARGET@HEAD
This case falls under the last sentence of point (C) above. Issue #3651
[3] claims that the 'HEAD' peg should be ignored in this case, and that
claim is explicitly tested by copy_tests 62 [4]. (It is the only test
that fails with the attached patch.)
But is 'HEAD' supposed to be a special case on the basis that it is
redundant but still correct? If so, then we should also allow '123' iff
that is currently the head, and 'BASE' for a local path iff that is
currently the head, and so on. In other words I don't think we have
reason to claim that 'HEAD' is a special case.
The logic gets very messy if we think we must support redundant pegs.
Even if we only implement a subset of the messy logic (such as just
allowing 'HEAD') the conceptual logic is messy which is a bad thing for
documentation and learning. So my conclusion is that we should reject
any peg revision specifier, even 'HEAD', in places where we're not
prepared to use it.
If that's agreeable, I'll make it happen. (The attached patch is not
quite finished; for example it has no doc string for one of the new
functions.)
- Julian
[1] Issue #3416 "Cannot add or commit 'dir/@file.txt'",
<http://subversion.tigris.org/issues/show_bug.cgi?id=3416>.
[2] The only ways I can imagine the second example could make sense are
if it were to mean, "Make this directory, assuming that HEAD is 123, and
fail if not" or, "Make this directory, assuming that HEAD is 122 so that
the committed revision will be 123, and fail if not." But we don't have
any plans to implement such semantics, and the syntax does not make
sense for the semantics we have implemented.
[3] Issue #3651 "svn copy does not eat peg revision within copy target
path", <http://subversion.tigris.org/issues/show_bug.cgi?id=3651>.
[4] <http://svn.apache.org/viewvc?revision=952992&view=revision>
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;
}
