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_at_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 "._at_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
Received on 2009-05-29 20:02:54 CEST