subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: svn commit: r1846237 - /subversion/trunk/subversion/tests/cmdline/pegrev_parse_tests.py
Date Sun, 11 Nov 2018 09:54:37 GMT
Branko Čibej wrote on Sun, Nov 11, 2018 at 07:47:18 +0100:
> On 11.11.2018 07:29, Daniel Shahaf wrote:
> > Branko Čibej wrote on Sat, Nov 10, 2018 at 15:12:48 +0100:
> >> 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, brane@apache.org wrote:
> > The correct syntaxes for removing "./@" and "E/@", of course, are
> > «svn rm E/@@» and «svn rm @@» respectively.  
> >
> > I think that «svn rm E/@» should _not_ remove "E/@", for two reasons:
> 
> I didn't say it should. I said it should error out just like 'svn rm @'
> does.
> 

It wasn't clear to me what you were proposing.

It still isn't, actually.  Have I overlooked a commit or email where you
spelled out what the new algorithm would be?  None of the emails in this
thread spells out an algorithm.

> > Whatever we do, we must have a documented syntax that means "recursively
> > delete E/ and everything thereunder" and works regardless of what
> > filenames E/ contains.
> 
> Yes, it's called 'svn rm E'.

That won't work in the general case:

% /bin/mkdir foo@bar foo@bar@ foo@bar/@
% svn add . 
% svn ci
% <how do I 'svn rm' foo@bar now?>

Secondly, the incumbent escaping syntax is independent of the subcommand,
filename, and tree contents: to run «svn verb» on the file called
${foo} on disk, one runs «svn verb -- "${foo}@"», no exceptions.
I think this property should be preserved by the new escaping rules.
(Not necessarily this specific syntax, but the
independence/no-exceptions aspect.)

> > 2. In order to preserve the property that «svn ${subcommand} -- ${target}@»
> > is parsed in exactly the same way regardless of the values of
> > ${subcommand} and ${target} and regardless of the tree contents.
> 
> I do think that this rule was not sufficiently thought out.
> 
> > IIRC «svn up @» used to be the same as «svn up ./», so I think we
> > shouldn't make it mean "update the file './@'" because that would be
> > a silent incompatibility for some users.
> 
> Again, I didn't say it should. I said it should be an error because
> './@' is ambiguous.

As a point of fact, it is not ambiguous.  "./@" means {path_or_url="./",
peg=""}.

Terminology aside, the problem you have identified here is that if
a user has a file called "@" and forgets to escape it, he doesn't get
a syntax error.  That we can address.

> >   I don't remember what the last
> > minor line with the non-error meaning was, though.
> >
> >> Because otherwise the behaviour of the client depends on whether you're
> >> removing things from the current directory or not.
> > To be more precise, the question is whether one spells the command
> > «svn rm @» or «svn rm ./@».
> 
> or 'svn rm .@' or 'svn rm './.@'. I'd argue that premature
> canonicalization is a bad thing.
> 
> 

What change, if any, do you propose?

> >> 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.
> >>
> > I see that there's an argument that we should require some option here,
> > à la --force-log, but frankly, I don't think files literally called "@"
> > or "@HEAD" or "@{2018-01-01}" and so on are common enough to worry
> > about.
> 
> Perhaps not, but then there's SVN-4530, so I'd say they're common enough.

Fair enough.

> >> 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.
> > I don't follow what you mean by "this"; do you refer to forbidding
> > slashes in pegrevs or to something else?  (The former wouldn't seem to
> > address all the concerns you described.)
> >
> > Cheers,
> >
> > Daniel
> >
> > P.S.  Other languages also have examples of usage errors that aren't
> > syntax errors.  For example, in C the tokens «+», «-», and «*» can be
> > either unary or binary operators, so «double hypotenuse(double a, double b)
> > { return sqrt(*a + b*b); }» is a syntax error but «return sqrt( + b*b)»
> > is not.
> 
> I really don't see how this comparison is valid. C operator syntax and
> precedence is precisely defined. Our usage of @ in paths may be
> well-defined too, but its interaction with aggressive canonicalization
> creates unintentional side effects in the implementation.

The relevance of the example is that it's another case in which a usage
error (programmer forgot to paste «a*a» after the opening parenthesis) 
is not a syntax error, in relation to your observation that using «svn
rm E/@» to remove "E/@" isn't a syntax error.

Mime
View raw message