{"thread":{"id":"29737","subject":"[PATCH 2/3] parse-options: allow positivation of options starting, with no-","startedAt":"2012-02-25T19:07:38Z","lastAt":"2012-02-29T19:02:57Z","messageCount":19,"participants":["René Scharfe","Junio C Hamano","Thomas Rast","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"185439","messageId":"4F49317A.3080809@lsrfire.ath.cx","threadId":"29737","inReplyTo":null,"subject":"[PATCH 0/3] parse-options: no- symmetry","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-25T19:07:38Z","receivedAt":"2012-02-25T19:07:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Boolean long options can be negated by adding a \"no-\" at the\nbeginning, unless the flag PARSE_OPT_NONEG is set.  There are\nseveral options defined to start out with \"no-\" already (e.g.\nformat-patch --no-numbered).  Their negation with a second\n\"no-\" looks a bit strange.\n\nThe flag PARSE_OPT_NEGHELP was introduced to avoid this\nawkwardness.  It is used twice (in fast-export and grep) to\ndefine option pairs (--data/--no-data and --index/--no-index)\nwhose negative part is shown in the help text.\n\nHowever, PARSE_OPT_NEGHELP is strange as well.  Short options\ndefined with it do the opposite of what the help text says\n(we currently don't have any).  And the multiple negations\nare confusing.\n\nThis series adds the ability for users to negate options that\nalready start with \"no-\" by simply leaving it out.  The last\npatch removes PARSE_OPT_NEGHELP as it isn't needed anymore at\nthat point.\n\n  test-parse-options: convert to OPT_BOOL()\n  parse-options: allow positivation of options starting with no-\n  parse-options: remove PARSE_OPT_NEGHELP\n\n Documentation/technical/api-parse-options.txt |    3 +-\n builtin/fast-export.c                         |    4 +-\n builtin/grep.c                                |   15 +++----\n parse-options.c                               |   33 ++++++++------\n parse-options.h                               |    4 --\n t/t0040-parse-options.sh                      |   60 ++++++++++++++++++++++++-\n test-parse-options.c                          |   12 +++--\n 7 files changed, 95 insertions(+), 36 deletions(-)\n\n-- \n1.7.9.2\n"},{"id":"185438","messageId":"4F493254.1020801@lsrfire.ath.cx","threadId":"29737","inReplyTo":"4F49317A.3080809@lsrfire.ath.cx","subject":"[PATCH 1/3] test-parse-options: convert to OPT_BOOL()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-25T19:11:16Z","receivedAt":"2012-02-25T19:11:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Introduce OPT_BOOL() to test-parse-options and add some tests for\nthese \"true\" boolean options. Rename OPT_BOOLEAN to OPT_COUNTUP and\nOPTION_BOOLEAN to OPTION_COUNTUP as well.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n t/t0040-parse-options.sh |   60 ++++++++++++++++++++++++++++++++++++++++++++--\n test-parse-options.c     |   12 ++++++----\n 2 files changed, 66 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex a1e4616..79aefe2 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -10,7 +10,10 @@ test_description='our own option parser'\n cat > expect << EOF\n usage: test-parse-options <options>\n \n-    -b, --boolean         get a boolean\n+    --yes                 get a boolean\n+    -D, --no-doubt        begins with 'no-'\n+    -B, --no-fear         be brave\n+    -b, --boolean         increment by one\n     -4, --or4             bitwise-or boolean with ...0100\n     --neg-or4             same as --no-or4\n \n@@ -53,6 +56,59 @@ test_expect_success 'test help' '\n \n mv expect expect.err\n \n+cat >expect.template <<EOF\n+boolean: 0\n+integer: 0\n+timestamp: 0\n+string: (not set)\n+abbrev: 7\n+verbose: 0\n+quiet: no\n+dry run: no\n+file: (not set)\n+EOF\n+\n+check() {\n+\twhat=\"$1\" &&\n+\tshift &&\n+\texpect=\"$1\" &&\n+\tshift &&\n+\tsed \"s/^$what .*/$what $expect/\" <expect.template >expect &&\n+\ttest-parse-options $* >output 2>output.err &&\n+\ttest ! -s output.err &&\n+\ttest_cmp expect output\n+}\n+\n+check_unknown() {\n+\tcase \"$1\" in\n+\t--*)\n+\t\techo error: unknown option \\`${1#--}\\' >expect ;;\n+\t-*)\n+\t\techo error: unknown switch \\`${1#-}\\' >expect ;;\n+\tesac &&\n+\tcat expect.err >>expect &&\n+\ttest_must_fail test-parse-options $* >output 2>output.err &&\n+\ttest ! -s output &&\n+\ttest_cmp expect output.err\n+}\n+\n+test_expect_success 'OPT_BOOL() #1' 'check boolean: 1 --yes'\n+test_expect_success 'OPT_BOOL() #2' 'check boolean: 1 --no-doubt'\n+test_expect_success 'OPT_BOOL() #3' 'check boolean: 1 -D'\n+test_expect_success 'OPT_BOOL() #4' 'check boolean: 1 --no-fear'\n+test_expect_success 'OPT_BOOL() #5' 'check boolean: 1 -B'\n+\n+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 --fear'\n+test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'\n+\n+test_expect_failure 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n+\n cat > expect << EOF\n boolean: 2\n integer: 1729\n@@ -296,7 +352,7 @@ test_expect_success 'OPT_NEGBIT() works' '\n \ttest_cmp expect output\n '\n \n-test_expect_success 'OPT_BOOLEAN() with PARSE_OPT_NODASH works' '\n+test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n \ttest-parse-options + + + + + + > output 2> output.err &&\n \ttest ! -s output.err &&\n \ttest_cmp expect output\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 36487c4..3c9510a 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -37,7 +37,11 @@ int main(int argc, const char **argv)\n \t\tNULL\n \t};\n \tstruct option options[] = {\n-\t\tOPT_BOOLEAN('b', \"boolean\", &boolean, \"get a boolean\"),\n+\t\tOPT_BOOL(0, \"yes\", &boolean, \"get a boolean\"),\n+\t\tOPT_BOOL('D', \"no-doubt\", &boolean, \"begins with 'no-'\"),\n+\t\t{ OPTION_SET_INT, 'B', \"no-fear\", &boolean, NULL,\n+\t\t  \"be brave\", PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },\n+\t\tOPT_COUNTUP('b', \"boolean\", &boolean, \"increment by one\"),\n \t\tOPT_BIT('4', \"or4\", &boolean,\n \t\t\t\"bitwise-or boolean with ...0100\", 4),\n \t\tOPT_NEGBIT(0, \"neg-or4\", &boolean, \"same as --no-or4\", 4),\n@@ -62,11 +66,11 @@ int main(int argc, const char **argv)\n \t\tOPT_ARGUMENT(\"quux\", \"means --quux\"),\n \t\tOPT_NUMBER_CALLBACK(&integer, \"set integer to NUM\",\n \t\t\tnumber_callback),\n-\t\t{ OPTION_BOOLEAN, '+', NULL, &boolean, NULL, \"same as -b\",\n+\t\t{ OPTION_COUNTUP, '+', NULL, &boolean, NULL, \"same as -b\",\n \t\t  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH },\n-\t\t{ OPTION_BOOLEAN, 0, \"ambiguous\", &ambiguous, NULL,\n+\t\t{ OPTION_COUNTUP, 0, \"ambiguous\", &ambiguous, NULL,\n \t\t  \"positive ambiguity\", PARSE_OPT_NOARG | PARSE_OPT_NONEG },\n-\t\t{ OPTION_BOOLEAN, 0, \"no-ambiguous\", &ambiguous, NULL,\n+\t\t{ OPTION_COUNTUP, 0, \"no-ambiguous\", &ambiguous, NULL,\n \t\t  \"negative ambiguity\", PARSE_OPT_NOARG | PARSE_OPT_NONEG },\n \t\tOPT_GROUP(\"Standard options\"),\n \t\tOPT__ABBREV(&abbrev),\n-- \n1.7.9.2\n"},{"id":"185436","messageId":"4F49332E.7070003@lsrfire.ath.cx","threadId":"29737","inReplyTo":"4F49317A.3080809@lsrfire.ath.cx","subject":"[PATCH 2/3] parse-options: allow positivation of options starting, with no-","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-25T19:14:54Z","receivedAt":"2012-02-25T19:14:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Long options can be negated by adding no- right after the leading\ntwo dashes. This is useful e.g. to override options set by aliases.\n\nFor options that are defined to start with no- already, this looks\na bit funny. Allow such options to also be negated by removing the\nprefix.\n\nThe following thirteen options are affected:\n\n\tapply          --no-add\n\tbisect--helper --no-checkout\n\tcheckout-index --no-create\n\tclone          --no-checkout --no-hardlinks\n\tcommit         --no-verify   --no-post-rewrite\n\tformat-patch   --no-binary\n\thash-object    --no-filters\n\tread-tree      --no-sparse-checkout\n\trevert         --no-commit\n\tshow-branch    --no-name\n\tupdate-ref     --no-deref\n\nThe following five are NOT affected because they are defined with\nPARSE_OPT_NONEG or the non-negated version is defined as well:\n\n\tbranch       --no-merged\n\tformat-patch --no-stat             --no-numbered\n\tupdate-index --no-assume-unchanged --no-skip-worktree\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n Documentation/technical/api-parse-options.txt |    3 ++-\n parse-options.c                               |   27 ++++++++++++++++---------\n t/t0040-parse-options.sh                      |    2 +-\n 3 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 4b92514..2527b7e 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -39,7 +39,8 @@ The parse-options API allows:\n * Short options may be bundled, e.g. `-a -b` can be specified as `-ab`.\n \n * Boolean long options can be 'negated' (or 'unset') by prepending\n-  `no-`, e.g. `\\--no-abbrev` instead of `\\--abbrev`.\n+  `no-`, e.g. `\\--no-abbrev` instead of `\\--abbrev`. Conversely,\n+  options that begin with `no-` can be 'negated' by removing it.\n \n * Options and non-option arguments can clearly be separated using the `\\--`\n   option, e.g. `-a -b \\--option \\-- \\--this-is-a-file` indicates that\ndiff --git a/parse-options.c b/parse-options.c\nindex f0098eb..8906841 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -193,13 +193,14 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\targ_end = arg + strlen(arg);\n \n \tfor (; options->type != OPTION_END; options++) {\n-\t\tconst char *rest;\n-\t\tint flags = 0;\n+\t\tconst char *rest, *long_name = options->long_name;\n+\t\tint flags = 0, opt_flags = 0;\n \n-\t\tif (!options->long_name)\n+\t\tif (!long_name)\n \t\t\tcontinue;\n \n-\t\trest = skip_prefix(arg, options->long_name);\n+again:\n+\t\trest = skip_prefix(arg, long_name);\n \t\tif (options->type == OPTION_ARGUMENT) {\n \t\t\tif (!rest)\n \t\t\t\tcontinue;\n@@ -212,7 +213,7 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\t}\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n-\t\t\tif (!strncmp(options->long_name, arg, arg_end - arg)) {\n+\t\t\tif (!strncmp(long_name, arg, arg_end - arg)) {\n is_abbreviated:\n \t\t\t\tif (abbrev_option) {\n \t\t\t\t\t/*\n@@ -227,7 +228,7 @@ is_abbreviated:\n \t\t\t\tif (!(flags & OPT_UNSET) && *arg_end)\n \t\t\t\t\tp->opt = arg_end + 1;\n \t\t\t\tabbrev_option = options;\n-\t\t\t\tabbrev_flags = flags;\n+\t\t\t\tabbrev_flags = flags ^ opt_flags;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\t/* negation allowed? */\n@@ -239,12 +240,18 @@ is_abbreviated:\n \t\t\t\tgoto is_abbreviated;\n \t\t\t}\n \t\t\t/* negated? */\n-\t\t\tif (strncmp(arg, \"no-\", 3))\n+\t\t\tif (prefixcmp(arg, \"no-\")) {\n+\t\t\t\tif (!prefixcmp(long_name, \"no-\")) {\n+\t\t\t\t\tlong_name += 3;\n+\t\t\t\t\topt_flags |= OPT_UNSET;\n+\t\t\t\t\tgoto again;\n+\t\t\t\t}\n \t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tflags |= OPT_UNSET;\n-\t\t\trest = skip_prefix(arg + 3, options->long_name);\n+\t\t\trest = skip_prefix(arg + 3, long_name);\n \t\t\t/* abbreviated and negated? */\n-\t\t\tif (!rest && !prefixcmp(options->long_name, arg + 3))\n+\t\t\tif (!rest && !prefixcmp(long_name, arg + 3))\n \t\t\t\tgoto is_abbreviated;\n \t\t\tif (!rest)\n \t\t\t\tcontinue;\n@@ -254,7 +261,7 @@ is_abbreviated:\n \t\t\t\tcontinue;\n \t\t\tp->opt = rest + 1;\n \t\t}\n-\t\treturn get_value(p, options, flags);\n+\t\treturn get_value(p, options, flags ^ opt_flags);\n \t}\n \n \tif (ambiguous_option)\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 79aefe2..aa57299 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -107,7 +107,7 @@ test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'\n test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown --fear'\n test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'\n \n-test_expect_failure 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n+test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n \n cat > expect << EOF\n boolean: 2\n-- \n1.7.9.2\n"},{"id":"185437","messageId":"4F49336C.3000303@lsrfire.ath.cx","threadId":"29737","inReplyTo":"4F49317A.3080809@lsrfire.ath.cx","subject":"[PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-25T19:15:56Z","receivedAt":"2012-02-25T19:15:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"PARSE_OPT_NEGHELP is confusing because short options defined with that\nflag do the opposite of what the helptext says. It is also not needed\nanymore now that options starting with no- can be negated by removing\nthat prefix. Convert its only two users to OPT_BOOL() and then remove\nsupport for PARSE_OPT_NEGHELP.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin/fast-export.c |    4 +---\n builtin/grep.c        |   15 +++++++--------\n parse-options.c       |    6 ++----\n parse-options.h       |    4 ----\n 4 files changed, 10 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 08fed98..19509ea 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -647,9 +647,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \t\t\t     \"Output full tree for each commit\"),\n \t\tOPT_BOOLEAN(0, \"use-done-feature\", &use_done_feature,\n \t\t\t     \"Use the done feature to terminate the stream\"),\n-\t\t{ OPTION_NEGBIT, 0, \"data\", &no_data, NULL,\n-\t\t\t\"Skip output of blob data\",\n-\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },\n+\t\tOPT_BOOL(0, \"no-data\", &no_data, \"Skip output of blob data\"),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e4ea900..b151467 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstruct string_list path_list = STRING_LIST_INIT_NODUP;\n \tint i;\n \tint dummy;\n-\tint use_index = 1;\n+\tint no_index = 0;\n \tenum {\n \t\tpattern_type_unspecified = 0,\n \t\tpattern_type_bre,\n@@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n \t\t\t\"search in index instead of in the work tree\"),\n-\t\t{ OPTION_BOOLEAN, 0, \"index\", &use_index, NULL,\n-\t\t\t\"finds in contents not managed by git\",\n-\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NEGHELP },\n+\t\tOPT_BOOL(0, \"no-index\", &no_index,\n+\t\t\t \"finds in contents not managed by git\"),\n \t\tOPT_BOOLEAN(0, \"untracked\", &untracked,\n \t\t\t\"search in both tracked and untracked files\"),\n \t\tOPT_SET_INT(0, \"exclude-standard\", &opt_exclude,\n@@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tbreak; /* nothing */\n \t}\n \n-\tif (use_index && !startup_info->have_repository)\n+\tif (!no_index && !startup_info->have_repository)\n \t\t/* die the same way as if we did it at the beginning */\n \t\tsetup_git_directory();\n \n@@ -963,11 +962,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!show_in_pager)\n \t\tsetup_pager();\n \n-\tif (!use_index && (untracked || cached))\n+\tif (no_index && (untracked || cached))\n \t\tdie(_(\"--cached or --untracked cannot be used with --no-index.\"));\n \n-\tif (!use_index || untracked) {\n-\t\tint use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;\n+\tif (no_index || untracked) {\n+\t\tint use_exclude = (opt_exclude < 0) ? !no_index : !!opt_exclude;\n \t\tif (list.nr)\n \t\t\tdie(_(\"--no-index or --untracked cannot be used with revs.\"));\n \t\thit = grep_directory(&opt, &pathspec, use_exclude);\ndiff --git a/parse-options.c b/parse-options.c\nindex 8906841..1908996 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -533,7 +533,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\tcontinue;\n \n \t\tpos = fprintf(outfile, \"    \");\n-\t\tif (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {\n+\t\tif (opts->short_name) {\n \t\t\tif (opts->flags & PARSE_OPT_NODASH)\n \t\t\t\tpos += fprintf(outfile, \"%c\", opts->short_name);\n \t\t\telse\n@@ -542,9 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tif (opts->long_name && opts->short_name)\n \t\t\tpos += fprintf(outfile, \", \");\n \t\tif (opts->long_name)\n-\t\t\tpos += fprintf(outfile, \"--%s%s\",\n-\t\t\t\t(opts->flags & PARSE_OPT_NEGHELP) ?  \"no-\" : \"\",\n-\t\t\t\topts->long_name);\n+\t\t\tpos += fprintf(outfile, \"--%s\", opts->long_name);\n \t\tif (opts->type == OPTION_NUMBER)\n \t\t\tpos += fprintf(outfile, \"-NUM\");\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 2e811dc..def9ced 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -40,7 +40,6 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_LASTARG_DEFAULT = 16,\n \tPARSE_OPT_NODASH = 32,\n \tPARSE_OPT_LITERAL_ARGHELP = 64,\n-\tPARSE_OPT_NEGHELP = 128,\n \tPARSE_OPT_SHELL_EVAL = 256\n };\n \n@@ -90,9 +89,6 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets\n  *\t\t\t\t(i.e. '<argh>') in the help message.\n  *\t\t\t\tUseful for options with multiple parameters.\n- *   PARSE_OPT_NEGHELP: says that the long option should always be shown with\n- *\t\t\t\tthe --no prefix in the usage message. Sometimes\n- *\t\t\t\tuseful for users of OPTION_NEGBIT.\n  *\n  * `callback`::\n  *   pointer to the callback to use for OPTION_CALLBACK or\n-- \n1.7.9.2\n"},{"id":"185476","messageId":"7vy5rpcgrk.fsf@alter.siamese.dyndns.org","threadId":"29737","inReplyTo":"4F49332E.7070003@lsrfire.ath.cx","subject":"Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-26T23:32:47Z","receivedAt":"2012-02-26T23:32:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Long options can be negated by adding no- right after the leading\n> two dashes. This is useful e.g. to override options set by aliases.\n>\n> For options that are defined to start with no- already, this looks\n> a bit funny. Allow such options to also be negated by removing the\n> prefix.\n\nTrue about \"a bit funny\", but the fact that the solution has to touch\nparse-options.c confuses me.\n\nI would naïvely expect that it would be sufficient to update an existing\ndefinition for \"--no-frotz\" that uses PARSE_OPT_NONEG to instead define\n\"--frotz\" that by itself is a no-op, and \"--no-frotz\" would cause whatever\nthe option currently means, with an update to the help text that says\nsomething to the effect that \"--frotz by itself is meaningless and is\nalways used as --no-frotz\".\n\nThere must be a reason the patch had to take an approach in the opposite\ndirection to allow removal of --[no-] prefix, but it is not obvious to me\nwhat it is.\n\nNote that I am _not_ saying that this is a bad change. I am just saying\nthat it is unclear why we still want two different ways to support the\n\"--no-frotz\" option, one by defining \"frotz\" option that allows \"no\" to be\nprefixed, and the other by defining \"no-frotz\" that allows \"no-\" to be\nstripped.\n"},{"id":"185511","messageId":"87d390smpa.fsf@thomas.inf.ethz.ch","threadId":"29737","inReplyTo":"7vy5rpcgrk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-27T08:30:09Z","receivedAt":"2012-02-27T08:30:09Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I would naïvely expect that it would be sufficient to update an existing\n> definition for \"--no-frotz\" that uses PARSE_OPT_NONEG to instead define\n> \"--frotz\" that by itself is a no-op, and \"--no-frotz\" would cause whatever\n> the option currently means, with an update to the help text that says\n> something to the effect that \"--frotz by itself is meaningless and is\n> always used as --no-frotz\".\n\nDoesn't that last quote already answer your question?  It would be\nrather awkward to see, in 'git apply -h',\n\n  --add                 Also apply additions in the patch.  This is the\n                        default; use --no-add to disable it.\n\nCompare to the current concise wording\n\n  --no-add              ignore additions made by the patch\n\nwhich lists the main (or mainly used) form in the left column, and\ndoesn't have to implicitly mention the 'no-' convention again to make\nthe help display useful.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185524","messageId":"7v8vjob3ff.fsf@alter.siamese.dyndns.org","threadId":"29737","inReplyTo":"87d390smpa.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T17:18:28Z","receivedAt":"2012-02-27T17:18:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@inf.ethz.ch> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> I would naïvely expect that it would be sufficient to update an existing\n>> definition for \"--no-frotz\" that uses PARSE_OPT_NONEG to instead define\n>> \"--frotz\" that by itself is a no-op, and \"--no-frotz\" would cause whatever\n>> the option currently means, with an update to the help text that says\n>> something to the effect that \"--frotz by itself is meaningless and is\n>> always used as --no-frotz\".\n>\n> Doesn't that last quote already answer your question?\n\nYes, but only partly.  I would agree with the awkwardness in\n\n> It would be rather awkward to see, in 'git apply -h',\n>\n>   --add                 Also apply additions in the patch.  This is the\n>                         default; use --no-add to disable it.\n\nbut it feeels somewhat questionable that the solution to get this:\n\n>\n> Compare to the current concise wording\n>\n>   --no-add              ignore additions made by the patch\n\nis to define OPT_BOOL(\"no-add\") that does not have any hint (other than\nthe fact that the option name begins with 3 character \"no-\") that this is\nan already negated boolean and the \"no-\" negation can be removed.\n\nThis means an option \"no-$foo\" can never mean anything but \"not foo\".  Not\nthat we would have to or necessarily want to support an option to give the\nnumber of foo as --no-foo=47, as --num-foo=47 is a perfectly good spelling\nfor such an option.\n\nIf it were OPT_BOOL(\"no-foo\", OPT_ISNEG | ...) that signals the parser\nthat:\n\n - the option name is already negative;\n - the leading \"no-\" is to be removed to negate it; and\n - no extra leading \"no-\", i.e. \"--no-no-foo\", is accepted.\n\nI probably wouldn't have felt this uneasy iffiness.\n\nBut it is just a minor issue.\n"},{"id":"185530","messageId":"4F4BC3B3.7080000@lsrfire.ath.cx","threadId":"29737","inReplyTo":"7v8vjob3ff.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-27T17:56:03Z","receivedAt":"2012-02-27T17:56:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2012 18:18, schrieb Junio C Hamano:\n> Thomas Rast<trast@inf.ethz.ch>  writes:\n>\n>> Junio C Hamano<gitster@pobox.com>  writes:\n>>\n>>> I would naïvely expect that it would be sufficient to update an existing\n>>> definition for \"--no-frotz\" that uses PARSE_OPT_NONEG to instead define\n>>> \"--frotz\" that by itself is a no-op, and \"--no-frotz\" would cause whatever\n>>> the option currently means, with an update to the help text that says\n>>> something to the effect that \"--frotz by itself is meaningless and is\n>>> always used as --no-frotz\".\n>>\n>> Doesn't that last quote already answer your question?\n>\n> Yes, but only partly.  I would agree with the awkwardness in\n>\n>> It would be rather awkward to see, in 'git apply -h',\n>>\n>>    --add                 Also apply additions in the patch.  This is the\n>>                          default; use --no-add to disable it.\n>\n> but it feeels somewhat questionable that the solution to get this:\n>\n>>\n>> Compare to the current concise wording\n>>\n>>    --no-add              ignore additions made by the patch\n>\n> is to define OPT_BOOL(\"no-add\") that does not have any hint (other than\n> the fact that the option name begins with 3 character \"no-\") that this is\n> an already negated boolean and the \"no-\" negation can be removed.\n\nThe parser already knows that the prefix \"no-\" negates an option.  It \ncurrenmtly only applies this knowledge if that prefix is added, but not \nif it is removed, which is inconsistent.\n\n> This means an option \"no-$foo\" can never mean anything but \"not foo\".  Not\n> that we would have to or necessarily want to support an option to give the\n> number of foo as --no-foo=47, as --num-foo=47 is a perfectly good spelling\n> for such an option.\n\nWith the patch, you can define a --no-foo option that doesn't accept \n--foo as its negation by specifying PARSE_OPT_NONEG.  That would also \nforbid --no-no-foo, though, but that's probably a good thing.\n\n> If it were OPT_BOOL(\"no-foo\", OPT_ISNEG | ...) that signals the parser\n> that:\n>\n>   - the option name is already negative;\n>   - the leading \"no-\" is to be removed to negate it; and\n>   - no extra leading \"no-\", i.e. \"--no-no-foo\", is accepted.\n>\n> I probably wouldn't have felt this uneasy iffiness.\n\nTeaching the parser to understand that removal of the prefix \"no-\" \nnegates an option on top of its existing knowledge that adding it does \nthe same just adds the other side of the same coin, which was curiously \nmissing.\n\nThe patch does not forbid adding \"no-\" to an option that already starts \nwith \"no-\".  This stricter rule would be easy to add, but since that is \ncurrently the only way to negate such options, it would break backwards \ncompatibility and thus should be added in a separate patch, if at all.\n\nWith the patch, the following guidelines are followed:\n\n\t- \"no-\" means no, for both developers and users.\n\t- The user doesn't have to to say \"no-no-\".\n\nThe results feels simpler to me.\n\nRené\n"},{"id":"185535","messageId":"20120227182504.GA1600@sigill.intra.peff.net","threadId":"29737","inReplyTo":"4F49336C.3000303@lsrfire.ath.cx","subject":"Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-27T18:25:04Z","receivedAt":"2012-02-27T18:25:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index e4ea900..b151467 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \tstruct string_list path_list = STRING_LIST_INIT_NODUP;\n>  \tint i;\n>  \tint dummy;\n> -\tint use_index = 1;\n> +\tint no_index = 0;\n>  \tenum {\n>  \t\tpattern_type_unspecified = 0,\n>  \t\tpattern_type_bre,\n> @@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \tstruct option options[] = {\n>  \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n>  \t\t\t\"search in index instead of in the work tree\"),\n> -\t\t{ OPTION_BOOLEAN, 0, \"index\", &use_index, NULL,\n> -\t\t\t\"finds in contents not managed by git\",\n> -\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NEGHELP },\n> +\t\tOPT_BOOL(0, \"no-index\", &no_index,\n> +\t\t\t \"finds in contents not managed by git\"),\n>  \t\tOPT_BOOLEAN(0, \"untracked\", &untracked,\n>  \t\t\t\"search in both tracked and untracked files\"),\n>  \t\tOPT_SET_INT(0, \"exclude-standard\", &opt_exclude,\n> @@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\tbreak; /* nothing */\n>  \t}\n>  \n> -\tif (use_index && !startup_info->have_repository)\n> +\tif (!no_index && !startup_info->have_repository)\n\nHmm. We usually try to avoid these sorts of double negations in the\ncode, as they can often make the logic hard to read. In this case, it is\nnot _so_ bad, because out of the 4 uses of use_index/no_index, only one\nis \"!no_index\", and it is in a relatively simple conditional.\n\nBut I do feel like the original was slightly easier to read, and that\ngetting rid of NEGHELP is restricting how the developer can express the\noptions.\n\nI think your original motivation was that NEGHELP lead to confusion\nwhere the name of the option does not match its description. Would it be\nbetter to simply be explicit that an option is a reversed boolean (i.e.,\nwhat the user specifies on the command line and what is in the code are\nnaturally opposites). Like:\n\n OPT_REVERSE_BOOL(0, \"no-index\", &use_index,\n             \"finds in contents not managed by git\"),\n\nUsing NEGHELP, the \"reverse\" is between the option name and the\ndescription, which is very subtle. Here it is between the option name\nand the variable, which is hopefully a little more explicit (especially\nwith the big REVERSE in the macro name).\n\nI dunno. Given that there are only two uses of NEGHELP, and that they\ndon't come out too badly, I don't care _too_ much. But I have seen some\nreally tortured logic with double-negations like this, and I'm concerned\nthat a few months down the road somebody is going to want NEGHELP (or\nsomething similar) in a case where it actually does really impact\nreadability.\n\n-Peff\n"},{"id":"185544","messageId":"7vpqd085nj.fsf@alter.siamese.dyndns.org","threadId":"29737","inReplyTo":"20120227182504.GA1600@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T18:58:40Z","receivedAt":"2012-02-27T18:58:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... Would it be\n> better to simply be explicit that an option is a reversed boolean (i.e.,\n> what the user specifies on the command line and what is in the code are\n> naturally opposites). Like:\n>\n>  OPT_REVERSE_BOOL(0, \"no-index\", &use_index,\n>              \"finds in contents not managed by git\"),\n\nYou said it much better than my attempt ;-).\n\n> Using NEGHELP, the \"reverse\" is between the option name and the\n> description, which is very subtle. Here it is between the option name\n> and the variable, which is hopefully a little more explicit (especially\n> with the big REVERSE in the macro name).\n>\n> I dunno. Given that there are only two uses of NEGHELP, and that they\n> don't come out too badly, I don't care _too_ much. But I have seen some\n> really tortured logic with double-negations like this, and I'm concerned\n> that a few months down the road somebody is going to want NEGHELP (or\n> something similar) in a case where it actually does really impact\n> readability.\n\nYeah, I share a similar minor and iffy feeling about the result.\n"},{"id":"185559","messageId":"7vzkc457g3.fsf@alter.siamese.dyndns.org","threadId":"29737","inReplyTo":"4F4BC3B3.7080000@lsrfire.ath.cx","subject":"Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T20:48:12Z","receivedAt":"2012-02-27T20:48:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> The patch does not forbid adding \"no-\" to an option that already starts\n> with \"no-\".  This stricter rule would be easy to add, but since that is\n> currently the only way to negate such options, it would break backwards\n> compatibility and thus should be added in a separate patch, if at all.\n>\n> With the patch, the following guidelines are followed:\n>\n> \t- \"no-\" means no, for both developers and users.\n> \t- The user doesn't have to to say \"no-no-\".\n>\n> The results feels simpler to me.\n\nSounds fair.\n\nI agree that the backward compatibility of --no-no-foo is a potential\nproblem, if any of the actions controlled by \"--no-foo\" option defaults to\nthe behaviour when \"--no-foo\" is given.  Among the existing 13 that you\nlisted, I do not think there is any that tempts any existing user to ask\nfor negation with \"--no-no-foo\" form, so I think we should be Ok.\n"},{"id":"185576","messageId":"4F4C0308.2050804@lsrfire.ath.cx","threadId":"29737","inReplyTo":"20120227182504.GA1600@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-27T22:26:16Z","receivedAt":"2012-02-27T22:26:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2012 19:25, schrieb Jeff King:\n> On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:\n>\n>> diff --git a/builtin/grep.c b/builtin/grep.c\n>> index e4ea900..b151467 100644\n>> --- a/builtin/grep.c\n>> +++ b/builtin/grep.c\n>> @@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>   \tstruct string_list path_list = STRING_LIST_INIT_NODUP;\n>>   \tint i;\n>>   \tint dummy;\n>> -\tint use_index = 1;\n>> +\tint no_index = 0;\n>>   \tenum {\n>>   \t\tpattern_type_unspecified = 0,\n>>   \t\tpattern_type_bre,\n>> @@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>   \tstruct option options[] = {\n>>   \t\tOPT_BOOLEAN(0, \"cached\",&cached,\n>>   \t\t\t\"search in index instead of in the work tree\"),\n>> -\t\t{ OPTION_BOOLEAN, 0, \"index\",&use_index, NULL,\n>> -\t\t\t\"finds in contents not managed by git\",\n>> -\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NEGHELP },\n>> +\t\tOPT_BOOL(0, \"no-index\",&no_index,\n>> +\t\t\t \"finds in contents not managed by git\"),\n>>   \t\tOPT_BOOLEAN(0, \"untracked\",&untracked,\n>>   \t\t\t\"search in both tracked and untracked files\"),\n>>   \t\tOPT_SET_INT(0, \"exclude-standard\",&opt_exclude,\n>> @@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>   \t\tbreak; /* nothing */\n>>   \t}\n>>\n>> -\tif (use_index&&  !startup_info->have_repository)\n>> +\tif (!no_index&&  !startup_info->have_repository)\n\n[Unrelated: The whitespace in the two lines above and before ampersands \nin general was damaged by Thunderbird.  First time I noticed.]\n\n> Hmm. We usually try to avoid these sorts of double negations in the\n> code, as they can often make the logic hard to read. In this case, it is\n> not _so_ bad, because out of the 4 uses of use_index/no_index, only one\n> is \"!no_index\", and it is in a relatively simple conditional.\n\nThe variable could be named \"unmanaged\", \"external\" or similar instead \nof \"no_index\".  The latter just matches the option name and thus was the \nobvious first choice to me.\n\n> But I do feel like the original was slightly easier to read, and that\n> getting rid of NEGHELP is restricting how the developer can express the\n> options.\n>\n> I think your original motivation was that NEGHELP lead to confusion\n> where the name of the option does not match its description. Would it be\n> better to simply be explicit that an option is a reversed boolean (i.e.,\n> what the user specifies on the command line and what is in the code are\n> naturally opposites). Like:\n>\n>   OPT_REVERSE_BOOL(0, \"no-index\",&use_index,\n>               \"finds in contents not managed by git\"),\n\nIt's better than NEGHELP, but I find your use of two negations (REVERSE \nand \"no-\") confusing.  We don't need to invent new OPT_ types for this, \nby the way, we can just do this:\n\n\tOPT_NEGBIT(0, \"no-index\", &use_index,\n\t           \"finds in contents not managed by git\", 1),\n\nIt certainly shortens the patch.\n\n> Using NEGHELP, the \"reverse\" is between the option name and the\n> description, which is very subtle. Here it is between the option name\n> and the variable, which is hopefully a little more explicit (especially\n> with the big REVERSE in the macro name).\n\nWe have precedence for OPT_NEGBIT in grep, although the double negations \nfor -h and --full-name are required because both turn off bits that \nother options turn on, while for --no-index it wouldn't be strictly \nneeded, as there is no option that overrules it except --index.\n\nI don't care too much either way, though.  The changes from patch 2 (the \nno no-no one) are not restricted to OPT_BOOL.\n\n> I dunno. Given that there are only two uses of NEGHELP, and that they\n> don't come out too badly, I don't care _too_ much. But I have seen some\n> really tortured logic with double-negations like this, and I'm concerned\n> that a few months down the road somebody is going to want NEGHELP (or\n> something similar) in a case where it actually does really impact\n> readability.\n\nI'm curious to see a case that can be solved better using NEGHELP, but \nwe can always add it back if we find such a beast.  I'd much rather see \nit go until then because of it's non-obvious semantics.\n\nRené\n"},{"id":"185583","messageId":"20120228003402.GA20784@sigill.intra.peff.net","threadId":"29737","inReplyTo":"4F4C0308.2050804@lsrfire.ath.cx","subject":"Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-28T00:34:03Z","receivedAt":"2012-02-28T00:34:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2012 at 11:26:16PM +0100, René Scharfe wrote:\n\n> >  OPT_REVERSE_BOOL(0, \"no-index\",&use_index,\n> >              \"finds in contents not managed by git\"),\n> \n> It's better than NEGHELP, but I find your use of two negations\n> (REVERSE and \"no-\") confusing.  We don't need to invent new OPT_\n> types for this, by the way, we can just do this:\n> \n> \tOPT_NEGBIT(0, \"no-index\", &use_index,\n> \t           \"finds in contents not managed by git\", 1),\n> \n> It certainly shortens the patch.\n\nRight. The point is that the code and the option want to look at the\nconditional in two different ways. So you have to have negate it\n_somewhere_. Either at point of use (your original patch), between the\noption name and the variable name (what I wrote above), or using a\nreversed help text (the original code before your patch).\n\nI think we both agree that NEGHELP is the worst one. I have a slight\npreference for doing the reversal at the point of the option definition,\nif only because we know it happens at a single point, and it lets the\nactual code (which is usually the trickier part) be more clear. But\nbeyond that, I think it is largely a matter of aesthetics.\n\n> >I dunno. Given that there are only two uses of NEGHELP, and that they\n> >don't come out too badly, I don't care _too_ much. But I have seen some\n> >really tortured logic with double-negations like this, and I'm concerned\n> >that a few months down the road somebody is going to want NEGHELP (or\n> >something similar) in a case where it actually does really impact\n> >readability.\n> \n> I'm curious to see a case that can be solved better using NEGHELP,\n> but we can always add it back if we find such a beast.  I'd much\n> rather see it go until then because of it's non-obvious semantics.\n\nYeah, I should have spelled out my \"or something similar\" more. I'd\nrather see something like NEGBIT above than NEGHELP; the latter is a bit\ntoo subtle.\n\n-Peff\n"},{"id":"185652","messageId":"4F4D25A1.8050702@lsrfire.ath.cx","threadId":"29737","inReplyTo":"4F49336C.3000303@lsrfire.ath.cx","subject":"[PATCH 3/3 v2] parse-options: remove PARSE_OPT_NEGHELP","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-28T19:06:09Z","receivedAt":"2012-02-28T19:06:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"PARSE_OPT_NEGHELP is confusing because short options defined with that\nflag do the opposite of what the helptext says. It is also not needed\nanymore now that options starting with no- can be negated by removing\nthat prefix. Convert its only two users to OPT_NEGBIT() and OPT_BOOL()\nand then remove support for PARSE_OPT_NEGHELP.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nThis version doesn't invert the logic in grep anymore.\n\n builtin/fast-export.c |    4 +---\n builtin/grep.c        |    5 ++---\n parse-options.c       |    6 ++----\n parse-options.h       |    4 ----\n 4 files changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 08fed98..19509ea 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -647,9 +647,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \t\t\t     \"Output full tree for each commit\"),\n \t\tOPT_BOOLEAN(0, \"use-done-feature\", &use_done_feature,\n \t\t\t     \"Use the done feature to terminate the stream\"),\n-\t\t{ OPTION_NEGBIT, 0, \"data\", &no_data, NULL,\n-\t\t\t\"Skip output of blob data\",\n-\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },\n+\t\tOPT_BOOL(0, \"no-data\", &no_data, \"Skip output of blob data\"),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e4ea900..643938d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOLEAN(0, \"cached\", &cached,\n \t\t\t\"search in index instead of in the work tree\"),\n-\t\t{ OPTION_BOOLEAN, 0, \"index\", &use_index, NULL,\n-\t\t\t\"finds in contents not managed by git\",\n-\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NEGHELP },\n+\t\tOPT_NEGBIT(0, \"no-index\", &use_index,\n+\t\t\t \"finds in contents not managed by git\", 1),\n \t\tOPT_BOOLEAN(0, \"untracked\", &untracked,\n \t\t\t\"search in both tracked and untracked files\"),\n \t\tOPT_SET_INT(0, \"exclude-standard\", &opt_exclude,\ndiff --git a/parse-options.c b/parse-options.c\nindex 8906841..1908996 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -533,7 +533,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\tcontinue;\n \n \t\tpos = fprintf(outfile, \"    \");\n-\t\tif (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {\n+\t\tif (opts->short_name) {\n \t\t\tif (opts->flags & PARSE_OPT_NODASH)\n \t\t\t\tpos += fprintf(outfile, \"%c\", opts->short_name);\n \t\t\telse\n@@ -542,9 +542,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tif (opts->long_name && opts->short_name)\n \t\t\tpos += fprintf(outfile, \", \");\n \t\tif (opts->long_name)\n-\t\t\tpos += fprintf(outfile, \"--%s%s\",\n-\t\t\t\t(opts->flags & PARSE_OPT_NEGHELP) ?  \"no-\" : \"\",\n-\t\t\t\topts->long_name);\n+\t\t\tpos += fprintf(outfile, \"--%s\", opts->long_name);\n \t\tif (opts->type == OPTION_NUMBER)\n \t\t\tpos += fprintf(outfile, \"-NUM\");\n \ndiff --git a/parse-options.h b/parse-options.h\nindex 2e811dc..def9ced 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -40,7 +40,6 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_LASTARG_DEFAULT = 16,\n \tPARSE_OPT_NODASH = 32,\n \tPARSE_OPT_LITERAL_ARGHELP = 64,\n-\tPARSE_OPT_NEGHELP = 128,\n \tPARSE_OPT_SHELL_EVAL = 256\n };\n \n@@ -90,9 +89,6 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets\n  *\t\t\t\t(i.e. '<argh>') in the help message.\n  *\t\t\t\tUseful for options with multiple parameters.\n- *   PARSE_OPT_NEGHELP: says that the long option should always be shown with\n- *\t\t\t\tthe --no prefix in the usage message. Sometimes\n- *\t\t\t\tuseful for users of OPTION_NEGBIT.\n  *\n  * `callback`::\n  *   pointer to the callback to use for OPTION_CALLBACK or\n-- \n1.7.9.2\n"},{"id":"185653","messageId":"20120228190936.GC11260@sigill.intra.peff.net","threadId":"29737","inReplyTo":"4F4D25A1.8050702@lsrfire.ath.cx","subject":"Re: [PATCH 3/3 v2] parse-options: remove PARSE_OPT_NEGHELP","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-28T19:09:36Z","receivedAt":"2012-02-28T19:09:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 08:06:09PM +0100, René Scharfe wrote:\n\n> PARSE_OPT_NEGHELP is confusing because short options defined with that\n> flag do the opposite of what the helptext says. It is also not needed\n> anymore now that options starting with no- can be negated by removing\n> that prefix. Convert its only two users to OPT_NEGBIT() and OPT_BOOL()\n> and then remove support for PARSE_OPT_NEGHELP.\n> \n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> ---\n> This version doesn't invert the logic in grep anymore.\n\nThanks, I like this one much better.\n\n-Peff\n"},{"id":"185669","messageId":"4F4D3545.6060704@lsrfire.ath.cx","threadId":"29737","inReplyTo":"7vzkc457g3.fsf@alter.siamese.dyndns.org","subject":"[PATCH 4/3] parse-options: disallow --no-no-sth","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-28T20:12:53Z","receivedAt":"2012-02-28T20:12:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Now that options whose definition starts with \"no-\" can be negated\nby removing said \"no-\", there is no need anymore to allow them to\nbe negated by adding a second \"no-\", which just looks silly.\n\nThe following thirteen options are affected:\n\n\tapply          --no-add\n\tbisect--helper --no-checkout\n\tcheckout-index --no-create\n\tclone          --no-checkout --no-hardlinks\n\tcommit         --no-verify   --no-post-rewrite\n\tformat-patch   --no-binary\n\thash-object    --no-filters\n\tread-tree      --no-sparse-checkout\n\trevert         --no-commit\n\tshow-branch    --no-name\n\tupdate-ref     --no-deref\n\nE.g., with this patch --no-add and --add (its reverse) are still\naccepted by git apply, but --no-no-add isn't anymore.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n parse-options.c          |    3 +++\n t/t0040-parse-options.sh |    4 ++--\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 1908996..dc59bba 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -248,6 +248,9 @@ is_abbreviated:\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\t/* double negation? */\n+\t\t\tif (!prefixcmp(long_name, \"no-\"))\n+\t\t\t\tcontinue;\n \t\t\tflags |= OPT_UNSET;\n \t\t\trest = skip_prefix(arg + 3, long_name);\n \t\t\t/* abbreviated and negated? */\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex a44bcb9..b124f3c 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -101,11 +101,11 @@ test_expect_success 'OPT_BOOL() #5' 'check boolean: 1 -B'\n 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+test_expect_success 'OPT_BOOL() negation' 'check boolean: 0 -D --no-yes'\n \n test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown --fear'\n test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'\n+test_expect_success 'OPT_BOOL() no negation #3' 'check_unknown --no-no-doubt'\n \n test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n \n-- \n1.7.9.2\n"},{"id":"185675","messageId":"7vlinmy80x.fsf@alter.siamese.dyndns.org","threadId":"29737","inReplyTo":"4F4D3545.6060704@lsrfire.ath.cx","subject":"Re: [PATCH 4/3] parse-options: disallow --no-no-sth","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T21:15:10Z","receivedAt":"2012-02-28T21:15:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Now that options whose definition starts with \"no-\" can be negated\n> by removing said \"no-\", there is no need anymore to allow them to\n> be negated by adding a second \"no-\", which just looks silly.\n\nThanks.  But accepting them silently and do what the user would have\nexpected, especially if we do not advertise it, would not hurt anybody,\nno?\n"},{"id":"185727","messageId":"4F4E6912.7060204@lsrfire.ath.cx","threadId":"29737","inReplyTo":"7vlinmy80x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/3] parse-options: disallow --no-no-sth","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-02-29T18:06:10Z","receivedAt":"2012-02-29T18:06:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.02.2012 22:15, schrieb Junio C Hamano:\n> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:\n>\n>> Now that options whose definition starts with \"no-\" can be negated\n>> by removing said \"no-\", there is no need anymore to allow them to\n>> be negated by adding a second \"no-\", which just looks silly.\n>\n> Thanks.  But accepting them silently and do what the user would have\n> expected, especially if we do not advertise it, would not hurt anybody,\n> no?\n\nYes, that's why I didn't include this fourth patch from the beginning. \nHowever, I understood your comment \"Among the existing 13 that you\nlisted, I do not think there is any that tempts any existing user to ask\nfor negation with \"--no-no-foo\" form, so I think we should be Ok.\" to \nmean that nobody uses the double-no form of the existing negative \noptions anyway, and therefore we should disallow it.\n\nPlease drop this extra patch if I misunderstood you, in order to keep \nbackward compatibility.\n\nRené\n"},{"id":"185732","messageId":"7vd38xtqce.fsf@alter.siamese.dyndns.org","threadId":"29737","inReplyTo":"4F4E6912.7060204@lsrfire.ath.cx","subject":"Re: [PATCH 4/3] parse-options: disallow --no-no-sth","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-29T19:02:57Z","receivedAt":"2012-02-29T19:02:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Please drop this extra patch if I misunderstood you, in order to keep\n> backward compatibility.\n\nI do not think it makes practical difference between accepting silently or\nforbidding, so let's stop at 3/3 and keep --no-no-sth working.\n\nThanks.\n"}]}