On Tue, Jun 02, 2009 at 05:06:39PM +0100, Julian Foad wrote:
> On Tue, 2009-06-02 at 15:35 +0100, Stefan Sperling wrote:
> > On Tue, Jun 02, 2009 at 01:38:22PM +0100, Julian Foad wrote:
> > > Stefan Sperling wrote:
> > > > On Sat, May 30, 2009 at 11:51:09AM +0100, Stefan Sperling wrote:
> > > > > I'll need more time diving into the matter to pin-point the exact
> > > > > cause of the problem.
> > > >
> > > > Still talking to myself here, but just following up with latest
> > > > thoughts in case anyone is listening.
> > >
> > > Hi Stefan. I'm listening!
> >
> > Great! :)
> >
> > Thanks for your comments. I agree that a quick short-term
> > fix could be made. I'd say we could add peg-rev parsing to all
> > subcommands (not just 'add') and port those changes back to 1.6.x.
> > On trunk, we can do the "proper" fix, transforming the target
> > array from an array of strings to an array of (name, reg-rev) pairs.
> >
> > While I got you here, can you explain to me why we moved
> > client_args_to_target_array functionality from libsvn_subr
> > into libsvn_client?
> > I can't find the rational for the move in the log message of r30753.
> >
> > See also r33317 which partly restored the functionality in libsvn_subr
> > as svn_opt__args_to_target_array(), so that e.g. svnadmin doesn't
> > need to link to libsvn_client.
> >
> > Why can't we just put things in one place?
>
> Ah... That looks like a mistake. The email in which I advocated it is
> <http://svn.haxx.se/dev/archive-2008-03/0188.shtml>. I think my flawed
> reasoning was that "clients" meant "programs that use the Subversion
> libraries", forgetting that it really meant "programs that communicate
> remotely to a Subversion server".
It still makes sense to have it libsvn_client because
svn_client_args_to_target_array() may attempt to contact the repository.
I have now taken the simple approach to this problem, and made
every subcommand parse peg revisions. I added a new helper
function that subcommands not using peg revisions can use to
do this comfortably.
The "proper" fix using a new struct separating paths and peg
revisions is much harder to do and might be overkill.
The patch is below, comments welcome. It passes make check,
so I will commit it soonish if there are no negative comments.
Thanks,
Stefan
Index: subversion/libsvn_subr/opt.c
===================================================================
--- subversion/libsvn_subr/opt.c (revision 37973)
+++ subversion/libsvn_subr/opt.c (working copy)
@@ -873,13 +873,22 @@ svn_opt__split_arg_at_peg_revision(const char **tr
if (peg_start)
{
+ /* Error out if target is the empty string. */
+ if (j == 0)
+ return svn_error_createf(SVN_ERR_BAD_FILENAME, NULL,
+ _("'%s' is just a peg revision. "
+ "Maybe try '%s@' instead?"),
+ utf8_target, utf8_target);
+
*true_target = apr_pstrmemdup(pool, utf8_target, j);
- *peg_revision = apr_pstrdup(pool, peg_start);
+ if (peg_revision)
+ *peg_revision = apr_pstrdup(pool, peg_start);
}
else
{
*true_target = utf8_target;
- *peg_revision = "";
+ if (peg_revision)
+ *peg_revision = "";
}
return SVN_NO_ERROR;
@@ -1018,3 +1027,29 @@ svn_opt_print_help3(apr_getopt_t *os,
return SVN_NO_ERROR;
}
+
+svn_error_t *
+svn_opt_eat_peg_revisions(apr_array_header_t **true_targets_p,
+ apr_array_header_t *targets,
+ apr_pool_t *pool)
+{
+ unsigned int i;
+ apr_array_header_t *true_targets;
+
+ true_targets = apr_array_make(pool, DEFAULT_ARRAY_SIZE, sizeof(const char *));
+
+ for (i = 0; i < targets->nelts; i++)
+ {
+ 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));
+ APR_ARRAY_PUSH(true_targets, const char *) = true_target;
+ }
+
+ SVN_ERR_ASSERT(true_targets_p);
+ *true_targets_p = true_targets;
+
+ return SVN_NO_ERROR;
+}
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 37973)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -1660,14 +1660,14 @@ def basic_peg_revision(sbox):
wc_dir = sbox.wc_dir
repos_dir = sbox.repo_url
filename = 'abc_at_abc'
-
- wc_file = wc_dir + '/' + filename
+ wc_file = os.path.join(wc_dir, filename)
url = repos_dir + '/' + filename
svntest.main.file_append(wc_file, 'xyz\n')
- svntest.main.run_svn(None, 'add', wc_file)
+ # We need to escape the @ in the middle of abc_at_abc by appending another @
+ svntest.main.run_svn(None, 'add', wc_file + '@')
svntest.main.run_svn(None,
- 'ci', '-m', 'secret log msg', wc_file)
+ 'ci', '-m', 'secret log msg', wc_file + '@')
# Without the trailing "@", expect failure.
exit_code, output, errlines = svntest.actions.run_and_verify_svn(
@@ -1677,11 +1677,33 @@ def basic_peg_revision(sbox):
# With the trailing "@", expect success.
exit_code, output, errlines = svntest.actions.run_and_verify_svn(
- None, ["xyz\n"], [], 'cat', wc_file+'@')
+ None, ["xyz\n"], [], 'cat', wc_file + '@')
exit_code, output, errlines = svntest.actions.run_and_verify_svn(
- None, ["xyz\n"], [], 'cat', url+'@')
+ None, ["xyz\n"], [], 'cat', url + '@')
+ # Test with leading @ character in filename.
+ filename = '@abc'
+ wc_file = os.path.join(wc_dir, filename)
+ url = repos_dir + '/' + filename
+ svntest.main.file_append(wc_file, 'xyz\n')
+ svntest.main.run_svn(None, 'add', wc_file + '@')
+ svntest.main.run_svn(None, 'ci', '-m', 'secret log msg', wc_file + '@')
+
+ # With a leading "@" which isn't escaped, expect failure.
+ # Note that we just test with filename starting with '@', because
+ # wc_file + '@' + filename is a different situation where svn
+ # will try to parse filename as a peg revision.
+ exit_code, output, errlines = svntest.actions.run_and_verify_svn(
+ None, None, ".*'%s' is just a peg revision.*" % filename,
+ 'cat', filename)
+
+ # With a leading "@" which is escaped, expect success.
+ exit_code, output, errlines = svntest.actions.run_and_verify_svn(
+ None, ["xyz\n"], [], 'cat', wc_file + '@')
+ exit_code, output, errlines = svntest.actions.run_and_verify_svn(
+ None, ["xyz\n"], [], 'cat', repos_dir + '/' + filename + '@')
+
def info_nonhead(sbox):
"info on file not existing in HEAD"
sbox.build()
Index: subversion/svn/patch-cmd.c
===================================================================
--- subversion/svn/patch-cmd.c (revision 37973)
+++ subversion/svn/patch-cmd.c (working copy)
@@ -61,6 +61,7 @@ svn_cl__patch(apr_getopt_t *os,
svn_opt_push_implicit_dot_target(targets, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
target_path = APR_ARRAY_IDX(targets, 0, const char *);
if (! opt_state->quiet)
Index: subversion/svn/propdel-cmd.c
===================================================================
--- subversion/svn/propdel-cmd.c (revision 37973)
+++ subversion/svn/propdel-cmd.c (working copy)
@@ -92,6 +92,8 @@ svn_cl__propdel(apr_getopt_t *os,
ctx->notify_baton2 = &nwb;
}
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
if (opt_state->revprop) /* operate on a revprop */
{
svn_revnum_t rev;
Index: subversion/svn/mkdir-cmd.c
===================================================================
--- subversion/svn/mkdir-cmd.c (revision 37973)
+++ subversion/svn/mkdir-cmd.c (working copy)
@@ -73,6 +73,8 @@ svn_cl__mkdir(apr_getopt_t *os,
NULL, ctx->config, pool));
}
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
err = svn_client_mkdir3(&commit_info, targets, opt_state->parents,
opt_state->revprop_table, ctx, pool);
Index: subversion/svn/move-cmd.c
===================================================================
--- subversion/svn/move-cmd.c (revision 37973)
+++ subversion/svn/move-cmd.c (working copy)
@@ -82,6 +82,8 @@ svn_cl__move(apr_getopt_t *os,
SVN_ERR(svn_cl__make_log_msg_baton(&(ctx->log_msg_baton3), opt_state,
NULL, ctx->config, pool));
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
err = svn_client_move5(&commit_info, targets, dst_path, opt_state->force,
TRUE, opt_state->parents, opt_state->revprop_table,
ctx, pool);
Index: subversion/svn/revert-cmd.c
===================================================================
--- subversion/svn/revert-cmd.c (revision 37973)
+++ subversion/svn/revert-cmd.c (working copy)
@@ -60,6 +60,8 @@ svn_cl__revert(apr_getopt_t *os,
if (opt_state->depth == svn_depth_unknown)
opt_state->depth = svn_depth_empty;
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
err = svn_client_revert2(targets, opt_state->depth,
opt_state->changelists, ctx, pool);
Index: subversion/svn/changelist-cmd.c
===================================================================
--- subversion/svn/changelist-cmd.c (revision 37973)
+++ subversion/svn/changelist-cmd.c (working copy)
@@ -92,6 +92,8 @@ svn_cl__changelist(apr_getopt_t *os,
if (depth == svn_depth_unknown)
depth = svn_depth_empty;
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
if (changelist_name)
{
return svn_cl__try
Index: subversion/svn/update-cmd.c
===================================================================
--- subversion/svn/update-cmd.c (revision 37973)
+++ subversion/svn/update-cmd.c (working copy)
@@ -53,6 +53,8 @@ svn_cl__update(apr_getopt_t *os,
/* Add "." if user passed 0 arguments */
svn_opt_push_implicit_dot_target(targets, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
/* If using changelists, convert targets into a set of paths that
match the specified changelist(s). */
if (opt_state->changelists)
Index: subversion/svn/resolved-cmd.c
===================================================================
--- subversion/svn/resolved-cmd.c (revision 37973)
+++ subversion/svn/resolved-cmd.c (working copy)
@@ -62,6 +62,8 @@ svn_cl__resolved(apr_getopt_t *os,
if (opt_state->depth == svn_depth_unknown)
opt_state->depth = svn_depth_empty;
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
for (i = 0; i < targets->nelts; i++)
{
const char *target = APR_ARRAY_IDX(targets, i, const char *);
Index: subversion/svn/upgrade-cmd.c
===================================================================
--- subversion/svn/upgrade-cmd.c (revision 37973)
+++ subversion/svn/upgrade-cmd.c (working copy)
@@ -53,6 +53,8 @@ svn_cl__upgrade(apr_getopt_t *os,
/* Add "." if user passed 0 arguments */
svn_opt_push_implicit_dot_target(targets, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
subpool = svn_pool_create(pool);
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/svn/cleanup-cmd.c
===================================================================
--- subversion/svn/cleanup-cmd.c (revision 37973)
+++ subversion/svn/cleanup-cmd.c (working copy)
@@ -50,6 +50,8 @@ svn_cl__cleanup(apr_getopt_t *os,
/* Add "." if user passed 0 arguments */
svn_opt_push_implicit_dot_target(targets, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
subpool = svn_pool_create(pool);
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/svn/commit-cmd.c
===================================================================
--- subversion/svn/commit-cmd.c (revision 37973)
+++ subversion/svn/commit-cmd.c (working copy)
@@ -69,6 +69,8 @@ svn_cl__commit(apr_getopt_t *os,
/* Add "." if user passed 0 arguments. */
svn_opt_push_implicit_dot_target(targets, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
/* Condense the targets (like commit does)... */
SVN_ERR(svn_path_condense_targets(&base_dir,
&condensed_targets,
Index: subversion/svn/add-cmd.c
===================================================================
--- subversion/svn/add-cmd.c (revision 37973)
+++ subversion/svn/add-cmd.c (working copy)
@@ -59,6 +59,8 @@ svn_cl__add(apr_getopt_t *os,
if (opt_state->depth == svn_depth_unknown)
opt_state->depth = svn_depth_infinity;
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
subpool = svn_pool_create(pool);
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/svn/propset-cmd.c
===================================================================
--- subversion/svn/propset-cmd.c (revision 37973)
+++ subversion/svn/propset-cmd.c (working copy)
@@ -104,6 +104,8 @@ svn_cl__propset(apr_getopt_t *os,
if (opt_state->revprop)
svn_opt_push_implicit_dot_target(targets, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
if (opt_state->revprop) /* operate on a revprop */
{
svn_revnum_t rev;
Index: subversion/svn/delete-cmd.c
===================================================================
--- subversion/svn/delete-cmd.c (revision 37973)
+++ subversion/svn/delete-cmd.c (working copy)
@@ -72,6 +72,8 @@ svn_cl__delete(apr_getopt_t *os,
NULL, ctx->config, pool));
}
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
err = svn_client_delete3(&commit_info, targets, opt_state->force,
opt_state->keep_local, opt_state->revprop_table,
ctx, pool);
Index: subversion/svn/resolve-cmd.c
===================================================================
--- subversion/svn/resolve-cmd.c (revision 37973)
+++ subversion/svn/resolve-cmd.c (working copy)
@@ -91,6 +91,8 @@ svn_cl__resolve(apr_getopt_t *os,
if (opt_state->depth == svn_depth_unknown)
opt_state->depth = svn_depth_empty;
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
for (i = 0; i < targets->nelts; i++)
{
const char *target = APR_ARRAY_IDX(targets, i, const char *);
Index: subversion/svn/status-cmd.c
===================================================================
--- subversion/svn/status-cmd.c (revision 37973)
+++ subversion/svn/status-cmd.c (working copy)
@@ -218,6 +218,8 @@ svn_cl__status(apr_getopt_t *os,
sb.cached_changelists = master_cl_hash;
sb.cl_pool = pool;
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
for (i = 0; i < targets->nelts; i++)
{
const char *target = APR_ARRAY_IDX(targets, i, const char *);
Index: subversion/svn/propedit-cmd.c
===================================================================
--- subversion/svn/propedit-cmd.c (revision 37973)
+++ subversion/svn/propedit-cmd.c (working copy)
@@ -169,6 +169,8 @@ svn_cl__propedit(apr_getopt_t *os,
_("Explicit target argument required"));
}
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
/* For each target, edit the property PNAME. */
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/svn/lock-cmd.c
===================================================================
--- subversion/svn/lock-cmd.c (revision 37973)
+++ subversion/svn/lock-cmd.c (working copy)
@@ -99,5 +99,7 @@ svn_cl__lock(apr_getopt_t *os,
svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2, FALSE,
FALSE, FALSE, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
return svn_client_lock(targets, comment, opt_state->force, ctx, pool);
}
Index: subversion/svn/unlock-cmd.c
===================================================================
--- subversion/svn/unlock-cmd.c (revision 37973)
+++ subversion/svn/unlock-cmd.c (working copy)
@@ -55,5 +55,7 @@ svn_cl__unlock(apr_getopt_t *os,
svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2, FALSE,
FALSE, FALSE, pool);
+ SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+
return svn_client_unlock(targets, opt_state->force, ctx, pool);
}
Index: subversion/include/private/svn_opt_private.h
===================================================================
--- subversion/include/private/svn_opt_private.h (revision 37973)
+++ subversion/include/private/svn_opt_private.h (working copy)
@@ -32,15 +32,23 @@
extern "C" {
#endif /* __cplusplus */
-/* Extract the peg revision, if any, from UTF8_TARGET. Return the peg
- * revision in *PEG_REVISION and the true target portion in *TRUE_TARGET.
+/* Extract the peg revision, if any, from UTF8_TARGET.
+ *
+ * If PEG_REVISION is not NULL, return the peg revision in *PEG_REVISION.
* *PEG_REVISION will be an empty string if no peg revision is found.
+ * Return the true target portion in *TRUE_TARGET.
*
* UTF8_TARGET need not be canonical. *TRUE_TARGET will not be canonical
* unless UTF8_TARGET is.
*
+ * It is an error if *TRUE_TARGET results in the empty string after the
+ * split, which happens in case UTF8_TARGET has a leading '@' character
+ * with no additional '@' characters to escape the first '@'.
+ *
* Note that *PEG_REVISION will still contain the '@' symbol as the first
- * character if a peg revision was found.
+ * character if a peg revision was found. If a trailing '@' symbol was
+ * used to escape other '@' characters in UTF8_TARGET, *PEG_REVISION will
+ * point to the string "@", containing only a single character.
*
* All allocations are done in POOL.
*/
Index: subversion/include/svn_opt.h
===================================================================
--- subversion/include/svn_opt.h (revision 37973)
+++ subversion/include/svn_opt.h (working copy)
@@ -621,11 +621,14 @@ svn_opt_parse_all_args(apr_array_header_t **args_p
* "foo/bar_at_1:2" -> error
* "foo/bar_at_baz" -> error
* "foo/bar@" -> "foo/bar", (base)
+ * "foo/@bar@" -> "foo/@bar", (base)
* "foo/bar/@13" -> "foo/bar/", (number, 13)
* "foo/bar@@13" -> "foo/bar@", (number, 13)
* "foo/@bar_at_HEAD" -> "foo/@bar", (head)
* "foo@/bar" -> "foo@/bar", (unspecified)
* "foo_at_HEAD/bar" -> "foo_at_HEAD/bar", (unspecified)
+ * "@foo/bar" -> error
+ * "@foo/bar@" -> "@foo/bar", (unspecified)
*
* [*] Syntactically valid but probably not semantically useful.
*
@@ -734,6 +737,27 @@ svn_opt_print_help(apr_getopt_t *os,
const char *footer,
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.
+ *
+ * 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
+ * the command line client can consistently escape '@' characters
+ * in filenames by appending an '@' character, regardless of the
+ * subcommand being used.
+ *
+ * If a peg revision is present but cannot be parsed, an error is thrown.
+ * The user has likely forgotten to escape an '@' character in a filename.
+ *
+ * It is safe to pass the address of @a targets as @a true_targets_p.
+ *
+ * Do all allocations in @a pool. */
+svn_error_t *
+svn_opt_eat_peg_revisions(apr_array_header_t **true_targets_p,
+ apr_array_header_t *targets,
+ apr_pool_t *pool);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Received on 2009-06-10 02:52:14 CEST