{"thread":{"id":"45734","subject":"[PATCH] various: disallow --no-no-OPT for --no-opt options","startedAt":"2017-04-18T17:09:34Z","lastAt":"2017-04-19T15:06:19Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","René Scharfe","Jeff King","Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"317119","messageId":"20170418170914.9701-1-avarab@gmail.com","threadId":"45734","inReplyTo":null,"subject":"[PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-18T17:09:14Z","receivedAt":"2017-04-18T17:09:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change various --no-OPT options which don't supply PARSE_OPT_NONEG to\nmake --no-no-OPT an error.\n\nAll of these worked before this change, e.g. doing cloning by doing\n\"git clone --no-no-checkout\" is equivalent to just \"git clone\", but\nthis was never intended, and is inconsistent with other --no-OPT\noptions which do pass PARSE_OPT_NONEG.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nFor this one I'm not bothering to track down & CC every single person\nwho originally added these --no-OPT options. Clearly just a trivial\nbug we can fix.\n\n apply.c                       | 2 +-\n builtin/bisect--helper.c      | 2 +-\n builtin/check-ignore.c        | 2 +-\n builtin/checkout-index.c      | 2 +-\n builtin/clone.c               | 4 ++--\n builtin/commit.c              | 4 ++--\n builtin/fast-export.c         | 2 +-\n builtin/grep.c                | 2 +-\n builtin/hash-object.c         | 2 +-\n builtin/log.c                 | 4 ++--\n builtin/push.c                | 2 +-\n builtin/read-tree.c           | 2 +-\n builtin/revert.c              | 2 +-\n builtin/show-branch.c         | 2 +-\n builtin/update-ref.c          | 2 +-\n parse-options.h               | 7 +++++++\n t/helper/test-parse-options.c | 1 +\n t/t0040-parse-options.sh      | 3 +++\n 18 files changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex e6dbab26ad..47a91d0762 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4917,7 +4917,7 @@ int apply_parse_options(int argc, const char **argv,\n \t\t{ OPTION_CALLBACK, 'p', NULL, state, N_(\"num\"),\n \t\t\tN_(\"remove <num> leading slashes from traditional diff paths\"),\n \t\t\t0, apply_option_parse_p },\n-\t\tOPT_BOOL(0, \"no-add\", &state->no_add,\n+\t\tOPT_BOOL_NONEG(0, \"no-add\", &state->no_add,\n \t\t\tN_(\"ignore additions made by the patch\")),\n \t\tOPT_BOOL(0, \"stat\", &state->diffstat,\n \t\t\tN_(\"instead of applying the patch, output diffstat for the input\")),\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 3324229025..5fe9093947 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -15,7 +15,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"next-all\", &next_all,\n \t\t\t N_(\"perform 'git bisect next'\")),\n-\t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n+\t\tOPT_BOOL_NONEG(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_END()\n \t};\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 1d73d3ca3d..cb6467946e 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -24,7 +24,7 @@ static const struct option check_ignore_options[] = {\n \t\t N_(\"terminate input and output records by a NUL character\")),\n \tOPT_BOOL('n', \"non-matching\", &show_non_matching,\n \t\t N_(\"show non-matching input paths\")),\n-\tOPT_BOOL(0, \"no-index\", &no_index,\n+\tOPT_BOOL_NONEG(0, \"no-index\", &no_index,\n \t\t N_(\"ignore index when checking\")),\n \tOPT_END()\n };\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 07631d0c9c..cf84e31ce8 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -161,7 +161,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tOPT__FORCE(&force, N_(\"force overwrite of existing files\")),\n \t\tOPT__QUIET(&quiet,\n \t\t\tN_(\"no warning for existing files and files not in index\")),\n-\t\tOPT_BOOL('n', \"no-create\", &not_new,\n+\t\tOPT_BOOL_NONEG('n', \"no-create\", &not_new,\n \t\t\tN_(\"don't checkout new files\")),\n \t\tOPT_BOOL('u', \"index\", &index_opt,\n \t\t\t N_(\"update stat information in the index file\")),\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex de85b85254..32c5843563 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -76,7 +76,7 @@ static struct option builtin_clone_options[] = {\n \tOPT__VERBOSITY(&option_verbosity),\n \tOPT_BOOL(0, \"progress\", &option_progress,\n \t\t N_(\"force progress reporting\")),\n-\tOPT_BOOL('n', \"no-checkout\", &option_no_checkout,\n+\tOPT_BOOL_NONEG('n', \"no-checkout\", &option_no_checkout,\n \t\t N_(\"don't create a checkout\")),\n \tOPT_BOOL(0, \"bare\", &option_bare, N_(\"create a bare repository\")),\n \tOPT_HIDDEN_BOOL(0, \"naked\", &option_bare,\n@@ -85,7 +85,7 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"create a mirror repository (implies bare)\")),\n \tOPT_BOOL('l', \"local\", &option_local,\n \t\tN_(\"to clone from a local repository\")),\n-\tOPT_BOOL(0, \"no-hardlinks\", &option_no_hardlinks,\n+\tOPT_BOOL_NONEG(0, \"no-hardlinks\", &option_no_hardlinks,\n \t\t    N_(\"don't use local hardlinks, always copy\")),\n \tOPT_BOOL('s', \"shared\", &option_shared,\n \t\t    N_(\"setup as shared repository\")),\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4e288bc513..46830ebfaf 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1607,7 +1607,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"interactive\", &interactive, N_(\"interactively add files\")),\n \t\tOPT_BOOL('p', \"patch\", &patch_interactive, N_(\"interactively add changes\")),\n \t\tOPT_BOOL('o', \"only\", &only, N_(\"commit only specified files\")),\n-\t\tOPT_BOOL('n', \"no-verify\", &no_verify, N_(\"bypass pre-commit and commit-msg hooks\")),\n+\t\tOPT_BOOL_NONEG('n', \"no-verify\", &no_verify, N_(\"bypass pre-commit and commit-msg hooks\")),\n \t\tOPT_BOOL(0, \"dry-run\", &dry_run, N_(\"show what would be committed\")),\n \t\tOPT_SET_INT(0, \"short\", &status_format, N_(\"show status concisely\"),\n \t\t\t    STATUS_FORMAT_SHORT),\n@@ -1620,7 +1620,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('z', \"null\", &s.null_termination,\n \t\t\t N_(\"terminate entries with NUL\")),\n \t\tOPT_BOOL(0, \"amend\", &amend, N_(\"amend previous commit\")),\n-\t\tOPT_BOOL(0, \"no-post-rewrite\", &no_post_rewrite, N_(\"bypass post-rewrite hook\")),\n+\t\tOPT_BOOL_NONEG(0, \"no-post-rewrite\", &no_post_rewrite, N_(\"bypass post-rewrite hook\")),\n \t\t{ OPTION_STRING, 'u', \"untracked-files\", &untracked_files_arg, N_(\"mode\"), N_(\"show untracked files, optional modes: all, normal, no. (Default: all)\"), PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n \t\t/* end commit contents options */\n \ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex e0220630d0..dfbd245842 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -994,7 +994,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"Output full tree for each commit\")),\n \t\tOPT_BOOL(0, \"use-done-feature\", &use_done_feature,\n \t\t\t     N_(\"Use the done feature to terminate the stream\")),\n-\t\tOPT_BOOL(0, \"no-data\", &no_data, N_(\"Skip output of blob data\")),\n+\t\tOPT_BOOL_NONEG(0, \"no-data\", &no_data, N_(\"Skip output of blob data\")),\n \t\tOPT_STRING_LIST(0, \"refspec\", &refspecs_list, N_(\"refspec\"),\n \t\t\t     N_(\"Apply refspec to exported refs\")),\n \t\tOPT_BOOL(0, \"anonymize\", &anonymize, N_(\"anonymize output\")),\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 65070c52fc..923cc570e6 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -978,7 +978,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"cached\", &cached,\n \t\t\tN_(\"search in index instead of in the work tree\")),\n-\t\tOPT_NEGBIT(0, \"no-index\", &use_index,\n+\t\tOPT_NEGBIT_NONEG(0, \"no-index\", &use_index,\n \t\t\t N_(\"find in contents not managed by git\"), 1),\n \t\tOPT_BOOL(0, \"untracked\", &untracked,\n \t\t\tN_(\"search in both tracked and untracked files\")),\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex bbeaf20bcc..c563f2a9ad 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -96,7 +96,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t\t\tHASH_WRITE_OBJECT),\n \t\tOPT_COUNTUP( 0 , \"stdin\", &hashstdin, N_(\"read the object from stdin\")),\n \t\tOPT_BOOL( 0 , \"stdin-paths\", &stdin_paths, N_(\"read file names from stdin\")),\n-\t\tOPT_BOOL( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n+\t\tOPT_BOOL_NONEG( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n \t\tOPT_BOOL( 0, \"literally\", &literally, N_(\"just hash any random garbage to create corrupt objects for debugging Git\")),\n \t\tOPT_STRING( 0 , \"path\", &vpath, N_(\"file\"), N_(\"process file as it were from this path\")),\n \t\tOPT_END()\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b3b10cc1ed..9824e137ec 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1416,7 +1416,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t    PARSE_OPT_NOARG, numbered_callback },\n \t\t{ OPTION_CALLBACK, 'N', \"no-numbered\", &numbered, NULL,\n \t\t\t    N_(\"use [PATCH] even with multiple patches\"),\n-\t\t\t    PARSE_OPT_NOARG, no_numbered_callback },\n+\t\t\t    PARSE_OPT_NOARG | PARSE_OPT_NONEG , no_numbered_callback },\n \t\tOPT_BOOL('s', \"signoff\", &do_signoff, N_(\"add Signed-off-by:\")),\n \t\tOPT_BOOL(0, \"stdout\", &use_stdout,\n \t\t\t    N_(\"print patches to standard out\")),\n@@ -1442,7 +1442,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 'k', \"keep-subject\", &rev, NULL,\n \t\t\t    N_(\"don't strip/add [PATCH]\"),\n \t\t\t    PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback },\n-\t\tOPT_BOOL(0, \"no-binary\", &no_binary_diff,\n+\t\tOPT_BOOL_NONEG(0, \"no-binary\", &no_binary_diff,\n \t\t\t N_(\"don't output binary diffs\")),\n \t\tOPT_BOOL(0, \"zero-commit\", &zero_commit,\n \t\t\t N_(\"output all-zero hash in From header\")),\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 5c22e9f2e5..216307c0fa 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -539,7 +539,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"progress\", &progress, N_(\"force progress reporting\")),\n \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune locally removed refs\"),\n \t\t\tTRANSPORT_PUSH_PRUNE),\n-\t\tOPT_BIT(0, \"no-verify\", &flags, N_(\"bypass pre-push hook\"), TRANSPORT_PUSH_NO_HOOK),\n+\t\tOPT_BIT_NONEG(0, \"no-verify\", &flags, N_(\"bypass pre-push hook\"), TRANSPORT_PUSH_NO_HOOK),\n \t\tOPT_BIT(0, \"follow-tags\", &flags, N_(\"push missing but relevant tags\"),\n \t\t\tTRANSPORT_PUSH_FOLLOW_TAGS),\n \t\t{ OPTION_CALLBACK,\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 23e212ee8c..e9359ecb1e 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -153,7 +153,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\tOPT_BOOL('i', NULL, &opts.index_only,\n \t\t\t N_(\"don't check the working tree after merging\")),\n \t\tOPT__DRY_RUN(&opts.dry_run, N_(\"don't update the index or the work tree\")),\n-\t\tOPT_BOOL(0, \"no-sparse-checkout\", &opts.skip_sparse_checkout,\n+\t\tOPT_BOOL_NONEG(0, \"no-sparse-checkout\", &opts.skip_sparse_checkout,\n \t\t\t N_(\"skip applying sparse checkout filter\")),\n \t\tOPT_BOOL(0, \"debug-unpack\", &opts.debug_unpack,\n \t\t\t N_(\"debug unpack-trees\")),\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 345d9586a7..e708fdc228 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -98,7 +98,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), 'q'),\n \t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n \t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), 'a'),\n-\t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n+\t\tOPT_BOOL_NONEG('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n \t\tOPT_NOOP_NOARG('r', NULL),\n \t\tOPT_BOOL('s', \"signoff\", &opts->signoff, N_(\"add Signed-off-by:\")),\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 19756595d5..6827846b54 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -638,7 +638,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t    N_(\"show <n> more commits after the common ancestor\"),\n \t\t\t    PARSE_OPT_OPTARG, NULL, (intptr_t)1 },\n \t\tOPT_SET_INT(0, \"list\", &extra, N_(\"synonym to more=-1\"), -1),\n-\t\tOPT_BOOL(0, \"no-name\", &no_name, N_(\"suppress naming strings\")),\n+\t\tOPT_BOOL_NONEG(0, \"no-name\", &no_name, N_(\"suppress naming strings\")),\n \t\tOPT_BOOL(0, \"current\", &with_current_branch,\n \t\t\t N_(\"include the current branch\")),\n \t\tOPT_BOOL(0, \"sha1-name\", &sha1_name,\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 0b2ecf41ae..b9622b2136 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -361,7 +361,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\n-\t\tOPT_BOOL( 0 , \"no-deref\", &no_deref,\n+\t\tOPT_BOOL_NONEG( 0 , \"no-deref\", &no_deref,\n \t\t\t\t\tN_(\"update <refname> not the one it points to\")),\n \t\tOPT_BOOL('z', NULL, &end_null, N_(\"stdin has NUL-terminated arguments\")),\n \t\tOPT_BOOL( 0 , \"stdin\", &read_stdin, N_(\"read updates from stdin\")),\ndiff --git a/parse-options.h b/parse-options.h\nindex af711227ae..8211d2d5f1 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -118,13 +118,20 @@ struct option {\n #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }\n #define OPT_BIT(s, l, v, h, b)      { OPTION_BIT, (s), (l), (v), NULL, (h), \\\n \t\t\t\t      PARSE_OPT_NOARG, NULL, (b) }\n+#define OPT_BIT_NONEG(s, l, v, h, b) { OPTION_BIT, (s), (l), (v), NULL, (h), \\\n+\t\t\t\t       PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }\n #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (b) }\n+#define OPT_NEGBIT_NONEG(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \\\n+\t\t\t\t\t    (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }\n #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG }\n #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (i) }\n+#define OPT_SET_INT_NONEG(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \\\n+\t\t\t\t            (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)\n+#define OPT_BOOL_NONEG(s, l, v, h)  OPT_SET_INT_NONEG(s, l, v, h, 1)\n #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \\\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex a01430c24b..f6517a9305 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -105,6 +105,7 @@ int cmd_main(int argc, const char **argv)\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"yes\", &boolean, \"get a boolean\"),\n \t\tOPT_BOOL('D', \"no-doubt\", &boolean, \"begins with 'no-'\"),\n+\t\tOPT_BOOL_NONEG(0, \"no-neg\", &boolean, \"begins with 'no-', can't be negated\"),\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\"),\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 74d2cd76fe..e571746775 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -12,6 +12,7 @@ usage: test-parse-options <options>\n \n     --yes                 get a boolean\n     -D, --no-doubt        begins with 'no-'\n+    --no-neg              begins with 'no-', can't be negated\n     -B, --no-fear         be brave\n     -b, --boolean         increment by one\n     -4, --or4             bitwise-or boolean with ...0100\n@@ -80,6 +81,7 @@ check_unknown_i18n() {\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() #2' 'check boolean: 1 --no-neg'\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@@ -89,6 +91,7 @@ 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 #3' 'test_must_fail test-parse-options --no-no-neg'\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-- \n2.11.0\n\n"},{"id":"317146","messageId":"de79d366-3609-8efe-b23a-7e793036b3ef@web.de","threadId":"45734","inReplyTo":"20170418170914.9701-1-avarab@gmail.com","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-18T22:29:18Z","receivedAt":"2017-04-18T22:29:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:\n> Change various --no-OPT options which don't supply PARSE_OPT_NONEG to\n> make --no-no-OPT an error.\n> \n> All of these worked before this change, e.g. doing cloning by doing\n> \"git clone --no-no-checkout\" is equivalent to just \"git clone\", but\n> this was never intended, and is inconsistent with other --no-OPT\n> options which do pass PARSE_OPT_NONEG.\n\nFirst: Why does that bother you, i.e. what's the harm?\n\nSetting PARSE_OPT_NONEG takes away the ability to toggle the affected\noption.  E.g. git clone would reject --checkout.  Currently users can\nspecify --no- options as defaults in aliases and override them on the\ncommand line if needed, with the patch that won't be possible anymore.\n\nPARSE_OPT_NONEG should only be used for options where a negation doesn't\nmake sense, e.g. for the --stage option of checkout-index.\n\nAlso: https://public-inbox.org/git/4F4D3545.6060704@lsrfire.ath.cx/\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> \n> For this one I'm not bothering to track down & CC every single person\n> who originally added these --no-OPT options. Clearly just a trivial\n> bug we can fix.\n> \n>   apply.c                       | 2 +-\n>   builtin/bisect--helper.c      | 2 +-\n>   builtin/check-ignore.c        | 2 +-\n>   builtin/checkout-index.c      | 2 +-\n>   builtin/clone.c               | 4 ++--\n>   builtin/commit.c              | 4 ++--\n>   builtin/fast-export.c         | 2 +-\n>   builtin/grep.c                | 2 +-\n>   builtin/hash-object.c         | 2 +-\n>   builtin/log.c                 | 4 ++--\n>   builtin/push.c                | 2 +-\n>   builtin/read-tree.c           | 2 +-\n>   builtin/revert.c              | 2 +-\n>   builtin/show-branch.c         | 2 +-\n>   builtin/update-ref.c          | 2 +-\n>   parse-options.h               | 7 +++++++\n>   t/helper/test-parse-options.c | 1 +\n>   t/t0040-parse-options.sh      | 3 +++\n>   18 files changed, 29 insertions(+), 18 deletions(-)\n> \n> diff --git a/apply.c b/apply.c\n> index e6dbab26ad..47a91d0762 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4917,7 +4917,7 @@ int apply_parse_options(int argc, const char **argv,\n>   \t\t{ OPTION_CALLBACK, 'p', NULL, state, N_(\"num\"),\n>   \t\t\tN_(\"remove <num> leading slashes from traditional diff paths\"),\n>   \t\t\t0, apply_option_parse_p },\n> -\t\tOPT_BOOL(0, \"no-add\", &state->no_add,\n> +\t\tOPT_BOOL_NONEG(0, \"no-add\", &state->no_add,\n>   \t\t\tN_(\"ignore additions made by the patch\")),\n>   \t\tOPT_BOOL(0, \"stat\", &state->diffstat,\n>   \t\t\tN_(\"instead of applying the patch, output diffstat for the input\")),\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 3324229025..5fe9093947 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -15,7 +15,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n>   \tstruct option options[] = {\n>   \t\tOPT_BOOL(0, \"next-all\", &next_all,\n>   \t\t\t N_(\"perform 'git bisect next'\")),\n> -\t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n> +\t\tOPT_BOOL_NONEG(0, \"no-checkout\", &no_checkout,\n>   \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n>   \t\tOPT_END()\n>   \t};\n> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\n> index 1d73d3ca3d..cb6467946e 100644\n> --- a/builtin/check-ignore.c\n> +++ b/builtin/check-ignore.c\n> @@ -24,7 +24,7 @@ static const struct option check_ignore_options[] = {\n>   \t\t N_(\"terminate input and output records by a NUL character\")),\n>   \tOPT_BOOL('n', \"non-matching\", &show_non_matching,\n>   \t\t N_(\"show non-matching input paths\")),\n> -\tOPT_BOOL(0, \"no-index\", &no_index,\n> +\tOPT_BOOL_NONEG(0, \"no-index\", &no_index,\n>   \t\t N_(\"ignore index when checking\")),\n>   \tOPT_END()\n>   };\n> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> index 07631d0c9c..cf84e31ce8 100644\n> --- a/builtin/checkout-index.c\n> +++ b/builtin/checkout-index.c\n> @@ -161,7 +161,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n>   \t\tOPT__FORCE(&force, N_(\"force overwrite of existing files\")),\n>   \t\tOPT__QUIET(&quiet,\n>   \t\t\tN_(\"no warning for existing files and files not in index\")),\n> -\t\tOPT_BOOL('n', \"no-create\", &not_new,\n> +\t\tOPT_BOOL_NONEG('n', \"no-create\", &not_new,\n>   \t\t\tN_(\"don't checkout new files\")),\n>   \t\tOPT_BOOL('u', \"index\", &index_opt,\n>   \t\t\t N_(\"update stat information in the index file\")),\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index de85b85254..32c5843563 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -76,7 +76,7 @@ static struct option builtin_clone_options[] = {\n>   \tOPT__VERBOSITY(&option_verbosity),\n>   \tOPT_BOOL(0, \"progress\", &option_progress,\n>   \t\t N_(\"force progress reporting\")),\n> -\tOPT_BOOL('n', \"no-checkout\", &option_no_checkout,\n> +\tOPT_BOOL_NONEG('n', \"no-checkout\", &option_no_checkout,\n>   \t\t N_(\"don't create a checkout\")),\n>   \tOPT_BOOL(0, \"bare\", &option_bare, N_(\"create a bare repository\")),\n>   \tOPT_HIDDEN_BOOL(0, \"naked\", &option_bare,\n> @@ -85,7 +85,7 @@ static struct option builtin_clone_options[] = {\n>   \t\t N_(\"create a mirror repository (implies bare)\")),\n>   \tOPT_BOOL('l', \"local\", &option_local,\n>   \t\tN_(\"to clone from a local repository\")),\n> -\tOPT_BOOL(0, \"no-hardlinks\", &option_no_hardlinks,\n> +\tOPT_BOOL_NONEG(0, \"no-hardlinks\", &option_no_hardlinks,\n>   \t\t    N_(\"don't use local hardlinks, always copy\")),\n>   \tOPT_BOOL('s', \"shared\", &option_shared,\n>   \t\t    N_(\"setup as shared repository\")),\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 4e288bc513..46830ebfaf 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1607,7 +1607,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>   \t\tOPT_BOOL(0, \"interactive\", &interactive, N_(\"interactively add files\")),\n>   \t\tOPT_BOOL('p', \"patch\", &patch_interactive, N_(\"interactively add changes\")),\n>   \t\tOPT_BOOL('o', \"only\", &only, N_(\"commit only specified files\")),\n> -\t\tOPT_BOOL('n', \"no-verify\", &no_verify, N_(\"bypass pre-commit and commit-msg hooks\")),\n> +\t\tOPT_BOOL_NONEG('n', \"no-verify\", &no_verify, N_(\"bypass pre-commit and commit-msg hooks\")),\n>   \t\tOPT_BOOL(0, \"dry-run\", &dry_run, N_(\"show what would be committed\")),\n>   \t\tOPT_SET_INT(0, \"short\", &status_format, N_(\"show status concisely\"),\n>   \t\t\t    STATUS_FORMAT_SHORT),\n> @@ -1620,7 +1620,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>   \t\tOPT_BOOL('z', \"null\", &s.null_termination,\n>   \t\t\t N_(\"terminate entries with NUL\")),\n>   \t\tOPT_BOOL(0, \"amend\", &amend, N_(\"amend previous commit\")),\n> -\t\tOPT_BOOL(0, \"no-post-rewrite\", &no_post_rewrite, N_(\"bypass post-rewrite hook\")),\n> +\t\tOPT_BOOL_NONEG(0, \"no-post-rewrite\", &no_post_rewrite, N_(\"bypass post-rewrite hook\")),\n>   \t\t{ OPTION_STRING, 'u', \"untracked-files\", &untracked_files_arg, N_(\"mode\"), N_(\"show untracked files, optional modes: all, normal, no. (Default: all)\"), PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n>   \t\t/* end commit contents options */\n>   \n> diff --git a/builtin/fast-export.c b/builtin/fast-export.c\n> index e0220630d0..dfbd245842 100644\n> --- a/builtin/fast-export.c\n> +++ b/builtin/fast-export.c\n> @@ -994,7 +994,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n>   \t\t\t N_(\"Output full tree for each commit\")),\n>   \t\tOPT_BOOL(0, \"use-done-feature\", &use_done_feature,\n>   \t\t\t     N_(\"Use the done feature to terminate the stream\")),\n> -\t\tOPT_BOOL(0, \"no-data\", &no_data, N_(\"Skip output of blob data\")),\n> +\t\tOPT_BOOL_NONEG(0, \"no-data\", &no_data, N_(\"Skip output of blob data\")),\n>   \t\tOPT_STRING_LIST(0, \"refspec\", &refspecs_list, N_(\"refspec\"),\n>   \t\t\t     N_(\"Apply refspec to exported refs\")),\n>   \t\tOPT_BOOL(0, \"anonymize\", &anonymize, N_(\"anonymize output\")),\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 65070c52fc..923cc570e6 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -978,7 +978,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>   \tstruct option options[] = {\n>   \t\tOPT_BOOL(0, \"cached\", &cached,\n>   \t\t\tN_(\"search in index instead of in the work tree\")),\n> -\t\tOPT_NEGBIT(0, \"no-index\", &use_index,\n> +\t\tOPT_NEGBIT_NONEG(0, \"no-index\", &use_index,\n>   \t\t\t N_(\"find in contents not managed by git\"), 1),\n>   \t\tOPT_BOOL(0, \"untracked\", &untracked,\n>   \t\t\tN_(\"search in both tracked and untracked files\")),\n> diff --git a/builtin/hash-object.c b/builtin/hash-object.c\n> index bbeaf20bcc..c563f2a9ad 100644\n> --- a/builtin/hash-object.c\n> +++ b/builtin/hash-object.c\n> @@ -96,7 +96,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n>   \t\t\tHASH_WRITE_OBJECT),\n>   \t\tOPT_COUNTUP( 0 , \"stdin\", &hashstdin, N_(\"read the object from stdin\")),\n>   \t\tOPT_BOOL( 0 , \"stdin-paths\", &stdin_paths, N_(\"read file names from stdin\")),\n> -\t\tOPT_BOOL( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n> +\t\tOPT_BOOL_NONEG( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n>   \t\tOPT_BOOL( 0, \"literally\", &literally, N_(\"just hash any random garbage to create corrupt objects for debugging Git\")),\n>   \t\tOPT_STRING( 0 , \"path\", &vpath, N_(\"file\"), N_(\"process file as it were from this path\")),\n>   \t\tOPT_END()\n> diff --git a/builtin/log.c b/builtin/log.c\n> index b3b10cc1ed..9824e137ec 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1416,7 +1416,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>   \t\t\t    PARSE_OPT_NOARG, numbered_callback },\n>   \t\t{ OPTION_CALLBACK, 'N', \"no-numbered\", &numbered, NULL,\n>   \t\t\t    N_(\"use [PATCH] even with multiple patches\"),\n> -\t\t\t    PARSE_OPT_NOARG, no_numbered_callback },\n> +\t\t\t    PARSE_OPT_NOARG | PARSE_OPT_NONEG , no_numbered_callback },\n>   \t\tOPT_BOOL('s', \"signoff\", &do_signoff, N_(\"add Signed-off-by:\")),\n>   \t\tOPT_BOOL(0, \"stdout\", &use_stdout,\n>   \t\t\t    N_(\"print patches to standard out\")),\n> @@ -1442,7 +1442,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>   \t\t{ OPTION_CALLBACK, 'k', \"keep-subject\", &rev, NULL,\n>   \t\t\t    N_(\"don't strip/add [PATCH]\"),\n>   \t\t\t    PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback },\n> -\t\tOPT_BOOL(0, \"no-binary\", &no_binary_diff,\n> +\t\tOPT_BOOL_NONEG(0, \"no-binary\", &no_binary_diff,\n>   \t\t\t N_(\"don't output binary diffs\")),\n>   \t\tOPT_BOOL(0, \"zero-commit\", &zero_commit,\n>   \t\t\t N_(\"output all-zero hash in From header\")),\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 5c22e9f2e5..216307c0fa 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -539,7 +539,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>   \t\tOPT_BOOL(0, \"progress\", &progress, N_(\"force progress reporting\")),\n>   \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune locally removed refs\"),\n>   \t\t\tTRANSPORT_PUSH_PRUNE),\n> -\t\tOPT_BIT(0, \"no-verify\", &flags, N_(\"bypass pre-push hook\"), TRANSPORT_PUSH_NO_HOOK),\n> +\t\tOPT_BIT_NONEG(0, \"no-verify\", &flags, N_(\"bypass pre-push hook\"), TRANSPORT_PUSH_NO_HOOK),\n>   \t\tOPT_BIT(0, \"follow-tags\", &flags, N_(\"push missing but relevant tags\"),\n>   \t\t\tTRANSPORT_PUSH_FOLLOW_TAGS),\n>   \t\t{ OPTION_CALLBACK,\n> diff --git a/builtin/read-tree.c b/builtin/read-tree.c\n> index 23e212ee8c..e9359ecb1e 100644\n> --- a/builtin/read-tree.c\n> +++ b/builtin/read-tree.c\n> @@ -153,7 +153,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n>   \t\tOPT_BOOL('i', NULL, &opts.index_only,\n>   \t\t\t N_(\"don't check the working tree after merging\")),\n>   \t\tOPT__DRY_RUN(&opts.dry_run, N_(\"don't update the index or the work tree\")),\n> -\t\tOPT_BOOL(0, \"no-sparse-checkout\", &opts.skip_sparse_checkout,\n> +\t\tOPT_BOOL_NONEG(0, \"no-sparse-checkout\", &opts.skip_sparse_checkout,\n>   \t\t\t N_(\"skip applying sparse checkout filter\")),\n>   \t\tOPT_BOOL(0, \"debug-unpack\", &opts.debug_unpack,\n>   \t\t\t N_(\"debug unpack-trees\")),\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 345d9586a7..e708fdc228 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -98,7 +98,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)\n>   \t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), 'q'),\n>   \t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n>   \t\tOPT_CMDMODE(0, \"abort\", &cmd, N_(\"cancel revert or cherry-pick sequence\"), 'a'),\n> -\t\tOPT_BOOL('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n> +\t\tOPT_BOOL_NONEG('n', \"no-commit\", &opts->no_commit, N_(\"don't automatically commit\")),\n>   \t\tOPT_BOOL('e', \"edit\", &opts->edit, N_(\"edit the commit message\")),\n>   \t\tOPT_NOOP_NOARG('r', NULL),\n>   \t\tOPT_BOOL('s', \"signoff\", &opts->signoff, N_(\"add Signed-off-by:\")),\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 19756595d5..6827846b54 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -638,7 +638,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>   \t\t\t    N_(\"show <n> more commits after the common ancestor\"),\n>   \t\t\t    PARSE_OPT_OPTARG, NULL, (intptr_t)1 },\n>   \t\tOPT_SET_INT(0, \"list\", &extra, N_(\"synonym to more=-1\"), -1),\n> -\t\tOPT_BOOL(0, \"no-name\", &no_name, N_(\"suppress naming strings\")),\n> +\t\tOPT_BOOL_NONEG(0, \"no-name\", &no_name, N_(\"suppress naming strings\")),\n>   \t\tOPT_BOOL(0, \"current\", &with_current_branch,\n>   \t\t\t N_(\"include the current branch\")),\n>   \t\tOPT_BOOL(0, \"sha1-name\", &sha1_name,\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 0b2ecf41ae..b9622b2136 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -361,7 +361,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n>   \tstruct option options[] = {\n>   \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n>   \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\n> -\t\tOPT_BOOL( 0 , \"no-deref\", &no_deref,\n> +\t\tOPT_BOOL_NONEG( 0 , \"no-deref\", &no_deref,\n>   \t\t\t\t\tN_(\"update <refname> not the one it points to\")),\n>   \t\tOPT_BOOL('z', NULL, &end_null, N_(\"stdin has NUL-terminated arguments\")),\n>   \t\tOPT_BOOL( 0 , \"stdin\", &read_stdin, N_(\"read updates from stdin\")),\n> diff --git a/parse-options.h b/parse-options.h\n> index af711227ae..8211d2d5f1 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -118,13 +118,20 @@ struct option {\n>   #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }\n>   #define OPT_BIT(s, l, v, h, b)      { OPTION_BIT, (s), (l), (v), NULL, (h), \\\n>   \t\t\t\t      PARSE_OPT_NOARG, NULL, (b) }\n> +#define OPT_BIT_NONEG(s, l, v, h, b) { OPTION_BIT, (s), (l), (v), NULL, (h), \\\n> +\t\t\t\t       PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }\n>   #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \\\n>   \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (b) }\n> +#define OPT_NEGBIT_NONEG(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \\\n> +\t\t\t\t\t    (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }\n>   #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \\\n>   \t\t\t\t      (h), PARSE_OPT_NOARG }\n>   #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \\\n>   \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (i) }\n> +#define OPT_SET_INT_NONEG(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \\\n> +\t\t\t\t            (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (i) }\n\nThere's a whitespace error here ^^^^^^^^ according to git am.\n\n>   #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)\n> +#define OPT_BOOL_NONEG(s, l, v, h)  OPT_SET_INT_NONEG(s, l, v, h, 1)\n>   #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \\\n>   \t\t\t\t      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}\n>   #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n> diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\n> index a01430c24b..f6517a9305 100644\n> --- a/t/helper/test-parse-options.c\n> +++ b/t/helper/test-parse-options.c\n> @@ -105,6 +105,7 @@ int cmd_main(int argc, const char **argv)\n>   \tstruct option options[] = {\n>   \t\tOPT_BOOL(0, \"yes\", &boolean, \"get a boolean\"),\n>   \t\tOPT_BOOL('D', \"no-doubt\", &boolean, \"begins with 'no-'\"),\n> +\t\tOPT_BOOL_NONEG(0, \"no-neg\", &boolean, \"begins with 'no-', can't be negated\"),\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> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index 74d2cd76fe..e571746775 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -12,6 +12,7 @@ usage: test-parse-options <options>\n>   \n>       --yes                 get a boolean\n>       -D, --no-doubt        begins with 'no-'\n> +    --no-neg              begins with 'no-', can't be negated\n>       -B, --no-fear         be brave\n>       -b, --boolean         increment by one\n>       -4, --or4             bitwise-or boolean with ...0100\n> @@ -80,6 +81,7 @@ check_unknown_i18n() {\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() #2' 'check boolean: 1 --no-neg'\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> @@ -89,6 +91,7 @@ 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 #3' 'test_must_fail test-parse-options --no-no-neg'\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> \n"},{"id":"317171","messageId":"20170419014143.56io56xn6mawy5xi@sigill.intra.peff.net","threadId":"45734","inReplyTo":"de79d366-3609-8efe-b23a-7e793036b3ef@web.de","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-19T01:41:43Z","receivedAt":"2017-04-19T01:41:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 19, 2017 at 12:29:18AM +0200, René Scharfe wrote:\n\n> Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:\n> > Change various --no-OPT options which don't supply PARSE_OPT_NONEG to\n> > make --no-no-OPT an error.\n> > \n> > All of these worked before this change, e.g. doing cloning by doing\n> > \"git clone --no-no-checkout\" is equivalent to just \"git clone\", but\n> > this was never intended, and is inconsistent with other --no-OPT\n> > options which do pass PARSE_OPT_NONEG.\n> \n> First: Why does that bother you, i.e. what's the harm?\n> \n> Setting PARSE_OPT_NONEG takes away the ability to toggle the affected\n> option.  E.g. git clone would reject --checkout.  Currently users can\n> specify --no- options as defaults in aliases and override them on the\n> command line if needed, with the patch that won't be possible anymore.\n> \n> PARSE_OPT_NONEG should only be used for options where a negation doesn't\n> make sense, e.g. for the --stage option of checkout-index.\n\nI think we do strive to avoid \"--no-no-foo\", and instead have \"--no-foo\"\nand \"--foo\" to cover both sides.  So for example:\n\n> > -\t\tOPT_BOOL(0, \"no-add\", &state->no_add,\n> > +\t\tOPT_BOOL_NONEG(0, \"no-add\", &state->no_add,\n> >   \t\t\tN_(\"ignore additions made by the patch\")),\n\nThis could be more like:\n\n  OPT_NEGBOOL(0, \"add\", &state->no_add, ...)\n\nwhere NEGBOOL would be smart enough to show \"--no-add\" in the help as\nthe primary. It might even be possible to detect the existing line and\nhave parse-options automatically respect \"--foo\" when \"--no-foo\" is\npresent.  But that may run afoul of callers that add both \"--foo\" and\n\"--no-foo\" manually.\n\n-Peff\n"},{"id":"317175","messageId":"xmqqh91lce6y.fsf@gitster.mtv.corp.google.com","threadId":"45734","inReplyTo":"20170419014143.56io56xn6mawy5xi@sigill.intra.peff.net","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T02:40:37Z","receivedAt":"2017-04-19T02:40:48Z","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> On Wed, Apr 19, 2017 at 12:29:18AM +0200, René Scharfe wrote:\n> ...\n>> PARSE_OPT_NONEG should only be used for options where a negation doesn't\n>> make sense, e.g. for the --stage option of checkout-index.\n>\n> I think we do strive to avoid \"--no-no-foo\", and instead have \"--no-foo\"\n> and \"--foo\" to cover both sides.  So for example:\n>\n>> > -\t\tOPT_BOOL(0, \"no-add\", &state->no_add,\n>> > +\t\tOPT_BOOL_NONEG(0, \"no-add\", &state->no_add,\n>> >   \t\t\tN_(\"ignore additions made by the patch\")),\n>\n> This could be more like:\n>\n>   OPT_NEGBOOL(0, \"add\", &state->no_add, ...)\n>\n> where NEGBOOL would be smart enough to show \"--no-add\" in the help as\n> the primary.\n\nI very much appreciate that this topic to avoid --no-no-OPT\nnonsense, but just disabling --no-no-OPT without giving --OPT the\nmeaning the user who would have used --no-no-OPT wanted does not\nsound like a good solution.  Your NEGBOOL looks like a better\napproach.\n\n> It might even be possible to detect the existing line and\n> have parse-options automatically respect \"--foo\" when \"--no-foo\" is\n> present.  But that may run afoul of callers that add both \"--foo\" and\n> \"--no-foo\" manually.\n\nTrue but wouldn't that something we would want to avoid anyway?\nThat is, \"git cmd [--OPT | --no-OPT | --no-no-OPT]\" from the end\nuser's point of view should be an error because it is unclear what\ndifference there are between --OPT and --no-no-OPT.  And we should\nbe able to add a rule to parse_options_check() to catch such an\nerror.\n\nHaving said that, I am not sure if we want to go the route of\n\"existing line that begins with 'no-' behaves magical\".  For\nboolean, I suspect we may be get away with such a magic without\nconfusing ourselves too much, though.\n\nThanks.\n"},{"id":"317178","messageId":"20170419025016.we3tfuvgppuamc7g@sigill.intra.peff.net","threadId":"45734","inReplyTo":"xmqqh91lce6y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-19T02:50:16Z","receivedAt":"2017-04-19T02:50:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:\n\n> > It might even be possible to detect the existing line and\n> > have parse-options automatically respect \"--foo\" when \"--no-foo\" is\n> > present.  But that may run afoul of callers that add both \"--foo\" and\n> > \"--no-foo\" manually.\n> \n> True but wouldn't that something we would want to avoid anyway?\n> That is, \"git cmd [--OPT | --no-OPT | --no-no-OPT]\" from the end\n> user's point of view should be an error because it is unclear what\n> difference there are between --OPT and --no-no-OPT.  And we should\n> be able to add a rule to parse_options_check() to catch such an\n> error.\n\nI meant that if you have something like this in your options array:\n\n  { 0, \"foo\", OPTION_INTEGER, &foo, 1 },\n  { 0, \"no-foo\", OPTION_INTEGER, &foo, 2 },\n\nthat if we start magically treating \"--no-foo\" magically, it will\nconflict with \"--foo\" (in this case that's maybe OK because --foo comes\nfirst, but as a general rule it's dangerous to existing options arrays).\n\n> Having said that, I am not sure if we want to go the route of\n> \"existing line that begins with 'no-' behaves magical\".  For\n> boolean, I suspect we may be get away with such a magic without\n> confusing ourselves too much, though.\n\nYeah, at which point we might as well ask callers to explicitly ask for\nthe behavior with OPT_NEGBOOL.\n\n-Peff\n"},{"id":"317196","messageId":"CACBZZX6XWXjvDKeR+a1Ymb4csXgOKLiF==VkKLp5S9TRVQhgoA@mail.gmail.com","threadId":"45734","inReplyTo":"de79d366-3609-8efe-b23a-7e793036b3ef@web.de","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-19T07:00:26Z","receivedAt":"2017-04-19T07:00:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Apr 19, 2017 at 12:29 AM, René Scharfe <l.s.r@web.de> wrote:\n> Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> Change various --no-OPT options which don't supply PARSE_OPT_NONEG to\n>> make --no-no-OPT an error.\n>>\n>> All of these worked before this change, e.g. doing cloning by doing\n>> \"git clone --no-no-checkout\" is equivalent to just \"git clone\", but\n>> this was never intended, and is inconsistent with other --no-OPT\n>> options which do pass PARSE_OPT_NONEG.\n>\n>\n> First: Why does that bother you, i.e. what's the harm?\n\nIt's just unintended emergent behavior I noticed. I.e. I was hacking\nup clone.c and wondering if --no-no-tags for my new --no-tags would\nerror out, it didn't, but it should.\n\n> Setting PARSE_OPT_NONEG takes away the ability to toggle the affected\n> option.  E.g. git clone would reject --checkout.  Currently users can\n> specify --no- options as defaults in aliases and override them on the\n> command line if needed, with the patch that won't be possible anymore.\n>\n> PARSE_OPT_NONEG should only be used for options where a negation doesn't\n> make sense, e.g. for the --stage option of checkout-index.\n\nThat's a bad bug, I don't know whether to be surprised or not that we\nhad no tests for this :)\n\nI thought I was just disabling --no-no-checkout for --no-checkout, not\n--checkout, but didn't notice the subtleties of the special case\nhandling for --no-* in parse-options.c, thanks.\n\n> Also: https://public-inbox.org/git/4F4D3545.6060704@lsrfire.ath.cx/\n\nAh, so this whole thing has been discussed before & rejected. I'll\njust drop it too and use OPT_BOOL() in v2 of my other patch. Thanks.\n"},{"id":"317197","messageId":"CACBZZX5wHvWgqjBaiD4pJsVQug=+-4xRwkvfZ0uZ=meE3xR6NQ@mail.gmail.com","threadId":"45734","inReplyTo":"20170419025016.we3tfuvgppuamc7g@sigill.intra.peff.net","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-19T07:02:52Z","receivedAt":"2017-04-19T07:03:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Apr 19, 2017 at 4:50 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:\n>\n>> > It might even be possible to detect the existing line and\n>> > have parse-options automatically respect \"--foo\" when \"--no-foo\" is\n>> > present.  But that may run afoul of callers that add both \"--foo\" and\n>> > \"--no-foo\" manually.\n>>\n>> True but wouldn't that something we would want to avoid anyway?\n>> That is, \"git cmd [--OPT | --no-OPT | --no-no-OPT]\" from the end\n>> user's point of view should be an error because it is unclear what\n>> difference there are between --OPT and --no-no-OPT.  And we should\n>> be able to add a rule to parse_options_check() to catch such an\n>> error.\n>\n> I meant that if you have something like this in your options array:\n>\n>   { 0, \"foo\", OPTION_INTEGER, &foo, 1 },\n>   { 0, \"no-foo\", OPTION_INTEGER, &foo, 2 },\n\nI may be missing something, but don't we already do exactly what\nyou're describing here? See commit f1930c587 (\"parse-options: allow\npositivation of options starting, with no-\", 2012-02-25) from René\nScharfe.\n"},{"id":"317204","messageId":"CA+P7+xp-VzyqH2Sn9i0fO4rwszEo6NbTMsdTah+Z+zrc5VR4_g@mail.gmail.com","threadId":"45734","inReplyTo":"CACBZZX5wHvWgqjBaiD4pJsVQug=+-4xRwkvfZ0uZ=meE3xR6NQ@mail.gmail.com","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-19T09:08:11Z","receivedAt":"2017-04-19T09:08:43Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Apr 19, 2017 at 12:02 AM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Wed, Apr 19, 2017 at 4:50 AM, Jeff King <peff@peff.net> wrote:\n>> On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:\n>>\n>>> > It might even be possible to detect the existing line and\n>>> > have parse-options automatically respect \"--foo\" when \"--no-foo\" is\n>>> > present.  But that may run afoul of callers that add both \"--foo\" and\n>>> > \"--no-foo\" manually.\n>>>\n>>> True but wouldn't that something we would want to avoid anyway?\n>>> That is, \"git cmd [--OPT | --no-OPT | --no-no-OPT]\" from the end\n>>> user's point of view should be an error because it is unclear what\n>>> difference there are between --OPT and --no-no-OPT.  And we should\n>>> be able to add a rule to parse_options_check() to catch such an\n>>> error.\n>>\n>> I meant that if you have something like this in your options array:\n>>\n>>   { 0, \"foo\", OPTION_INTEGER, &foo, 1 },\n>>   { 0, \"no-foo\", OPTION_INTEGER, &foo, 2 },\n>\n> I may be missing something, but don't we already do exactly what\n> you're describing here? See commit f1930c587 (\"parse-options: allow\n> positivation of options starting, with no-\", 2012-02-25) from René\n> Scharfe.\n\nCorrect, but if you pass the NONEG flag to the option then it will no\nlonger perform this type of negation either.\n\nThanks,\nJake\n"},{"id":"317228","messageId":"b4eef4c1-cd5a-aa71-fc79-8af89dbe365d@web.de","threadId":"45734","inReplyTo":"CACBZZX6XWXjvDKeR+a1Ymb4csXgOKLiF==VkKLp5S9TRVQhgoA@mail.gmail.com","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-19T13:11:01Z","receivedAt":"2017-04-19T13:11:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.04.2017 um 09:00 schrieb Ævar Arnfjörð Bjarmason:\n> On Wed, Apr 19, 2017 at 12:29 AM, René Scharfe <l.s.r@web.de> wrote:\n>> Setting PARSE_OPT_NONEG takes away the ability to toggle the affected\n>> option.  E.g. git clone would reject --checkout.  Currently users can\n>> specify --no- options as defaults in aliases and override them on the\n>> command line if needed, with the patch that won't be possible anymore.\n>>\n>> PARSE_OPT_NONEG should only be used for options where a negation doesn't\n>> make sense, e.g. for the --stage option of checkout-index.\n> \n> That's a bad bug, I don't know whether to be surprised or not that we\n> had no tests for this :)\n> \n> I thought I was just disabling --no-no-checkout for --no-checkout, not\n> --checkout, but didn't notice the subtleties of the special case\n> handling for --no-* in parse-options.c, thanks.\n\nI'm confused.  What's the bug here?\n\n--no-no-checkout is undocumented; Jacob's patch addresses it. \n--no-checkout is the documented form.  Negation allows --checkout to be \nused as well, with the opposite meaning to --no-checkout.  Turning off \nnegation with PARSE_OPT_NONEG forbids --checkout to be used.\n\nPerhaps the issue is that a single line of documentation is not enough \n(\"PARSE_OPT_NONEG: says that this option cannot be negated\")?\n\nRené\n"},{"id":"317236","messageId":"CACBZZX7Y2cqabLMkx35ZU88Y3+8zffP5hxa8haThMaErbfhcfA@mail.gmail.com","threadId":"45734","inReplyTo":"b4eef4c1-cd5a-aa71-fc79-8af89dbe365d@web.de","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-19T13:19:42Z","receivedAt":"2017-04-19T13:20:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Apr 19, 2017 at 3:11 PM, René Scharfe <l.s.r@web.de> wrote:\n> Am 19.04.2017 um 09:00 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Wed, Apr 19, 2017 at 12:29 AM, René Scharfe <l.s.r@web.de> wrote:\n>>>\n>>> Setting PARSE_OPT_NONEG takes away the ability to toggle the affected\n>>> option.  E.g. git clone would reject --checkout.  Currently users can\n>>> specify --no- options as defaults in aliases and override them on the\n>>> command line if needed, with the patch that won't be possible anymore.\n>>>\n>>> PARSE_OPT_NONEG should only be used for options where a negation doesn't\n>>> make sense, e.g. for the --stage option of checkout-index.\n>>\n>>\n>> That's a bad bug, I don't know whether to be surprised or not that we\n>> had no tests for this :)\n>>\n>> I thought I was just disabling --no-no-checkout for --no-checkout, not\n>> --checkout, but didn't notice the subtleties of the special case\n>> handling for --no-* in parse-options.c, thanks.\n>\n>\n> I'm confused.  What's the bug here?\n>\n> --no-no-checkout is undocumented; Jacob's patch addresses it. --no-checkout\n> is the documented form.  Negation allows --checkout to be used as well, with\n> the opposite meaning to --no-checkout.  Turning off negation with\n> PARSE_OPT_NONEG forbids --checkout to be used.\n>\n> Perhaps the issue is that a single line of documentation is not enough\n> (\"PARSE_OPT_NONEG: says that this option cannot be negated\")?\n\nI mean a bug in my patch, i.e. I meant to remove --no-no-OPT in cases\nof --no-OPT but also removed --OPT unintentionally, but anyway, let's\ndrop this one, Jacob's patch is better.\n"},{"id":"317237","messageId":"6d9cedc4-3d3d-61a8-080c-98126c555e1b@web.de","threadId":"45734","inReplyTo":"CACBZZX7Y2cqabLMkx35ZU88Y3+8zffP5hxa8haThMaErbfhcfA@mail.gmail.com","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-19T13:44:35Z","receivedAt":"2017-04-19T13:44:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.04.2017 um 15:19 schrieb Ævar Arnfjörð Bjarmason:\n> I mean a bug in my patch, i.e. I meant to remove --no-no-OPT in cases\n> of --no-OPT but also removed --OPT unintentionally, but anyway, let's\n> drop this one, Jacob's patch is better.\n\nAh, OK.\n\nYou also wondered why no tests complained.  Good question.  Would it \nmake sense to test every option *and* its negation?  That would double \nthe number of tests in order to check a parseopt feature.  Hmm, not \nsure.  More test coverage would be good, but I doubt we'd ever arrive at \n100% for this.\n\nIf you had converted --no-doubt in t/helper/test-parse-options.c to \nOPT_BOOL_NONEG instead of or in addition to adding the new flag --no-neg \nthen you'd seen the effect in t0040.\n\nRené\n"},{"id":"317240","messageId":"20170419150556.ycydwyoi27j7veva@sigill.intra.peff.net","threadId":"45734","inReplyTo":"CACBZZX5wHvWgqjBaiD4pJsVQug=+-4xRwkvfZ0uZ=meE3xR6NQ@mail.gmail.com","subject":"Re: [PATCH] various: disallow --no-no-OPT for --no-opt options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-19T15:05:56Z","receivedAt":"2017-04-19T15:06:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 19, 2017 at 09:02:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Apr 19, 2017 at 4:50 AM, Jeff King <peff@peff.net> wrote:\n> > On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:\n> >\n> >> > It might even be possible to detect the existing line and\n> >> > have parse-options automatically respect \"--foo\" when \"--no-foo\" is\n> >> > present.  But that may run afoul of callers that add both \"--foo\" and\n> >> > \"--no-foo\" manually.\n> >>\n> >> True but wouldn't that something we would want to avoid anyway?\n> >> That is, \"git cmd [--OPT | --no-OPT | --no-no-OPT]\" from the end\n> >> user's point of view should be an error because it is unclear what\n> >> difference there are between --OPT and --no-no-OPT.  And we should\n> >> be able to add a rule to parse_options_check() to catch such an\n> >> error.\n> >\n> > I meant that if you have something like this in your options array:\n> >\n> >   { 0, \"foo\", OPTION_INTEGER, &foo, 1 },\n> >   { 0, \"no-foo\", OPTION_INTEGER, &foo, 2 },\n> \n> I may be missing something, but don't we already do exactly what\n> you're describing here? See commit f1930c587 (\"parse-options: allow\n> positivation of options starting, with no-\", 2012-02-25) from René\n> Scharfe.\n\nOh, so we do. I somehow totally missed that.\n\nI think all is well, then. We do accept --no-no-foo still. IMHO it is\nnot that big a deal (as long as we do not advertise it, then it does not\nhurt unless somebody actually tries it). But I do not mind if it goes\naway, as long as the positive --foo form still works (and it sounds from\nJake's response that we'd need to do more than just NONEG there).\n\n-Peff\n"}]}