{"thread":{"id":"60086","subject":"[PATCH] parse-options: disallow negating OPTION_SET_INT 0","startedAt":"2023-08-08T20:37:55Z","lastAt":"2023-08-08T23:52:53Z","messageCount":2,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480331","messageId":"c4cd1591-3a83-920a-6a80-19ffbfe3089d@web.de","threadId":"60086","inReplyTo":null,"subject":"[PATCH] parse-options: disallow negating OPTION_SET_INT 0","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-08-08T20:05:57Z","receivedAt":"2023-08-08T20:37:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"An option of type OPTION_SET_INT can be defined to set its variable to\nzero.  It's negated variant will do the same, though, which is\nconfusing.  Several such options were fixed by disabling negation,\nchanging the value to set or using a different option type:\n\n991c552916 (ls-tree: fix --no-full-name, 2023-07-18)\ne12cb98e1e (branch: reject \"--no-all\" and \"--no-remotes\" early, 2023-07-18)\n68cbb20e73 (show-branch: reject --[no-](topo|date)-order, 2023-07-19)\n3821eb6c3d (reset: reject --no-(mixed|soft|hard|merge|keep) option, 2023-07-19)\n36f76d2a25 (pack-objects: fix --no-quiet, 2023-07-21)\n3a5f308741 (pack-objects: fix --no-keep-true-parents, 2023-07-21)\nc95ae3ff9c (describe: fix --no-exact-match, 2023-07-21)\nd089a06421 (bundle: use OPT_PASSTHRU_ARGV, 2023-07-29)\n\nCheck for such options that allow negation in parse_options_check() and\nreport them to find future cases quicker.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n parse-options.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 87c9fae634..60224cf8d0 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -480,6 +480,9 @@ static void parse_options_check(const struct option *opts)\n \t\t     opts->long_name))\n \t\t\toptbug(opts, \"uses feature \"\n \t\t\t       \"not supported for dashless options\");\n+\t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n+\t\t    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n+\t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n \t\tswitch (opts->type) {\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_BIT:\n--\n2.41.0\n"},{"id":"480344","messageId":"xmqqh6p96qie.fsf@gitster.g","threadId":"60086","inReplyTo":"c4cd1591-3a83-920a-6a80-19ffbfe3089d@web.de","subject":"Re: [PATCH] parse-options: disallow negating OPTION_SET_INT 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-08T23:52:41Z","receivedAt":"2023-08-08T23:52:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> An option of type OPTION_SET_INT can be defined to set its variable to\n> zero.  It's negated variant will do the same, though, which is\n> confusing.  Several such options were fixed by disabling negation,\n> changing the value to set or using a different option type:\n>\n> 991c552916 (ls-tree: fix --no-full-name, 2023-07-18)\n> e12cb98e1e (branch: reject \"--no-all\" and \"--no-remotes\" early, 2023-07-18)\n> 68cbb20e73 (show-branch: reject --[no-](topo|date)-order, 2023-07-19)\n> 3821eb6c3d (reset: reject --no-(mixed|soft|hard|merge|keep) option, 2023-07-19)\n> 36f76d2a25 (pack-objects: fix --no-quiet, 2023-07-21)\n> 3a5f308741 (pack-objects: fix --no-keep-true-parents, 2023-07-21)\n> c95ae3ff9c (describe: fix --no-exact-match, 2023-07-21)\n> d089a06421 (bundle: use OPT_PASSTHRU_ARGV, 2023-07-29)\n>\n> Check for such options that allow negation in parse_options_check() and\n> report them to find future cases quicker.\n\nThat does make quite a lot of sense, and parse_options_check() is\nthe perfect place to do so.\n\nThanks.\n\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  parse-options.c | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 87c9fae634..60224cf8d0 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -480,6 +480,9 @@ static void parse_options_check(const struct option *opts)\n>  \t\t     opts->long_name))\n>  \t\t\toptbug(opts, \"uses feature \"\n>  \t\t\t       \"not supported for dashless options\");\n> +\t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n> +\t\t    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n> +\t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n>  \t\tswitch (opts->type) {\n>  \t\tcase OPTION_COUNTUP:\n>  \t\tcase OPTION_BIT:\n> --\n> 2.41.0\n"}]}