From dev-return-38547-archive-asf-public=cust-asf.ponee.io@subversion.apache.org Sun Nov 11 07:47:23 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 3AD58180645 for ; Sun, 11 Nov 2018 07:47:23 +0100 (CET) Received: (qmail 86859 invoked by uid 500); 11 Nov 2018 06:47:22 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 86849 invoked by uid 99); 11 Nov 2018 06:47:22 -0000 Received: from mail-relay.apache.org (HELO mailrelay2-lw-us.apache.org) (207.244.88.137) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 11 Nov 2018 06:47:22 +0000 Received: from [172.17.13.7] (unknown [77.234.149.122]) by mailrelay2-lw-us.apache.org (ASF Mail Server at mailrelay2-lw-us.apache.org) with ESMTPSA id DA49C20E8 for ; Sun, 11 Nov 2018 06:47:20 +0000 (UTC) Subject: Re: svn commit: r1846237 - /subversion/trunk/subversion/tests/cmdline/pegrev_parse_tests.py To: dev@subversion.apache.org References: <20181109115422.8B7093A006D@svn01-us-west.apache.org> <1541809913.2973178.1571960568.0D14F931@webmail.messagingengine.com> <885c7b33-8bf7-c7fa-cacd-19674f96310b@apache.org> <20181111062930.fuyigxvdtyvbl73t@tarpaulin.shahaf.local2> From: =?UTF-8?Q?Branko_=c4=8cibej?= Openpgp: preference=signencrypt Autocrypt: addr=brane@apache.org; prefer-encrypt=mutual; keydata= xsFNBFG3qpMBEACi+jRQDd2TiYeAxVgrLZ3cyyuGOFSMh4nCyUOG9BwXC69cDLH48RcE0Mpu TFTGlfdokz6JgLKU3uqShPXiflrL6JIVnJX4rTEKRzFNkcS6Zq0PfNRnFnkwiD2KIzyAG8XE y0c1Bt7hqZ5dfXaC1b7Xo+1cnlqjdLAOnr1ruTrtfQ5sO81p9jYtARVa+iVmf8bs/FvC9Yn2 QtEDtuUfUUHx2bnB9vmh8tOjErfIcWtzCPt8uTUkmiszlkRMiB5/X97oqXlX/5dSQWE9m4M5 6Fc9ixIrmCwkF515RLrCNTv/YAtmpu4VaB0rxgTuSku0cVk83xSMrH2hNFx1fAeYBZpwp2GL ONlTy3D2N+BjWXjEUE9baGOoYM7QUbAdj4JMstSByppaAi4AiG9+raxknTWtWt2IT9LHW7Pu i6S3k4WL5jmTdQKqNQ9/+vRqiSVsA98yHQLa+s19IYh4F7WIfo2lzBAn06HEntpKS9TtV20o JyMBLOVqQP1dARWRfB0xIxGtbI61CfjEhCeG8H+UynCrHkUxgUoKsXXkI/JxsIMZ3TivFj3U MJVur7KVwg/isqqaEyMfUnCrXJxexZp8kuTjkzzvDKfYs0vHJezPQYhlqBLkK2w9VzktGjA7 lb+TO69bEyPOcBjVsCtrdYVc442/Z37G+1UV5+1X06m14Pt9UQARAQABzSBCcmFua28gxIxp YmVqIDxicmFuZUBhcGFjaGUub3JnPsLBegQTAQoAJAIbAwULCQgHAwUVCgkICwUWAgMBAAIe AQIXgAUCUbetMwIZAQAKCRAbymWGo0eUP2tOD/9KOLYfxwTcGV/Nj3lnKE4Y4gRl0r4cfnWm 1/2KyPYVsmQ8vWRUZxjuVHAvZrAkTBvlu+CVzrCWEEpCzQC/jki0xkPQchTEU2XOHQ6PzkXB 17o1NSSu/vyKynh0pXMRTHm4wZodzUw/tHn/Ism5QyRyhlYUP4mVX8v2hbN+stkJHrkdVBPm FspnFidhulUP5hr+LWz2qd+Ab8MOn3+x25jsGE8yaUiqmNdrmq/trvHPGThySa4Hz0uEkhfP K2knc6PpV5GTbeRn/J1eu17xVgXYVgko35Qwz5s/LRat+5R79tgBAL9SKFybCVBPr6/1Zp4u w9b0NcHW6t3aQHCxv8iEqxrJ7UIDhh/hXh4no0vzpPR1Cgjn6fK997WrpUyaAtlnbSH5QGad YY9rpFka3o2Gj+f+cr75hq6c7DnNJo94eGw9L0JEjfgordi8UkWErGOklnGf8N8brlVG0TdW 7KOz60m1E3UzIwd2lQd9a0zd8Mqrmn2MMPdJt4EpKQWaJsoK+FOdEBX0Ezm3StEXufe1IOG9 DihVcOnsx/G6aTS9GyKjURVt0jDB4wsVSzsRHYHmQpw7/ekvHFNKZS5yMNwSt2X/Szmk4GmV 69gaI79kf8VD87xwE31p0s0uVIVp7MTOTEYT5HUh5Rz6Rr66+vg9qgN1enMj5sh4f8krXgRR wM7BTQRRt6qTARAAnxIdGqDTC2FU9AE2ElT/m/Hs/57BwqUUb8qod3mJ6Qkp7PpHCBnvtbwm krrCsJl5rR1fliton6qoJUNCSfmcfeujcU8Be+q75rNZxIWi6AjMmyrjyMp9JIO7g/7+VYmL dm9c1wRn4QDnIKxl7qMPz9q8/OF6BGEMEW4zRL8rHvM7CCapOikHUKKq7GnZMVyYbue6KUTA Tczxjt6E9Av1QDnnW9zbW56jqUKdgpNek/bSTuef2xYEDzIzFPQREyw8E/C3xx8zZfOJ0+XV s1n39GLp3vugP5IBNE2pgqcyFtKISj1pVJgDr7zXjD92ZGS8xgqDxePTuf1LcCwd65BJNVVK IFsFicvBVhdslCZ7l8jkCuZAzYoFJZthUKuuJg1n7HYi8XLifZmun9Z3fbM5gk9/vA1rXsWt An597BACKDUkWA5tOb3Si4/MaRDiZYvzplHGc4sTn4aBIj3VFGGFNlOUPFLWjZLHdudNOBGj 3eIlz/DQZh/mwNGn5g98c3xehHnWxcXa0PsN2Xl1iRM2dec8drEVVRYaWPcOmGhKfqnlwl2z OeuST2TMcWhxKshVimR9eSt5pX1oGOD9PZ9V0gQDIr4d35UjQaW5ABCWbgTd7e3yPTlHoWx4 qyv+YoxEf6AlQ4nvE+q1s4wRBs/eNVQsROnYmhKhYPZUsDE6EocAEQEAAcLBXwQYAQoACQUC UbeqkwIbDAAKCRAbymWGo0eUPwd6D/92i5LBHSluiBdnzYH3kYlkIMjhy3lcqtxb/TWV1X/z CVpaZkEXvL9NQ44ZqfiOFB8fnaJvy+9rfIL3MwHKLVHOjsurBRP2DJ8H/EI6QuZV//Nxh66A dicXlE5SSiKQ5KcIH+eqZHa4XjVeXGeNZummrlhOv3ItKXETVhh2qeIQ/7zCjuw5rQk606+2 isg6cs4Nwtie1rXQ1KFtkTNQqWfqyM4PrEP9Bq5pWBQVkcxDsxk1Yj3A8L80IY3Hzwm8nRlq F+HkD/0IPgHICVDyiOB4XZtqVk+DHNOolCcdrFSXOcwt+qwD5zk4p0hdHKHagAPGBDXS8shm k2vaUDbKMUoVDdj579Jtp4tNOoVEEqqXspT995w7+ckbHGoQhFlSxCwtaXCr/8wwdwcCA2eO w0aLYrU04EbnH7Ryj4aTjsBGvJdmyZQT8/lTj5VARbEkNXTdTOs61pebDliyWtcF9Uz9b44p cLNniphcBO4SP/IMlEh8pBAJ1C2QlD4G90iJ1WK0MsJsUDix9Vb5s1AE6WA/Ss1iPCOdhhif eToCAwoobIipoxUZF2ik3oESskmMDolpVBiaPaFg+YPtNp/53dLap7jBNRNgyKXaGJAZaolp L+9hCU1EOWswqusDHDFSRUuYOXfuXZJxcbQUTnhQhRbvSDy3tDMRGd252Ur1sCOU5g== Organization: The Apache Software Foundation Message-ID: <137c05e0-9573-f390-b287-6b7d6d6b027d@apache.org> Date: Sun, 11 Nov 2018 07:47:18 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181111062930.fuyigxvdtyvbl73t@tarpaulin.shahaf.local2> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-GB 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: >>>>> 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 '@'? > The argument parsing is documented to work as follows: remove the last > "@" and everything after it — that's the peg rev. What remains is the > target dirent or URL. Thus, ultimately the reason for the different > errors is that "E/" is a valid OS-level path specification but "" is not > (in the sense that «stat("E/")» works and «stat("")» errors). > > In other words, your examples are simply usage errors, but it so happens > that one of them is a syntax error and one of them is not. (It would > have been good if both usage errors had been syntax errors, but we're > 12 years too late to get that right to begin with. C'est la vie.) Life can also be fixed to suck less. > 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. > 1. Because that would be backwards incompatible. Obviouisly. > 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. > 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. >> 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. > 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'. >> 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. >> > So you're proposing changing the parsing rule from "the last @" to > "the last @ that isn't followed by any os.path.sep"? That's forward > compatible if we constrain ourselves never to use slash or backslash in > a pegrev specifier, and backwards compatible in that every non-erroneous > usage will continue to work, so it's something we can consider. > > How would that affect scheme://user@hostname/ URLs? I haven't tested that yet. > What about typos such as «foo@HEA/D»? This won't work currently, but you're right, it _could_ work with the proposed change. So my assumption that we'd only add more errors is wrong. >> 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. -- Brane