{"thread":{"id":"45742","subject":"[RFC PATCH] parse-options: disallow double-negations of options starting with no-","startedAt":"2017-04-19T09:08:36Z","lastAt":"2017-04-19T21:24:32Z","messageCount":9,"participants":["Jacob Keller","Ævar Arnfjörð Bjarmason","René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"317203","messageId":"20170419090820.20279-1-jacob.e.keller@intel.com","threadId":"45742","inReplyTo":null,"subject":"[RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-04-19T09:08:20Z","receivedAt":"2017-04-19T09:08:36Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nMany options can be negated by prefixing the option with \"no-\", for\nexample \"--3way\" can be prefixed with \"--no-3way\" to disable it. Since\n0f1930c58754 (\"parse-options: allow positivation of options\nstarting, with no-\", 2012-02-25) we have also had support to negate\noptions which start with \"no-\" by using the positive wording.\n\nThis leads to the confusing (and non-documented) case that you can still\nprefix options beginning with \"no-\" by a second \"no-\" to negate them.\nThat is, we allow \"no-no-hardlinks\" to negate the \"no-hardlinks\" option.\n\nThis can be confusing to the user so lets just disallow the\ndouble-negative forms. If the long_name begins with \"no-\" then we simply\ndon't allow the regular negation format, and only allow the option to be\nnegated by the positive form.\n\nSigned-off-by: Jacob Keller \n---\nI started going about implementing an OPT_NEGBOOL as suggested by Peff,\nbut realized this might just be simpler, and we already support the\npositive format for the negation, so we don't lose expressiveness. We\n*might* want to tie this to an option flag instead so that it only kicks\nin if the option specifically requests it. Thoughts?\n\n parse-options.c | 3 +++\n t/t0040-parse-options.sh | 5 ++++-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a23a1e67f04f..8e024c569f52 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -299,6 +299,9 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\t/* avoid double-negate on long_name */\n+\t\t\tif (starts_with(long_name, \"no-\"))\n+\t\t\t\tcontinue;\n \t\t\tflags |= OPT_UNSET;\n \t\t\tif (!skip_prefix(arg + 3, long_name, &rest)) {\n \t\t\t\t/* abbreviated and negated? */\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 74d2cd76fe56..abccfa5f265f 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -88,7 +88,6 @@ test_expect_success 'OPT_BOOL() is idempotent #1' 'check boolean: 1 --yes --yes'\n test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'\n \n test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'\n-test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'\n \n test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'\n test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'\n@@ -392,4 +391,8 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '\n \ttest-parse-options --expect=\"verbose: 0\" -v -v -v --no-verbose\n '\n \n+test_expect_success 'double negation not accepted' '\n+\ttest_must_fail test-parse-options --expect=\"boolean: 0\" --no-no-doubt\n+'\n+\n test_done\n-- \n2.12.2.882.g942b4cedeff1.dirty\n\n"},{"id":"317208","messageId":"CACBZZX5_fJ9w_K4H8R-NMi7kiR5QkR35pRqvzDRr_=rhMSQvfg@mail.gmail.com","threadId":"45742","inReplyTo":"20170419090820.20279-1-jacob.e.keller@intel.com","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-19T09:34:34Z","receivedAt":"2017-04-19T09:35:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Apr 19, 2017 at 11:08 AM, Jacob Keller wrote:\n> Many options can be negated by prefixing the option with \"no-\", for\n> example \"--3way\" can be prefixed with \"--no-3way\" to disable it. Since\n> 0f1930c58754 (\"parse-options: allow positivation of options\n> starting, with no-\", 2012-02-25) we have also had support to negate\n> options which start with \"no-\" by using the positive wording.\n>\n> This leads to the confusing (and non-documented) case that you can still\n> prefix options beginning with \"no-\" by a second \"no-\" to negate them.\n> That is, we allow \"no-no-hardlinks\" to negate the \"no-hardlinks\" option.\n>\n> This can be confusing to the user so lets just disallow the\n> double-negative forms. If the long_name begins with \"no-\" then we simply\n> don't allow the regular negation format, and only allow the option to be\n> negated by the positive form.\n\nLooks good to me. Addresses the bug I was trying to fix much better\nthan my patch.\n"},{"id":"317227","messageId":"68ed1250-534d-dc16-874e-ea14b592f8fd@web.de","threadId":"45742","inReplyTo":"20170419090820.20279-1-jacob.e.keller@intel.com","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-19T12:58:58Z","receivedAt":"2017-04-19T12:59:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.04.2017 um 11:08 schrieb Jacob Keller:\n> From: Jacob Keller \n> \n> Many options can be negated by prefixing the option with \"no-\", for\n> example \"--3way\" can be prefixed with \"--no-3way\" to disable it. Since\n> 0f1930c58754 (\"parse-options: allow positivation of options\n> starting, with no-\", 2012-02-25) we have also had support to negate\n> options which start with \"no-\" by using the positive wording.\n> \n> This leads to the confusing (and non-documented) case that you can still\n> prefix options beginning with \"no-\" by a second \"no-\" to negate them.\n> That is, we allow \"no-no-hardlinks\" to negate the \"no-hardlinks\" option.\n> \n> This can be confusing to the user so lets just disallow the\n> double-negative forms. If the long_name begins with \"no-\" then we simply\n> don't allow the regular negation format, and only allow the option to be\n> negated by the positive form.\n\nYour patch is a modernized version of my old one, so I'm fine with it.\n\nBut I wonder how --no-no-x being treated the same as --x can be \nconfusing. https://en.wikipedia.org/wiki/Double_negative explains it, I \nthink -- in some languages and dialects multiple negatives increase \nnegativity instead of cancelling themselves out pairwise. So users \nwould expect to get no x with --no-x and even less of it with --no-no-x?\n\n> Signed-off-by: Jacob Keller \n> ---\n> I started going about implementing an OPT_NEGBOOL as suggested by Peff,\n> but realized this might just be simpler, and we already support the\n> positive format for the negation, so we don't lose expressiveness. We\n> *might* want to tie this to an option flag instead so that it only kicks\n> in if the option specifically requests it. Thoughts?\n\nDo you mean that there should be a flag for allowing double negation? \nIn which situation would it be useful?\n\nOr do you mean that negation should be disabled by default and would \nhave to be enabled explicitly, unlike the current situation where it is \nenabled by default and can be turned off with PARSE_OPT_NONEG? That \ndepends on how often we'd want to disable negation, I guess. For \nboolean flags it probably makes sense to allow it by default.\n\n> parse-options.c | 3 +++\n> t/t0040-parse-options.sh | 5 ++++-\n> 2 files changed, 7 insertions(+), 1 deletion(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index a23a1e67f04f..8e024c569f52 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -299,6 +299,9 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n> \t\t\t\t}\n> \t\t\t\tcontinue;\n> \t\t\t}\n> +\t\t\t/* avoid double-negate on long_name */\n> +\t\t\tif (starts_with(long_name, \"no-\"))\n> +\t\t\t\tcontinue;\n> \t\t\tflags |= OPT_UNSET;\n> \t\t\tif (!skip_prefix(arg + 3, long_name, &rest)) {\n> \t\t\t\t/* abbreviated and negated? */\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index 74d2cd76fe56..abccfa5f265f 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -88,7 +88,6 @@ test_expect_success 'OPT_BOOL() is idempotent #1' 'check boolean: 1 --yes --yes'\n> test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'\n> \n> test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'\n> -test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'\n> \n> test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'\n> test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'\n> @@ -392,4 +391,8 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '\n> \ttest-parse-options --expect=\"verbose: 0\" -v -v -v --no-verbose\n> '\n> \n> +test_expect_success 'double negation not accepted' '\n> +\ttest_must_fail test-parse-options --expect=\"boolean: 0\" --no-no-doubt\n> +'\n> +\n> test_done\n\nUsing check_unknown_i18n like in the test for --no-no-fear would be \nshorter and more consistent.\n\nRené\n"},{"id":"317241","messageId":"20170419151005.wolvxiacjqxoqrq3@sigill.intra.peff.net","threadId":"45742","inReplyTo":"20170419090820.20279-1-jacob.e.keller@intel.com","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-19T15:10:05Z","receivedAt":"2017-04-19T15:10:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 19, 2017 at 02:08:20AM -0700, Jacob Keller wrote:\n\n> From: Jacob Keller \n> \n> Many options can be negated by prefixing the option with \"no-\", for\n> example \"--3way\" can be prefixed with \"--no-3way\" to disable it. Since\n> 0f1930c58754 (\"parse-options: allow positivation of options\n> starting, with no-\", 2012-02-25) we have also had support to negate\n> options which start with \"no-\" by using the positive wording.\n> \n> This leads to the confusing (and non-documented) case that you can still\n> prefix options beginning with \"no-\" by a second \"no-\" to negate them.\n> That is, we allow \"no-no-hardlinks\" to negate the \"no-hardlinks\" option.\n> \n> This can be confusing to the user so lets just disallow the\n> double-negative forms. If the long_name begins with \"no-\" then we simply\n> don't allow the regular negation format, and only allow the option to be\n> negated by the positive form.\n> \n> Signed-off-by: Jacob Keller \n> ---\n> I started going about implementing an OPT_NEGBOOL as suggested by Peff,\n> but realized this might just be simpler, and we already support the\n> positive format for the negation, so we don't lose expressiveness. We\n> *might* want to tie this to an option flag instead so that it only kicks\n> in if the option specifically requests it. Thoughts?\n\nYeah, if we are going to do anything, this is the right thing to do.\n\nI am on the fence on whether it actually needs addressing or not. Sure,\n--no-no-foo looks silly, but if the only way it happens is that the user\ntyped it, it doesn't seem so bad to me to respect it. I am tempted to\nsay we should support arbitrary levels of \"no-\" parsing as an easter\negg, but that is probably silly. :)\n\nSo I am fine with this patch, or without it.\n\n-Peff\n"},{"id":"317269","messageId":"CA+P7+xqo6BJ8LXy8EVvq0JXPD4r-jZ6boOGT=2XY8SVnWfVMcQ@mail.gmail.com","threadId":"45742","inReplyTo":"20170419151005.wolvxiacjqxoqrq3@sigill.intra.peff.net","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-19T20:54:06Z","receivedAt":"2017-04-19T20:54:39Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Apr 19, 2017 at 8:10 AM, Jeff King wrote:\n> On Wed, Apr 19, 2017 at 02:08:20AM -0700, Jacob Keller wrote:\n>\n>> From: Jacob Keller \n>>\n>> Many options can be negated by prefixing the option with \"no-\", for\n>> example \"--3way\" can be prefixed with \"--no-3way\" to disable it. Since\n>> 0f1930c58754 (\"parse-options: allow positivation of options\n>> starting, with no-\", 2012-02-25) we have also had support to negate\n>> options which start with \"no-\" by using the positive wording.\n>>\n>> This leads to the confusing (and non-documented) case that you can still\n>> prefix options beginning with \"no-\" by a second \"no-\" to negate them.\n>> That is, we allow \"no-no-hardlinks\" to negate the \"no-hardlinks\" option.\n>>\n>> This can be confusing to the user so lets just disallow the\n>> double-negative forms. If the long_name begins with \"no-\" then we simply\n>> don't allow the regular negation format, and only allow the option to be\n>> negated by the positive form.\n>>\n>> Signed-off-by: Jacob Keller \n>> ---\n>> I started going about implementing an OPT_NEGBOOL as suggested by Peff,\n>> but realized this might just be simpler, and we already support the\n>> positive format for the negation, so we don't lose expressiveness. We\n>> *might* want to tie this to an option flag instead so that it only kicks\n>> in if the option specifically requests it. Thoughts?\n>\n> Yeah, if we are going to do anything, this is the right thing to do.\n>\n> I am on the fence on whether it actually needs addressing or not. Sure,\n> --no-no-foo looks silly, but if the only way it happens is that the user\n> typed it, it doesn't seem so bad to me to respect it. I am tempted to\n> say we should support arbitrary levels of \"no-\" parsing as an easter\n> egg, but that is probably silly. :)\n>\n> So I am fine with this patch, or without it.\n>\n> -Peff\n\nThis is why it's an RFC. I don't really feel that it's too much of a\nproblem. As for the reason why I thought it might want a flag itself\nis because of concerns raised earlier that we might have something\nliek\n\nOPT_BOOL( ... \"no-stage\" ...)\nOPT_INT(... \"stage\" ....)\n\nor something already which might be broken and the only proper way to\ndisable no-stage is no-no-stage?\n\nIs this actually a concern? I thought this was a comment raised by you\nearlier as an objection to a change unless we specifically flagged\nthem as OPT_NEGBOOL()\n\nThanks,\nJake\n"},{"id":"317271","messageId":"20170419210028.sky24qda7kp4pl5y@sigill.intra.peff.net","threadId":"45742","inReplyTo":"CA+P7+xqo6BJ8LXy8EVvq0JXPD4r-jZ6boOGT=2XY8SVnWfVMcQ@mail.gmail.com","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-19T21:00:28Z","receivedAt":"2017-04-19T21:00:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 19, 2017 at 01:54:06PM -0700, Jacob Keller wrote:\n\n> This is why it's an RFC. I don't really feel that it's too much of a\n> problem. As for the reason why I thought it might want a flag itself\n> is because of concerns raised earlier that we might have something\n> liek\n> \n> OPT_BOOL( ... \"no-stage\" ...)\n> OPT_INT(... \"stage\" ....)\n> \n> or something already which might be broken and the only proper way to\n> disable no-stage is no-no-stage?\n> \n> Is this actually a concern? I thought this was a comment raised by you\n> earlier as an objection to a change unless we specifically flagged\n> them as OPT_NEGBOOL()\n\nI think the breakage in that case would be caused by \"--no-stage\" taking\nover \"--stage\" as well. And your patch doesn't change that; it happened\nalready in 2012.\n\nYour patch only affects the --no-no- form, which I think we would never\nwant. I don't think it needs callers to trigger it explicitly.\n\n-Peff\n"},{"id":"317272","messageId":"CA+P7+xpahRim9H1cHuHmzjGggYt2qbvxrDYyi+ri0QsDEUAkcw@mail.gmail.com","threadId":"45742","inReplyTo":"20170419210028.sky24qda7kp4pl5y@sigill.intra.peff.net","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-19T21:22:47Z","receivedAt":"2017-04-19T21:23:14Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Apr 19, 2017 at 2:00 PM, Jeff King wrote:\n> On Wed, Apr 19, 2017 at 01:54:06PM -0700, Jacob Keller wrote:\n>\n>> This is why it's an RFC. I don't really feel that it's too much of a\n>> problem. As for the reason why I thought it might want a flag itself\n>> is because of concerns raised earlier that we might have something\n>> liek\n>>\n>> OPT_BOOL( ... \"no-stage\" ...)\n>> OPT_INT(... \"stage\" ....)\n>>\n>> or something already which might be broken and the only proper way to\n>> disable no-stage is no-no-stage?\n>>\n>> Is this actually a concern? I thought this was a comment raised by you\n>> earlier as an objection to a change unless we specifically flagged\n>> them as OPT_NEGBOOL()\n>\n> I think the breakage in that case would be caused by \"--no-stage\" taking\n> over \"--stage\" as well. And your patch doesn't change that; it happened\n> already in 2012.\n>\n> Your patch only affects the --no-no- form, which I think we would never\n> want. I don't think it needs callers to trigger it explicitly.\n>\n> -Peff\n\nRight, I was just thinking in the weird cause were we *do* have a\n\"no-option\" that does want the \"no-no-option\" to negate it. Maybe this\nisn't ever a thing and we don't need to worry at all..?\n\nThanks,\nJake\n"},{"id":"317273","messageId":"CA+P7+xoMMPHwkaDiZWjD5gCoKh4iWGaVQ+206i8kS9aFM5zcEg@mail.gmail.com","threadId":"45742","inReplyTo":"CA+P7+xpahRim9H1cHuHmzjGggYt2qbvxrDYyi+ri0QsDEUAkcw@mail.gmail.com","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-19T21:23:43Z","receivedAt":"2017-04-19T21:24:11Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Apr 19, 2017 at 2:22 PM, Jacob Keller wrote:\n> On Wed, Apr 19, 2017 at 2:00 PM, Jeff King wrote:\n>> On Wed, Apr 19, 2017 at 01:54:06PM -0700, Jacob Keller wrote:\n>>\n>>> This is why it's an RFC. I don't really feel that it's too much of a\n>>> problem. As for the reason why I thought it might want a flag itself\n>>> is because of concerns raised earlier that we might have something\n>>> liek\n>>>\n>>> OPT_BOOL( ... \"no-stage\" ...)\n>>> OPT_INT(... \"stage\" ....)\n>>>\n>>> or something already which might be broken and the only proper way to\n>>> disable no-stage is no-no-stage?\n>>>\n>>> Is this actually a concern? I thought this was a comment raised by you\n>>> earlier as an objection to a change unless we specifically flagged\n>>> them as OPT_NEGBOOL()\n>>\n>> I think the breakage in that case would be caused by \"--no-stage\" taking\n>> over \"--stage\" as well. And your patch doesn't change that; it happened\n>> already in 2012.\n>>\n>> Your patch only affects the --no-no- form, which I think we would never\n>> want. I don't think it needs callers to trigger it explicitly.\n>>\n>> -Peff\n>\n> Right, I was just thinking in the weird cause were we *do* have a\n> \"no-option\" that does want the \"no-no-option\" to negate it. Maybe this\n> isn't ever a thing and we don't need to worry at all..?\n>\n> Thanks,\n> Jake\n\nAnd in this case the same PARSE_OPT_FLAG would be used to also not do\nthe \"no-option\" negates to \"option\" as well, since the options point\nwould be something liek \"this option starts with no- but negates\nnormally even though we don't normally allow that\"\n"},{"id":"317274","messageId":"20170419212423.i6jnu3q2npnbq2gz@sigill.intra.peff.net","threadId":"45742","inReplyTo":"CA+P7+xpahRim9H1cHuHmzjGggYt2qbvxrDYyi+ri0QsDEUAkcw@mail.gmail.com","subject":"Re: [RFC PATCH] parse-options: disallow double-negations of options starting with no-","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-19T21:24:23Z","receivedAt":"2017-04-19T21:24:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 19, 2017 at 02:22:47PM -0700, Jacob Keller wrote:\n\n> > I think the breakage in that case would be caused by \"--no-stage\" taking\n> > over \"--stage\" as well. And your patch doesn't change that; it happened\n> > already in 2012.\n> >\n> > Your patch only affects the --no-no- form, which I think we would never\n> > want. I don't think it needs callers to trigger it explicitly.\n> \n> Right, I was just thinking in the weird cause were we *do* have a\n> \"no-option\" that does want the \"no-no-option\" to negate it. Maybe this\n> isn't ever a thing and we don't need to worry at all..?\n\nYeah, my assumption is that we don't need to worry about that case.\n\n-Peff\n"}]}