On 29.10.2018 19:08, Daniel Shahaf wrote:
> Good morning Brane,
>
> brane_at_apache.org wrote on Sun, 28 Oct 2018 10:40 +0000:
>> Add XFAIL tests for issue #4530.
>>
>> +++ subversion/trunk/subversion/tests/cmdline/basic_tests.py Sun Oct 28 10:40:04 2018
>> @@ -3050,6 +3050,33 @@ def peg_rev_on_non_existent_wc_path(sbox
>> +def do_move_with_at_signs(sbox, src, dst, dst_cmdline):
>> + sbox.build()
>> +
>> + expected_status = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
>> + expected_status.tweak(src, status='D ', moved_to=dst)
>> + expected_status.add({dst: Item(status='A ', copied='+',
>> + moved_from=src, wc_rev='-')})
>> +
>> + sbox.simple_move(src, dst_cmdline)
>> + svntest.actions.run_and_verify_status(sbox.wc_dir, expected_status)
>> +
>> +@Issue(4530)
>> +@XFail()
>> +def move_to_target_with_leading_at_sign(sbox):
>> + "rename to dir/@file"
>> +
>> + do_move_with_at_signs(sbox, 'iota', 'A/@upsilon', 'A/@upsilon')
>> +
>> +
>> +@Issue(4530)
>> +@XFail()
>> +def move_to_target_with_leading_and_trailing_at_sign(sbox):
>> + "rename to dir/@file@"
>> +
>> + do_move_with_at_signs(sbox, 'iota', 'A/@upsilon', 'A/@upsilon@')
> I'm not actually sure these are supposed to work as these tests expect:
> supporting A/@upsilon as the first argument like this would prevent us
> from adding "foo_at_upsilon" as a command-line syntax for '"foo" at peg
> revision svn_opt_revision_working' in a future minor release. I think
> this line of thought applies to the second argument as well.
You must have misread what the test does. A/@upsilon (and A/@upsilon@)
are the _second_ arguments to 'svn rename'. They're given twice because
the first is the expected name in the status tree and the second is what
we give SVN on the command line.
The command tested is one of:
* 'svn rename iota A/@upsilon' (whch creates A_at_upsilon, which is wrong)
* 'svn rename iota A/@upsilont@' (which creates A/@upsilon@ which is
also wrong)
So there's no way to rename iota to A/@upsilon. We effectively forbd
renames to fiels with leading @-signs, unless one jumps through a lot of
hoops. Consider this case, trying to do the rename in the same directory:
$ svn mv @bar@ @qux
svn: E125001: '@qux' is just a peg revision. Maybe try '@qux@' instead?
$ svn mv @bar@ @qux@
A @qux@
D @bar
Notice how the code complains about bare '@qux' but doesn't about
'foo/@qux'. There are so many bugs in those few lines of code that I've
not even counted them all yet.
[[
Addendum:
Why is renaming to A/@upsilon@ in the second test case wrong? because a)
it gives you no way at all to rename to A/@upsilon, and b) an equivalent
'svn add A/@upsilon@' will add A/@upsilon (which must exist of course),
and so will an equivalent 'svn mkdir', where the target directory does
_not_ exist:
$ svn mkdir foo/@x@
A foo/@x
$ svn mkdir foo/@x
-svn: E200009: 'foo_at_x': a peg revision is not allowed here
]]
> It would probably be good to review the list of intended changes before
> time is spent implementing them. Is there such a list? (I don't recall
> seeing one on the issue.)
I'm not implementing any changes, for now I just added tests that
demonstrate the existing bug as reported. That it is a bug should be
obvious and I hope it doesn't need a lot of discussion. I'm really not
that interested in discussing, now, after the fact, how the peg revision
syntax should really work.
> I can see a number of options:
>
> - All PATH arguments (and all TARGET formal arguments when the actual
> argument isn't a URI) are parsed for peg revisions, no exceptions.
They seem to be, but edge cases are handled incorrectly.
> - Ditto, except when the argument denotes an unversioned un-disk path
> (like the targets of «svn add», the file passed as argument to the
> «--targets» option, and so on).
We could never do this in a backwards-compatible way, because all those
arguments are currently processed in the same way.
> - Ditto, plus something that excludes the second target argument to
> «svn move». (I'm not sure about how exactly to do this because the syntax
> is so ambiguous: «svn move file1 file2», «svn move baz foo/bar» where
> «foo/» exists and bar doesn't, and «svn move baz foo/bar» where
> «foo/bar/» is a directory, are all valid.)
>
> I'm sure there are more options, but my point is, let's agree on what
> the CLI documentation on escaping should say in 1.12.
We have always said, "add @ to the end of the name if there's an @ in
the name," no more and no less. I can live with that because it's a
simple and straight-forward rule, *provided* it's actually implemented
correctly. Adding exceptions for move or add or --targets seems like a
cure worse than the disease. It's true that 'svn rename' seems to have
an unintentional exception, but that exception is caused by a bug, it's
not by design.
We really should have created an svn_opt_target_t struct that contained
the path an peg revision as separate members instead of messing around
with splitting, mogrifying and concatenating strings in five different
places. Well, it's a bit late in the day for that.
-- Brane
Received on 2018-10-30 01:04:38 CET