On 10.11.2018 01:31, Daniel Shahaf wrote: > Branko Čibej wrote on Fri, 09 Nov 2018 12:56 +0100: >> On 09.11.2018 12:54, br...@apache.org wrote: >>> Author: brane >>> Date: Fri Nov 9 11:54:21 2018 >>> New Revision: 1846237 >>> >>> URL: http://svn.apache.org/viewvc?rev=1846237&view=rev >>> Log: >>> More tests for peg revision parsing. >> [...] >>> +@Wimp("Removes E instead of reporting ENOENT or E125001 for E/@") >>> +def remove_subdir_7a_no_escape_peg(sbox): >>> + "remove missing 'E/@' without pegrev escape" >>> + do_remove(sbox, 'E/@', 'E/@') #, "svn: E125001: 'E/@'") >> Yes indeed ... our target parsing is so incredibly reliable that >> attempting to delete a non-existent 'E/@' will happily delete 'E' with >> all its contents. I think this may be too much of a good thing. > The documented escaping rule for CLI consumers is to append an "@" after > every filename. By this rule, a CLI consumer that _wants_ to remove E/ > would run "svn rm E/@". That command should parse as { target = 'E/', > peg = '' } regardless of whether the directory "E" contains a child > named "@". > > I don't understand what change you're proposing.
Consider this: $ svn rm E@ D E $ svn rm @ svn: E125001: '@' is just a peg revision. Maybe try '@@' instead? $ svn rm E/@ D E would it not be more consistent that trying to remove 'E/@' would raise the same error as trying to remove '@'? Because otherwise the behaviour of the client depends on whether you're removing things from the current directory or not. This becomes especially error-prone in the case when E/@ exists: $ svn rm E/@ D E D E/@ I'm pretty sure we should not remove a parent directory if the child might have been the intended target. These usages were not ambiguous and possibly error-prone until we invented the peg-revision syntax. At the time we failed to strictly define the semantics , so maybe it's time we did that now. For example, given the path foo@bar/baz the parser will treat 'bar/baz' as the peg-revision specifier, even though it's obvious that's not what the user had in mind. If we changed the parser to only look for peg revisions on the basename of the path, then at least the only restriction we'll have is that directory separators can't be part of the peg revision specifier — which should be an acceptable restriction, since currently we only support known keywords, revision numbers or dates, none of which contain forward- or backslashes. Obviously that would only be a parser change, peg revisions would still apply to whole paths for command semantics. I'm aware that doing this would change the meaning of some forms of commands, but for now I'm fairly sure we'd only produce new errors, not silently behave differently. From the current set of tests, it seems to follow that this inconsistency only arises with targets whose basename starts with '@' or '.@' (and probably '..@'), the latter because '.' (and '..') are removed by canonicalization. -- Brane
