You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@hyrumwright.org> on 2009/05/29 17:58:23 UTC
svn_client_args_to_target_array and peg revisions -> no good
[ for Stefan ]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2356899
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 11, 2009 at 11:15:10AM -0400, C. Michael Pilato wrote:
> What's your backport plan, though? Didn't you add a new public API?
> Planning to make it a private/public one?
I made it public because as far as I remember it's OK for our
libraries to call each others' private API, but it's not OK
for our command line client to call our private API.
Just like it's not OK for other clients out there to call
our private API.
Is that not correct?
If not, we could decide to either:
1) Make the new function private everywhere.
2) Make the new function private in 1.6.x only.
I'm indifferent.
Stefan
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> On Thu, Jun 11, 2009 at 10:21:06AM -0400, C. Michael Pilato wrote:
>> Do we not having a single function that can take a array of const char *,
>> native encoding path-spec arguments and return an array of structures that
>> contain a UTF-8 const char * path field and peg revision? Seems like we
>> could stand to have such a thing if we don't already. We could then have
>> helper functions that accept one of those structures and do other
>> transformations on it, such as expanding shortcut URLs (^/) and IRIs
>> ("http://server/path with spaces") into canonical form, and so on.
>>
>> I dunno ... I guess I just figured that after 9 years, we'd know how to
>> consistently parse our command-line arguments.
>
> Yes, I agree that this is necessary.
> I have opened a new bite-sized issue:
> http://subversion.tigris.org/issues/show_bug.cgi?id=3423
>
> I've just committed my patch in r37988 (which probably triggered your
> response), and I still think it is useful because it should be far easier
> to backport to 1.6 than what you are describing. There are two minor
> tree conflicts, that's all. I'll create a backport branch in a minute.
Actually, it was mere coincidence that you committed while I was catching up
on the thread and composing my response. :-)
What's your backport plan, though? Didn't you add a new public API?
Planning to make it a private/public one?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361322
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 11, 2009 at 10:21:06AM -0400, C. Michael Pilato wrote:
> Do we not having a single function that can take a array of const char *,
> native encoding path-spec arguments and return an array of structures that
> contain a UTF-8 const char * path field and peg revision? Seems like we
> could stand to have such a thing if we don't already. We could then have
> helper functions that accept one of those structures and do other
> transformations on it, such as expanding shortcut URLs (^/) and IRIs
> ("http://server/path with spaces") into canonical form, and so on.
>
> I dunno ... I guess I just figured that after 9 years, we'd know how to
> consistently parse our command-line arguments.
Yes, I agree that this is necessary.
I have opened a new bite-sized issue:
http://subversion.tigris.org/issues/show_bug.cgi?id=3423
I've just committed my patch in r37988 (which probably triggered your
response), and I still think it is useful because it should be far easier
to backport to 1.6 than what you are describing. There are two minor
tree conflicts, that's all. I'll create a backport branch in a minute.
Thanks,
Stefan
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> 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
Do we not having a single function that can take a array of const char *,
native encoding path-spec arguments and return an array of structures that
contain a UTF-8 const char * path field and peg revision? Seems like we
could stand to have such a thing if we don't already. We could then have
helper functions that accept one of those structures and do other
transformations on it, such as expanding shortcut URLs (^/) and IRIs
("http://server/path with spaces") into canonical form, and so on.
I dunno ... I guess I just figured that after 9 years, we'd know how to
consistently parse our command-line arguments.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361301
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Stefan Sperling <st...@elego.de>.
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@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@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@1:2" -> error
* "foo/bar@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@HEAD" -> "foo/@bar", (head)
* "foo@/bar" -> "foo@/bar", (unspecified)
* "foo@HEAD/bar" -> "foo@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 */
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Julian Foad <ju...@btopenworld.com>.
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".
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2358825
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Stefan Sperling <st...@elego.de>.
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?
Thanks,
Stefan
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Julian Foad <ju...@btopenworld.com>.
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!
Your analysis (below) looks spot on, and I agree that replacing the
string data format with a (path, rev) structure would be good. See the
thread ending with this message
<http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=40874&orderBy=createDate&orderType=desc> in which Troy was starting to do just that. Troy, ping?
However, I think first it would be sensible to fix the problem with "svn
add" by simply making it parse peg revisions*, and preferably check that
no peg was specified. That should be sufficient to fix the reported bug.
The API change could be done as a separate internal improvement.
(* I believe all commands should accept peg-rev syntax even where peg
revs don't make sense, like the quote you found in the book suggests.
Otherwise the command-line syntax would have ugly consistencies. For
example, a sequence of commands specifying the same local target such as
"svn add $1; svn commit $1" would not work when the target contains
"@".)
- Julian
> Q: Why does issue #3416 happen?
>
> A: Well, let's look at one case at a time.
>
> First case:
>
> $ svn add 'dir/@File.txt'
> svn: warning: 'dir@File.txt' not found
>
> This happens because svn_client_args_to_target_array() splits peg
> revisions off all path arguments passed on the command line.
> 'dir/@File.txt' gets split into
> target_path = 'dir/'
> peg_rev = '@File.text'
> The function then canonicalizes the target path, so we get:
> target_path = 'dir'
> peg_rev = '@File.text'
> Then the two get concatenated again, resulting in 'dir@File.txt',
> the string which eventually ends up going to 'svn add' unmodified.
> 'svn add' does not try to parse peg revisions in its arguments, so
> it tries to add a file called 'dir@File.txt' which isn't what the
> user wanted.
>
> Second case (the user tries to escape the first @ by appending another):
>
> $ svn add 'dir/@File.txt@'
> svn: warning: 'dir/@File.txt@' not found
> In this case, svn_client_args_to_target_array() splits the path up as:
> target_path = 'dir/@File.text'
> peg_rev = '@'
> Then the two get concatenated again, resulting in 'dir/@File.txt@',
> the string which eventually ends up going to 'svn add' unmodified.
> But 'svn add' does not try to parse peg revisions in its arguments,
> so even the escaping @ is just being ignored, which isn't what the
> user wanted.
>
> Q: OK well, so why does svn_client_args_to_target_array() concatenate
> these strings again after having split them up? It looks like
> doing so messes things up!
>
> A: Part of svn_client_args_to_target_array()'s job is to canonicalize
> input paths. It concatenates the target and the peg_rev again because
> when the paths end up in the argument list of the subcommand function,
> many subcommands will try to parse peg revisions again in a very similar
> way to what svn_client_args_to_target_array() already did.
> So svn_client_args_to_target_array() was written to accommodate for
> subcommands still doing their own peg-rev parsing. But not all subcommands
> actually do their own peg-rev parsing, e.g. 'svn add' does not.
>
> This is why we are seeing problems.
> !! There seems to be no single point of responsibility for peg-rev
> !! parsing in our code. Instead, peg-rev parsing is done in a rather
> !! ad-hoc and uncoordinated fashion. This is the root cause of #3416.
>
> Q: Right. What function(s) should do the parsing then?
>
> A: It seems like there is no consensus on this currently. One approach
> would be to parse peg-revs only for subcommands that can interpret them,
> and just interpret the '@' character as a literal '@' for all other
> subcommands, saving the user from having to escape '@' characters in
> arguments for those subcommands. But svn_client_args_to_target_array()
> currently breaks this approach, because it always runs before every
> subcommand and parses peg revisions in every argument path.
>
> The other approach would be to always do peg-rev parsing in all subcommands,
> and require users to always escape @ characters in all their filenames by
> appending an '@' to the filename, no matter which subcommand is being
> called. Some subcommands such as add currently prevent such escaping
> from happening, because they don't do any peg-rev parsing at all and
> just take all '@' characters literally.
>
> The svnbook suggests that the second approach is what should be
> happening:
> http://svnbook.red-bean.com/nightly/en/svn.advanced.pegrevs.html
> The perceptive reader is probably wondering at this point whether the
> peg revision syntax causes problems for working copy paths or URLs that
> actually have at signs in them. After all, how does svn know whether
> news@11 is the name of a directory in my tree or just a syntax for
> “revision 11 of news”? Thankfully, while
> svn will always assume the latter, <--- this statement is currently wrong!
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> there is a trivial workaround. You need only append an at sign
> to the end of the path, such as news@11@. svn cares only about the last
> at sign in the argument, and it is not considered illegal to omit a
> literal peg revision specifier after that at sign. This workaround even
> applies to paths that end in an at sign—you would use filename@@ to talk
> about a file named filename@.
>
> Q: Wouldn't it make more sense to do peg-rev parsing just once, and
> pass the parsed information to subcommands in a structured form that
> does not require subsequent parsing to get at peg-rev information?
> That way we'd have a clear point of responsibility for peg-rev parsing.
>
> A: Yes! I'm thinking that such an approach would be the most appropriate
> fix for issue #3416. Instead of passing targets as pure C strings
> with embedded peg-rev information, we'd pass a struct such as:
>
> struct { const char *path, svn_revision_t peg_rev} svn_target_t;
>
> svn_client_args_to_target_array() would create an array of such
> target structs. Subcommands would stop doing their own parsing.
> Those interested in peg revisions would refer to the 'peg_rev' field,
> while the other subcommands would just use the 'path' field.
>
> And an empty 'path' field would be an error. Conceptionally, this already
> is an error. But because of the way svn_client_args_to_target_array()
> splits and concatenates strings it currently is not an error.
> Which is why this works:
> $ svn add '@File.txt'
> A @File.txt
> svn_client_args_to_target_array() splits this up as:
> target = ''
> peg_rev = '@File.txt'
> Then the two get concatenated again, resulting in '@File.txt',
> the string which eventually ends up going to 'svn add' unmodified.
>
> Does anyone see anything wrong with the above observations and the
> suggested approach for a fix?
>
> Thanks,
> Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2358763
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Stefan Sperling <st...@elego.de>.
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.
Q: Why does issue #3416 happen?
A: Well, let's look at one case at a time.
First case:
$ svn add 'dir/@File.txt'
svn: warning: 'dir@File.txt' not found
This happens because svn_client_args_to_target_array() splits peg
revisions off all path arguments passed on the command line.
'dir/@File.txt' gets split into
target_path = 'dir/'
peg_rev = '@File.text'
The function then canonicalizes the target path, so we get:
target_path = 'dir'
peg_rev = '@File.text'
Then the two get concatenated again, resulting in 'dir@File.txt',
the string which eventually ends up going to 'svn add' unmodified.
'svn add' does not try to parse peg revisions in its arguments, so
it tries to add a file called 'dir@File.txt' which isn't what the
user wanted.
Second case (the user tries to escape the first @ by appending another):
$ svn add 'dir/@File.txt@'
svn: warning: 'dir/@File.txt@' not found
In this case, svn_client_args_to_target_array() splits the path up as:
target_path = 'dir/@File.text'
peg_rev = '@'
Then the two get concatenated again, resulting in 'dir/@File.txt@',
the string which eventually ends up going to 'svn add' unmodified.
But 'svn add' does not try to parse peg revisions in its arguments,
so even the escaping @ is just being ignored, which isn't what the
user wanted.
Q: OK well, so why does svn_client_args_to_target_array() concatenate
these strings again after having split them up? It looks like
doing so messes things up!
A: Part of svn_client_args_to_target_array()'s job is to canonicalize
input paths. It concatenates the target and the peg_rev again because
when the paths end up in the argument list of the subcommand function,
many subcommands will try to parse peg revisions again in a very similar
way to what svn_client_args_to_target_array() already did.
So svn_client_args_to_target_array() was written to accommodate for
subcommands still doing their own peg-rev parsing. But not all subcommands
actually do their own peg-rev parsing, e.g. 'svn add' does not.
This is why we are seeing problems.
!! There seems to be no single point of responsibility for peg-rev
!! parsing in our code. Instead, peg-rev parsing is done in a rather
!! ad-hoc and uncoordinated fashion. This is the root cause of #3416.
Q: Right. What function(s) should do the parsing then?
A: It seems like there is no consensus on this currently. One approach
would be to parse peg-revs only for subcommands that can interpret them,
and just interpret the '@' character as a literal '@' for all other
subcommands, saving the user from having to escape '@' characters in
arguments for those subcommands. But svn_client_args_to_target_array()
currently breaks this approach, because it always runs before every
subcommand and parses peg revisions in every argument path.
The other approach would be to always do peg-rev parsing in all subcommands,
and require users to always escape @ characters in all their filenames by
appending an '@' to the filename, no matter which subcommand is being
called. Some subcommands such as add currently prevent such escaping
from happening, because they don't do any peg-rev parsing at all and
just take all '@' characters literally.
The svnbook suggests that the second approach is what should be
happening:
http://svnbook.red-bean.com/nightly/en/svn.advanced.pegrevs.html
The perceptive reader is probably wondering at this point whether the
peg revision syntax causes problems for working copy paths or URLs that
actually have at signs in them. After all, how does svn know whether
news@11 is the name of a directory in my tree or just a syntax for
“revision 11 of news”? Thankfully, while
svn will always assume the latter, <--- this statement is currently wrong!
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
there is a trivial workaround. You need only append an at sign
to the end of the path, such as news@11@. svn cares only about the last
at sign in the argument, and it is not considered illegal to omit a
literal peg revision specifier after that at sign. This workaround even
applies to paths that end in an at sign—you would use filename@@ to talk
about a file named filename@.
Q: Wouldn't it make more sense to do peg-rev parsing just once, and
pass the parsed information to subcommands in a structured form that
does not require subsequent parsing to get at peg-rev information?
That way we'd have a clear point of responsibility for peg-rev parsing.
A: Yes! I'm thinking that such an approach would be the most appropriate
fix for issue #3416. Instead of passing targets as pure C strings
with embedded peg-rev information, we'd pass a struct such as:
struct { const char *path, svn_revision_t peg_rev} svn_target_t;
svn_client_args_to_target_array() would create an array of such
target structs. Subcommands would stop doing their own parsing.
Those interested in peg revisions would refer to the 'peg_rev' field,
while the other subcommands would just use the 'path' field.
And an empty 'path' field would be an error. Conceptionally, this already
is an error. But because of the way svn_client_args_to_target_array()
splits and concatenates strings it currently is not an error.
Which is why this works:
$ svn add '@File.txt'
A @File.txt
svn_client_args_to_target_array() splits this up as:
target = ''
peg_rev = '@File.txt'
Then the two get concatenated again, resulting in '@File.txt',
the string which eventually ends up going to 'svn add' unmodified.
Does anyone see anything wrong with the above observations and the
suggested approach for a fix?
Thanks,
Stefan
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 29, 2009 at 07:02:27PM +0100, Stefan Sperling wrote:
> So, looking at issue #3416, which is about "svn add" not being
> able to add files with names starting with the @ character,
> it seems to be a regression introduced in r30753 (that's why
> the two lads who did it are in Cc).
I really need to hone my archeological digging skills.
That revision was *not* the one that introduced the problem.
It was more likely r31458, which changed peg revision parsing
at multiple points in the code. So sorry for the noise. I'll
need more time diving into the matter to pin-point the exact
cause of the problem.
Stefan
Re: svn_client_args_to_target_array and peg revisions -> no good
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 29, 2009 at 12:58:23PM -0500, Hyrum K. Wright wrote:
> [ for Stefan ]
Thank you.
So, looking at issue #3416, which is about "svn add" not being
able to add files with names starting with the @ character,
it seems to be a regression introduced in r30753 (that's why
the two lads who did it are in Cc).
That commit introduced a command line parsing helper into
libsvn_client, called svn_client_args_to_target_array().
It effectively does reg-revisions parsing on all paths specified
on the command line, for every svn command, even for those where
it does not make sense to do so because the commands don't take
PATH@PEG-REV style arguments.
It looks like before this change, peg revision parsing was
only done when required, by explicitly calling svn_opt_parse_path().
If you grep for that function in libsvn_client, you'll see
that only commands accepting peg revisions use this function:
$ grep parse_path *c
blame-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
cat-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
checkout-cmd.c: SVN_ERR(svn_opt_parse_path(&pegrev, &local_dir, local_dir, pool));
checkout-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &true_url, repos_url,
copy-cmd.c: SVN_ERR(svn_opt_parse_path(peg_revision, &src, target, pool));
diff-cmd.c: SVN_ERR(svn_opt_parse_path(&opt_state->start_revision, &old_target,
diff-cmd.c: SVN_ERR(svn_opt_parse_path(&opt_state->end_revision, &new_target,
diff-cmd.c: SVN_ERR(svn_opt_parse_path(&old_rev, &old_target,
diff-cmd.c: SVN_ERR(svn_opt_parse_path(&new_rev, &new_target,
diff-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, path,
export-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truefrom, from, pool));
info-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target, subpool));
list-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
log-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &true_path, target, pool));
merge-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision1, &sourcepath1,
merge-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision2, &sourcepath2,
mergeinfo-cmd.c: SVN_ERR(svn_opt_parse_path(&src_peg_revision, &source,
mergeinfo-cmd.c: SVN_ERR(svn_opt_parse_path(&tgt_peg_revision, &target,
propget-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
proplist-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &truepath, target,
switch-cmd.c: SVN_ERR(svn_opt_parse_path(&peg_revision, &true_path, switch_url, pool));
Note that svn_opt_parse_path calls svn_opt__split_arg_at_peg_revision()
internally.
Now, why does "svn add" end up parsing peg revisions?
It should not be, because peg revs don't make sense when we're adding
a file.
Here is the trace:
#0 svn_opt__split_arg_at_peg_revision (true_target=0xbf80b80c,
peg_revision=0xbf80b808, utf8_target=0x89b73c0 "epsilon/@file",
pool=0x89ad9b0) at subversion/libsvn_subr/opt.c:857
#1 0x0806e724 in svn_client_args_to_target_array (targets_p=0xbf80b8d8,
os=0x89adb58, known_targets=0x0, ctx=0x89ae2e0, pool=0x89ad9b0)
at subversion/libsvn_client/cmdline.c:227
#2 0x080678dc in svn_cl__args_to_target_array_print_reserved (
targets=0xbf80b8d8, os=0x89adb58, known_targets=0x0, ctx=0x89ae2e0,
pool=0x89ad9b0) at subversion/svn/util.c:1102
#3 0x0804e7d8 in svn_cl__add (os=0x89adb58, baton=0xbf80ba04, pool=0x89ad9b0)
at subversion/svn/add-cmd.c:48
#4 0x0805ba02 in main (argc=3, argv=0xbf80bc34) at subversion/svn/main.c:2107
Looking into svn_client_args_to_target_array, we find this bit:
/*
* This is needed so that the target can be properly canonicalized,
* otherwise the canonicalization does not treat a ".@BASE" as a "."
* with a BASE peg revision, and it is not canonicalized to "@BASE".
* If any peg revision exists, it is appended to the final
* canonicalized path or URL. Do not use svn_opt_parse_path()
* because the resulting peg revision is a structure that would have
* to be converted back into a string. Converting from a string date
* to the apr_time_t field in the svn_opt_revision_value_t and back to
* a string would not necessarily preserve the exact bytes of the
* input date, so its easier just to keep it in string form.
*/
SVN_ERR(svn_opt__split_arg_at_peg_revision(&true_target, &peg_rev,
utf8_target, pool));
This is probably fine and all, except that it completely ignores the
fact that peg revision don't make sense everywhere. Instead of each
command which is able to cope with peg revs calling a function to
parse them, we now also unconditionally try to parse peg revs all
the time.
Which works fine until we wrongly interpret part of a filename as a peg
rev, and end up mangling the filename. Note that the problem described
in issue #3416 cannot be solved by tweaking the peg rev parser because
there is ambiguity.
The parser doesn't know that dir/@file is a directory "dir" containing
a file called "@file", and not a directory "dir" at peg revision "file".
And it should not need to, because its purpose is to detect the latter
case when asked to do so. We should not be running this parser when
we don't know whether we'll need a peg rev or not.
So I think we'll have to find a better way of dealing with
canonicalization in svn_client_args_to_target_array() somehow.
Comments? Suggestions?
Thanks,
Stefan