various: disallow --no-no-OPT for --no-opt options

PATCH In review Ævar Arnfjörð Bjarmason 5 participants 12 messages Apr 18, 2017 – Apr 19, 2017

lore

Share & tools

lore

Ævar Arnfjörð Bjarmason

author

2017-04-18 17:09 UTC #0 lore raw

PATCH 1/1 various: disallow --no-no-OPT for --no-opt options

Change various --no-OPT options which don't supply PARSE_OPT_NONEG to make --no-no-OPT an error.

All of these worked before this change, e.g. doing cloning by doing "git clone --no-no-checkout" is equivalent to just "git clone", but this was never intended, and is inconsistent with other --no-OPT options which do pass PARSE_OPT_NONEG.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---

For this one I'm not bothering to track down & CC every single person who originally added these --no-OPT options. Clearly just a trivial bug we can fix.

 apply.c                       | 2 +-
 builtin/bisect--helper.c      | 2 +-
 builtin/check-ignore.c        | 2 +-
 builtin/checkout-index.c      | 2 +-
 builtin/clone.c               | 4 ++--
 builtin/commit.c              | 4 ++--
 builtin/fast-export.c         | 2 +-
 builtin/grep.c                | 2 +-
 builtin/hash-object.c         | 2 +-
 builtin/log.c                 | 4 ++--
 builtin/push.c                | 2 +-
 builtin/read-tree.c           | 2 +-
 builtin/revert.c              | 2 +-
 builtin/show-branch.c         | 2 +-
 builtin/update-ref.c          | 2 +-
 parse-options.h               | 7 +++++++
 t/helper/test-parse-options.c | 1 +
 t/t0040-parse-options.sh      | 3 +++
 18 files changed, 29 insertions(+), 18 deletions(-)

apply.c +1 -1

index e6dbab26ad..47a91d0762 100644
@@ -4917,7 +4917,7 @@ int apply_parse_options(int argc, const char **argv,
 		{ OPTION_CALLBACK, 'p', NULL, state, N_("num"),
 			N_("remove <num> leading slashes from traditional diff paths"),
 			0, apply_option_parse_p },
-		OPT_BOOL(0, "no-add", &state->no_add,
+		OPT_BOOL_NONEG(0, "no-add", &state->no_add,
 			N_("ignore additions made by the patch")),
 		OPT_BOOL(0, "stat", &state->diffstat,
 			N_("instead of applying the patch, output diffstat for the input")),

builtin/bisect--helper.c +1 -1

index 3324229025..5fe9093947 100644
@@ -15,7 +15,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "next-all", &next_all,
 			 N_("perform 'git bisect next'")),
-		OPT_BOOL(0, "no-checkout", &no_checkout,
+		OPT_BOOL_NONEG(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
 	};

builtin/check-ignore.c +1 -1

index 1d73d3ca3d..cb6467946e 100644
@@ -24,7 +24,7 @@ static const struct option check_ignore_options[] = {
 		 N_("terminate input and output records by a NUL character")),
 	OPT_BOOL('n', "non-matching", &show_non_matching,
 		 N_("show non-matching input paths")),
-	OPT_BOOL(0, "no-index", &no_index,
+	OPT_BOOL_NONEG(0, "no-index", &no_index,
 		 N_("ignore index when checking")),
 	OPT_END()
 };

builtin/checkout-index.c +1 -1

index 07631d0c9c..cf84e31ce8 100644
@@ -161,7 +161,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		OPT__FORCE(&force, N_("force overwrite of existing files")),
 		OPT__QUIET(&quiet,
 			N_("no warning for existing files and files not in index")),
-		OPT_BOOL('n', "no-create", &not_new,
+		OPT_BOOL_NONEG('n', "no-create", &not_new,
 			N_("don't checkout new files")),
 		OPT_BOOL('u', "index", &index_opt,
 			 N_("update stat information in the index file")),

builtin/clone.c +2 -2

index de85b85254..32c5843563 100644
@@ -76,7 +76,7 @@ static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
 		 N_("force progress reporting")),
-	OPT_BOOL('n', "no-checkout", &option_no_checkout,
+	OPT_BOOL_NONEG('n', "no-checkout", &option_no_checkout,
 		 N_("don't create a checkout")),
 	OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
 	OPT_HIDDEN_BOOL(0, "naked", &option_bare,

@@ -85,7 +85,7 @@ static struct option builtin_clone_options[] = {
 		 N_("create a mirror repository (implies bare)")),
 	OPT_BOOL('l', "local", &option_local,
 		N_("to clone from a local repository")),
-	OPT_BOOL(0, "no-hardlinks", &option_no_hardlinks,
+	OPT_BOOL_NONEG(0, "no-hardlinks", &option_no_hardlinks,
 		    N_("don't use local hardlinks, always copy")),
 	OPT_BOOL('s', "shared", &option_shared,
 		    N_("setup as shared repository")),

builtin/commit.c +2 -2

index 4e288bc513..46830ebfaf 100644
@@ -1607,7 +1607,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "interactive", &interactive, N_("interactively add files")),
 		OPT_BOOL('p', "patch", &patch_interactive, N_("interactively add changes")),
 		OPT_BOOL('o', "only", &only, N_("commit only specified files")),
-		OPT_BOOL('n', "no-verify", &no_verify, N_("bypass pre-commit and commit-msg hooks")),
+		OPT_BOOL_NONEG('n', "no-verify", &no_verify, N_("bypass pre-commit and commit-msg hooks")),
 		OPT_BOOL(0, "dry-run", &dry_run, N_("show what would be committed")),
 		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
 			    STATUS_FORMAT_SHORT),

@@ -1620,7 +1620,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('z', "null", &s.null_termination,
 			 N_("terminate entries with NUL")),
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
-		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
+		OPT_BOOL_NONEG(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
 		{ 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" },
 		/* end commit contents options */

builtin/fast-export.c +1 -1

index e0220630d0..dfbd245842 100644
@@ -994,7 +994,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			 N_("Output full tree for each commit")),
 		OPT_BOOL(0, "use-done-feature", &use_done_feature,
 			     N_("Use the done feature to terminate the stream")),
-		OPT_BOOL(0, "no-data", &no_data, N_("Skip output of blob data")),
+		OPT_BOOL_NONEG(0, "no-data", &no_data, N_("Skip output of blob data")),
 		OPT_STRING_LIST(0, "refspec", &refspecs_list, N_("refspec"),
 			     N_("Apply refspec to exported refs")),
 		OPT_BOOL(0, "anonymize", &anonymize, N_("anonymize output")),

builtin/grep.c +1 -1

index 65070c52fc..923cc570e6 100644
@@ -978,7 +978,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
 			N_("search in index instead of in the work tree")),
-		OPT_NEGBIT(0, "no-index", &use_index,
+		OPT_NEGBIT_NONEG(0, "no-index", &use_index,
 			 N_("find in contents not managed by git"), 1),
 		OPT_BOOL(0, "untracked", &untracked,
 			N_("search in both tracked and untracked files")),

builtin/hash-object.c +1 -1

index bbeaf20bcc..c563f2a9ad 100644
@@ -96,7 +96,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 			HASH_WRITE_OBJECT),
 		OPT_COUNTUP( 0 , "stdin", &hashstdin, N_("read the object from stdin")),
 		OPT_BOOL( 0 , "stdin-paths", &stdin_paths, N_("read file names from stdin")),
-		OPT_BOOL( 0 , "no-filters", &no_filters, N_("store file as is without filters")),
+		OPT_BOOL_NONEG( 0 , "no-filters", &no_filters, N_("store file as is without filters")),
 		OPT_BOOL( 0, "literally", &literally, N_("just hash any random garbage to create corrupt objects for debugging Git")),
 		OPT_STRING( 0 , "path", &vpath, N_("file"), N_("process file as it were from this path")),
 		OPT_END()

builtin/log.c +2 -2

index b3b10cc1ed..9824e137ec 100644
@@ -1416,7 +1416,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_NOARG, numbered_callback },
 		{ OPTION_CALLBACK, 'N', "no-numbered", &numbered, NULL,
 			    N_("use [PATCH] even with multiple patches"),
-			    PARSE_OPT_NOARG, no_numbered_callback },
+			    PARSE_OPT_NOARG | PARSE_OPT_NONEG , no_numbered_callback },
 		OPT_BOOL('s', "signoff", &do_signoff, N_("add Signed-off-by:")),
 		OPT_BOOL(0, "stdout", &use_stdout,
 			    N_("print patches to standard out")),

@@ -1442,7 +1442,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 'k', "keep-subject", &rev, NULL,
 			    N_("don't strip/add [PATCH]"),
 			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback },
-		OPT_BOOL(0, "no-binary", &no_binary_diff,
+		OPT_BOOL_NONEG(0, "no-binary", &no_binary_diff,
 			 N_("don't output binary diffs")),
 		OPT_BOOL(0, "zero-commit", &zero_commit,
 			 N_("output all-zero hash in From header")),

builtin/push.c +1 -1

index 5c22e9f2e5..216307c0fa 100644
@@ -539,7 +539,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
 		OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"),
 			TRANSPORT_PUSH_PRUNE),
-		OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
+		OPT_BIT_NONEG(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
 		OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
 			TRANSPORT_PUSH_FOLLOW_TAGS),
 		{ OPTION_CALLBACK,

builtin/read-tree.c +1 -1

index 23e212ee8c..e9359ecb1e 100644
@@ -153,7 +153,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 		OPT_BOOL('i', NULL, &opts.index_only,
 			 N_("don't check the working tree after merging")),
 		OPT__DRY_RUN(&opts.dry_run, N_("don't update the index or the work tree")),
-		OPT_BOOL(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
+		OPT_BOOL_NONEG(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
 			 N_("skip applying sparse checkout filter")),
 		OPT_BOOL(0, "debug-unpack", &opts.debug_unpack,
 			 N_("debug unpack-trees")),

builtin/revert.c +1 -1

index 345d9586a7..e708fdc228 100644
@@ -98,7 +98,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
 		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
-		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
+		OPT_BOOL_NONEG('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
 		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
 		OPT_NOOP_NOARG('r', NULL),
 		OPT_BOOL('s', "signoff", &opts->signoff, N_("add Signed-off-by:")),

builtin/show-branch.c +1 -1

index 19756595d5..6827846b54 100644
@@ -638,7 +638,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			    N_("show <n> more commits after the common ancestor"),
 			    PARSE_OPT_OPTARG, NULL, (intptr_t)1 },
 		OPT_SET_INT(0, "list", &extra, N_("synonym to more=-1"), -1),
-		OPT_BOOL(0, "no-name", &no_name, N_("suppress naming strings")),
+		OPT_BOOL_NONEG(0, "no-name", &no_name, N_("suppress naming strings")),
 		OPT_BOOL(0, "current", &with_current_branch,
 			 N_("include the current branch")),
 		OPT_BOOL(0, "sha1-name", &sha1_name,

builtin/update-ref.c +1 -1

index 0b2ecf41ae..b9622b2136 100644
@@ -361,7 +361,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
 		OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
-		OPT_BOOL( 0 , "no-deref", &no_deref,
+		OPT_BOOL_NONEG( 0 , "no-deref", &no_deref,
 					N_("update <refname> not the one it points to")),
 		OPT_BOOL('z', NULL, &end_null, N_("stdin has NUL-terminated arguments")),
 		OPT_BOOL( 0 , "stdin", &read_stdin, N_("read updates from stdin")),

parse-options.h +7 -0

index af711227ae..8211d2d5f1 100644
@@ -118,13 +118,20 @@ struct option {
 #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
 #define OPT_BIT(s, l, v, h, b)      { OPTION_BIT, (s), (l), (v), NULL, (h), \
 				      PARSE_OPT_NOARG, NULL, (b) }
+#define OPT_BIT_NONEG(s, l, v, h, b) { OPTION_BIT, (s), (l), (v), NULL, (h), \
+				       PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }
 #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
+#define OPT_NEGBIT_NONEG(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
+					    (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG }
 #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_NONEG(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
+				            (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (i) }
 #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
+#define OPT_BOOL_NONEG(s, l, v, h)  OPT_SET_INT_NONEG(s, l, v, h, 1)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \

t/helper/test-parse-options.c +1 -0

index a01430c24b..f6517a9305 100644
@@ -105,6 +105,7 @@ int cmd_main(int argc, const char **argv)
 	struct option options[] = {
 		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
 		OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
+		OPT_BOOL_NONEG(0, "no-neg", &boolean, "begins with 'no-', can't be negated"),
 		{ OPTION_SET_INT, 'B', "no-fear", &boolean, NULL,
 		  "be brave", PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
 		OPT_COUNTUP('b', "boolean", &boolean, "increment by one"),

t/t0040-parse-options.sh +3 -0

index 74d2cd76fe..e571746775 100755
@@ -12,6 +12,7 @@ usage: test-parse-options <options>
 
     --yes                 get a boolean
     -D, --no-doubt        begins with 'no-'
+    --no-neg              begins with 'no-', can't be negated
     -B, --no-fear         be brave
     -b, --boolean         increment by one
     -4, --or4             bitwise-or boolean with ...0100

@@ -80,6 +81,7 @@ check_unknown_i18n() {
 
 test_expect_success 'OPT_BOOL() #1' 'check boolean: 1 --yes'
 test_expect_success 'OPT_BOOL() #2' 'check boolean: 1 --no-doubt'
+test_expect_success 'OPT_BOOL() #2' 'check boolean: 1 --no-neg'
 test_expect_success 'OPT_BOOL() #3' 'check boolean: 1 -D'
 test_expect_success 'OPT_BOOL() #4' 'check boolean: 1 --no-fear'
 test_expect_success 'OPT_BOOL() #5' 'check boolean: 1 -B'

@@ -89,6 +91,7 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'
 
 test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
 test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
+test_expect_success 'OPT_BOOL() negation #3' 'test_must_fail test-parse-options --no-no-neg'
 
 test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
 test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'

Signature · 1 line

2.11.0

René Scharfe

2017-04-18 22:29 UTC #1 lore raw

Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:

Quoted 7 lines

Change various --no-OPT options which don't supply PARSE_OPT_NONEG to make --no-no-OPT an error.
All of these worked before this change, e.g. doing cloning by doing "git clone --no-no-checkout" is equivalent to just "git clone", but this was never intended, and is inconsistent with other --no-OPT options which do pass PARSE_OPT_NONEG.

First: Why does that bother you, i.e. what's the harm?

Setting PARSE_OPT_NONEG takes away the ability to toggle the affected option. E.g. git clone would reject --checkout. Currently users can specify --no- options as defaults in aliases and override them on the command line if needed, with the patch that won't be possible anymore.

PARSE_OPT_NONEG should only be used for options where a negation doesn't make sense, e.g. for the --stage option of checkout-index.

Also: https://public-inbox.org/git/[email protected]/

Quoted 259 lines

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---

For this one I'm not bothering to track down & CC every single person who originally added these --no-OPT options. Clearly just a trivial bug we can fix.

  apply.c                       | 2 +-
  builtin/bisect--helper.c      | 2 +-
  builtin/check-ignore.c        | 2 +-
  builtin/checkout-index.c      | 2 +-
  builtin/clone.c               | 4 ++--
  builtin/commit.c              | 4 ++--
  builtin/fast-export.c         | 2 +-
  builtin/grep.c                | 2 +-
  builtin/hash-object.c         | 2 +-
  builtin/log.c                 | 4 ++--
  builtin/push.c                | 2 +-
  builtin/read-tree.c           | 2 +-
  builtin/revert.c              | 2 +-
  builtin/show-branch.c         | 2 +-
  builtin/update-ref.c          | 2 +-
  parse-options.h               | 7 +++++++
  t/helper/test-parse-options.c | 1 +
  t/t0040-parse-options.sh      | 3 +++
  18 files changed, 29 insertions(+), 18 deletions(-)

apply.c +1 -1

index e6dbab26ad..47a91d0762 100644
@@ -4917,7 +4917,7 @@ int apply_parse_options(int argc, const char **argv,
  		{ OPTION_CALLBACK, 'p', NULL, state, N_("num"),
  			N_("remove <num> leading slashes from traditional diff paths"),
  			0, apply_option_parse_p },
-		OPT_BOOL(0, "no-add", &state->no_add,
+		OPT_BOOL_NONEG(0, "no-add", &state->no_add,
  			N_("ignore additions made by the patch")),
  		OPT_BOOL(0, "stat", &state->diffstat,
  			N_("instead of applying the patch, output diffstat for the input")),

builtin/bisect--helper.c +1 -1

index 3324229025..5fe9093947 100644
@@ -15,7 +15,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
  	struct option options[] = {
  		OPT_BOOL(0, "next-all", &next_all,
  			 N_("perform 'git bisect next'")),
-		OPT_BOOL(0, "no-checkout", &no_checkout,
+		OPT_BOOL_NONEG(0, "no-checkout", &no_checkout,
  			 N_("update BISECT_HEAD instead of checking out the current commit")),
  		OPT_END()
  	};

builtin/check-ignore.c +1 -1

index 1d73d3ca3d..cb6467946e 100644
@@ -24,7 +24,7 @@ static const struct option check_ignore_options[] = {
  		 N_("terminate input and output records by a NUL character")),
  	OPT_BOOL('n', "non-matching", &show_non_matching,
  		 N_("show non-matching input paths")),
-	OPT_BOOL(0, "no-index", &no_index,
+	OPT_BOOL_NONEG(0, "no-index", &no_index,
  		 N_("ignore index when checking")),
  	OPT_END()
  };

builtin/checkout-index.c +1 -1

index 07631d0c9c..cf84e31ce8 100644
@@ -161,7 +161,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
  		OPT__FORCE(&force, N_("force overwrite of existing files")),
  		OPT__QUIET(&quiet,
  			N_("no warning for existing files and files not in index")),
-		OPT_BOOL('n', "no-create", &not_new,
+		OPT_BOOL_NONEG('n', "no-create", &not_new,
  			N_("don't checkout new files")),
  		OPT_BOOL('u', "index", &index_opt,
  			 N_("update stat information in the index file")),

builtin/clone.c +2 -2

index de85b85254..32c5843563 100644
@@ -76,7 +76,7 @@ static struct option builtin_clone_options[] = {
  	OPT__VERBOSITY(&option_verbosity),
  	OPT_BOOL(0, "progress", &option_progress,
  		 N_("force progress reporting")),
-	OPT_BOOL('n', "no-checkout", &option_no_checkout,
+	OPT_BOOL_NONEG('n', "no-checkout", &option_no_checkout,
  		 N_("don't create a checkout")),
  	OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
  	OPT_HIDDEN_BOOL(0, "naked", &option_bare,

@@ -85,7 +85,7 @@ static struct option builtin_clone_options[] = {
  		 N_("create a mirror repository (implies bare)")),
  	OPT_BOOL('l', "local", &option_local,
  		N_("to clone from a local repository")),
-	OPT_BOOL(0, "no-hardlinks", &option_no_hardlinks,
+	OPT_BOOL_NONEG(0, "no-hardlinks", &option_no_hardlinks,
  		    N_("don't use local hardlinks, always copy")),
  	OPT_BOOL('s', "shared", &option_shared,
  		    N_("setup as shared repository")),

builtin/commit.c +2 -2

index 4e288bc513..46830ebfaf 100644
@@ -1607,7 +1607,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
  		OPT_BOOL(0, "interactive", &interactive, N_("interactively add files")),
  		OPT_BOOL('p', "patch", &patch_interactive, N_("interactively add changes")),
  		OPT_BOOL('o', "only", &only, N_("commit only specified files")),
-		OPT_BOOL('n', "no-verify", &no_verify, N_("bypass pre-commit and commit-msg hooks")),
+		OPT_BOOL_NONEG('n', "no-verify", &no_verify, N_("bypass pre-commit and commit-msg hooks")),
  		OPT_BOOL(0, "dry-run", &dry_run, N_("show what would be committed")),
  		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
  			    STATUS_FORMAT_SHORT),

@@ -1620,7 +1620,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
  		OPT_BOOL('z', "null", &s.null_termination,
  			 N_("terminate entries with NUL")),
  		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
-		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
+		OPT_BOOL_NONEG(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
  		{ 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" },
  		/* end commit contents options */

builtin/fast-export.c +1 -1

index e0220630d0..dfbd245842 100644
@@ -994,7 +994,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
  			 N_("Output full tree for each commit")),
  		OPT_BOOL(0, "use-done-feature", &use_done_feature,
  			     N_("Use the done feature to terminate the stream")),
-		OPT_BOOL(0, "no-data", &no_data, N_("Skip output of blob data")),
+		OPT_BOOL_NONEG(0, "no-data", &no_data, N_("Skip output of blob data")),
  		OPT_STRING_LIST(0, "refspec", &refspecs_list, N_("refspec"),
  			     N_("Apply refspec to exported refs")),
  		OPT_BOOL(0, "anonymize", &anonymize, N_("anonymize output")),

builtin/grep.c +1 -1

index 65070c52fc..923cc570e6 100644
@@ -978,7 +978,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
  	struct option options[] = {
  		OPT_BOOL(0, "cached", &cached,
  			N_("search in index instead of in the work tree")),
-		OPT_NEGBIT(0, "no-index", &use_index,
+		OPT_NEGBIT_NONEG(0, "no-index", &use_index,
  			 N_("find in contents not managed by git"), 1),
  		OPT_BOOL(0, "untracked", &untracked,
  			N_("search in both tracked and untracked files")),

builtin/hash-object.c +1 -1

index bbeaf20bcc..c563f2a9ad 100644
@@ -96,7 +96,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
  			HASH_WRITE_OBJECT),
  		OPT_COUNTUP( 0 , "stdin", &hashstdin, N_("read the object from stdin")),
  		OPT_BOOL( 0 , "stdin-paths", &stdin_paths, N_("read file names from stdin")),
-		OPT_BOOL( 0 , "no-filters", &no_filters, N_("store file as is without filters")),
+		OPT_BOOL_NONEG( 0 , "no-filters", &no_filters, N_("store file as is without filters")),
  		OPT_BOOL( 0, "literally", &literally, N_("just hash any random garbage to create corrupt objects for debugging Git")),
  		OPT_STRING( 0 , "path", &vpath, N_("file"), N_("process file as it were from this path")),
  		OPT_END()

builtin/log.c +2 -2

index b3b10cc1ed..9824e137ec 100644
@@ -1416,7 +1416,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
  			    PARSE_OPT_NOARG, numbered_callback },
  		{ OPTION_CALLBACK, 'N', "no-numbered", &numbered, NULL,
  			    N_("use [PATCH] even with multiple patches"),
-			    PARSE_OPT_NOARG, no_numbered_callback },
+			    PARSE_OPT_NOARG | PARSE_OPT_NONEG , no_numbered_callback },
  		OPT_BOOL('s', "signoff", &do_signoff, N_("add Signed-off-by:")),
  		OPT_BOOL(0, "stdout", &use_stdout,
  			    N_("print patches to standard out")),

@@ -1442,7 +1442,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
  		{ OPTION_CALLBACK, 'k', "keep-subject", &rev, NULL,
  			    N_("don't strip/add [PATCH]"),
  			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback },
-		OPT_BOOL(0, "no-binary", &no_binary_diff,
+		OPT_BOOL_NONEG(0, "no-binary", &no_binary_diff,
  			 N_("don't output binary diffs")),
  		OPT_BOOL(0, "zero-commit", &zero_commit,
  			 N_("output all-zero hash in From header")),

builtin/push.c +1 -1

index 5c22e9f2e5..216307c0fa 100644
@@ -539,7 +539,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
  		OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
  		OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"),
  			TRANSPORT_PUSH_PRUNE),
-		OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
+		OPT_BIT_NONEG(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
  		OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
  			TRANSPORT_PUSH_FOLLOW_TAGS),
  		{ OPTION_CALLBACK,

builtin/read-tree.c +1 -1

index 23e212ee8c..e9359ecb1e 100644
@@ -153,7 +153,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
  		OPT_BOOL('i', NULL, &opts.index_only,
  			 N_("don't check the working tree after merging")),
  		OPT__DRY_RUN(&opts.dry_run, N_("don't update the index or the work tree")),
-		OPT_BOOL(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
+		OPT_BOOL_NONEG(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
  			 N_("skip applying sparse checkout filter")),
  		OPT_BOOL(0, "debug-unpack", &opts.debug_unpack,
  			 N_("debug unpack-trees")),

builtin/revert.c +1 -1

index 345d9586a7..e708fdc228 100644
@@ -98,7 +98,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
  		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
  		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
  		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
-		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
+		OPT_BOOL_NONEG('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
  		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
  		OPT_NOOP_NOARG('r', NULL),
  		OPT_BOOL('s', "signoff", &opts->signoff, N_("add Signed-off-by:")),

builtin/show-branch.c +1 -1

index 19756595d5..6827846b54 100644
@@ -638,7 +638,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
  			    N_("show <n> more commits after the common ancestor"),
  			    PARSE_OPT_OPTARG, NULL, (intptr_t)1 },
  		OPT_SET_INT(0, "list", &extra, N_("synonym to more=-1"), -1),
-		OPT_BOOL(0, "no-name", &no_name, N_("suppress naming strings")),
+		OPT_BOOL_NONEG(0, "no-name", &no_name, N_("suppress naming strings")),
  		OPT_BOOL(0, "current", &with_current_branch,
  			 N_("include the current branch")),
  		OPT_BOOL(0, "sha1-name", &sha1_name,

builtin/update-ref.c +1 -1

index 0b2ecf41ae..b9622b2136 100644
@@ -361,7 +361,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
  	struct option options[] = {
  		OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
  		OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
-		OPT_BOOL( 0 , "no-deref", &no_deref,
+		OPT_BOOL_NONEG( 0 , "no-deref", &no_deref,
  					N_("update <refname> not the one it points to")),
  		OPT_BOOL('z', NULL, &end_null, N_("stdin has NUL-terminated arguments")),
  		OPT_BOOL( 0 , "stdin", &read_stdin, N_("read updates from stdin")),

parse-options.h +2 -0

index af711227ae..8211d2d5f1 100644
@@ -118,13 +118,20 @@ struct option {
  #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
  #define OPT_BIT(s, l, v, h, b)      { OPTION_BIT, (s), (l), (v), NULL, (h), \
  				      PARSE_OPT_NOARG, NULL, (b) }
+#define OPT_BIT_NONEG(s, l, v, h, b) { OPTION_BIT, (s), (l), (v), NULL, (h), \
+				       PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }

parse-options.h struct option { · line 132

Quoted 10 lines

parse-options.h +4 -0

  #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
  				      (h), PARSE_OPT_NOARG, NULL, (b) }
+#define OPT_NEGBIT_NONEG(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
+					    (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (b) }
  #define OPT_COUNTUP(s, l, v, h)     { OPTION_COUNTUP, (s), (l), (v), NULL, \
  				      (h), PARSE_OPT_NOARG }
  #define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
  				      (h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_NONEG(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
+				            (h), PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (i) }

There's a whitespace error here ^^^^^^^^ according to git am.

Quoted 46 lines

parse-options.h +1 -0

  #define OPT_BOOL(s, l, v, h)        OPT_SET_INT(s, l, v, h, 1)
+#define OPT_BOOL_NONEG(s, l, v, h)  OPT_SET_INT_NONEG(s, l, v, h, 1)
  #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
  				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
  #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \

t/helper/test-parse-options.c +1 -0

index a01430c24b..f6517a9305 100644
@@ -105,6 +105,7 @@ int cmd_main(int argc, const char **argv)
  	struct option options[] = {
  		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
  		OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
+		OPT_BOOL_NONEG(0, "no-neg", &boolean, "begins with 'no-', can't be negated"),
  		{ OPTION_SET_INT, 'B', "no-fear", &boolean, NULL,
  		  "be brave", PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },
  		OPT_COUNTUP('b', "boolean", &boolean, "increment by one"),

t/t0040-parse-options.sh +3 -0

index 74d2cd76fe..e571746775 100755
@@ -12,6 +12,7 @@ usage: test-parse-options <options>
 
      --yes                 get a boolean
      -D, --no-doubt        begins with 'no-'
+    --no-neg              begins with 'no-', can't be negated
      -B, --no-fear         be brave
      -b, --boolean         increment by one
      -4, --or4             bitwise-or boolean with ...0100

@@ -80,6 +81,7 @@ check_unknown_i18n() {
 
  test_expect_success 'OPT_BOOL() #1' 'check boolean: 1 --yes'
  test_expect_success 'OPT_BOOL() #2' 'check boolean: 1 --no-doubt'
+test_expect_success 'OPT_BOOL() #2' 'check boolean: 1 --no-neg'
  test_expect_success 'OPT_BOOL() #3' 'check boolean: 1 -D'
  test_expect_success 'OPT_BOOL() #4' 'check boolean: 1 --no-fear'
  test_expect_success 'OPT_BOOL() #5' 'check boolean: 1 -B'

@@ -89,6 +91,7 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'
 
  test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
  test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
+test_expect_success 'OPT_BOOL() negation #3' 'test_must_fail test-parse-options --no-no-neg'
 
  test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
  test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'

Jeff King

2017-04-19 01:41 UTC #2 lore raw

Quoted 11 lines · René Scharfe

Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:
Quoted 7 lines
Change various --no-OPT options which don't supply PARSE_OPT_NONEG to make --no-no-OPT an error.
All of these worked before this change, e.g. doing cloning by doing "git clone --no-no-checkout" is equivalent to just "git clone", but this was never intended, and is inconsistent with other --no-OPT options which do pass PARSE_OPT_NONEG.
First: Why does that bother you, i.e. what's the harm?

Quoted 7 lines

Setting PARSE_OPT_NONEG takes away the ability to toggle the affected option. E.g. git clone would reject --checkout. Currently users can specify --no- options as defaults in aliases and override them on the command line if needed, with the patch that won't be possible anymore.
PARSE_OPT_NONEG should only be used for options where a negation doesn't make sense, e.g. for the --stage option of checkout-index.

I think we do strive to avoid "--no-no-foo", and instead have "--no-foo" and "--foo" to cover both sides. So for example:

Quoted 3 lines

apply.c int apply_parse_options(int argc, const char **argv, · line 4921
Quoted 3 lines
apply.c +1 -1
-		OPT_BOOL(0, "no-add", &state->no_add,
+		OPT_BOOL_NONEG(0, "no-add", &state->no_add,
  			N_("ignore additions made by the patch")),

This could be more like:

OPT_NEGBOOL(0, "add", &state->no_add, ...)

where NEGBOOL would be smart enough to show "--no-add" in the help as the primary. It might even be possible to detect the existing line and have parse-options automatically respect "--foo" when "--no-foo" is present. But that may run afoul of callers that add both "--foo" and "--no-foo" manually.

-Peff

Junio C Hamano

2017-04-19 02:40 UTC #3 lore raw

Quoted 14 lines · Jeff King

On Wed, Apr 19, 2017 at 12:29:18AM +0200, René Scharfe wrote:
...
Quoted 2 lines
PARSE_OPT_NONEG should only be used for options where a negation doesn't make sense, e.g. for the --stage option of checkout-index.
I think we do strive to avoid "--no-no-foo", and instead have "--no-foo" and "--foo" to cover both sides. So for example:
Quoted 3 lines
apply.c int apply_parse_options(int argc, const char **argv, · line 4921
Quoted 3 lines
apply.c +1 -1
-		OPT_BOOL(0, "no-add", &state->no_add,
+		OPT_BOOL_NONEG(0, "no-add", &state->no_add,
  			N_("ignore additions made by the patch")),
This could be more like:

Quoted 4 lines

OPT_NEGBOOL(0, "add", &state->no_add, ...)
where NEGBOOL would be smart enough to show "--no-add" in the help as the primary.

I very much appreciate that this topic to avoid --no-no-OPT nonsense, but just disabling --no-no-OPT without giving --OPT the meaning the user who would have used --no-no-OPT wanted does not sound like a good solution. Your NEGBOOL looks like a better approach.

Quoted 4 lines

It might even be possible to detect the existing line and have parse-options automatically respect "--foo" when "--no-foo" is present. But that may run afoul of callers that add both "--foo" and "--no-foo" manually.

True but wouldn't that something we would want to avoid anyway? That is, "git cmd [--OPT | --no-OPT | --no-no-OPT]" from the end user's point of view should be an error because it is unclear what difference there are between --OPT and --no-no-OPT. And we should be able to add a rule to parse_options_check() to catch such an error.

Having said that, I am not sure if we want to go the route of "existing line that begins with 'no-' behaves magical". For boolean, I suspect we may be get away with such a magic without confusing ourselves too much, though.

Thanks.

Jeff King

2017-04-19 02:50 UTC #4 lore raw

Quoted 11 lines · Junio C Hamano

Quoted 4 lines
It might even be possible to detect the existing line and have parse-options automatically respect "--foo" when "--no-foo" is present. But that may run afoul of callers that add both "--foo" and "--no-foo" manually.
True but wouldn't that something we would want to avoid anyway? That is, "git cmd [--OPT | --no-OPT | --no-no-OPT]" from the end user's point of view should be an error because it is unclear what difference there are between --OPT and --no-no-OPT. And we should be able to add a rule to parse_options_check() to catch such an error.

I meant that if you have something like this in your options array:

{ 0, "foo", OPTION_INTEGER, &foo, 1 },
{ 0, "no-foo", OPTION_INTEGER, &foo, 2 },

that if we start magically treating "--no-foo" magically, it will conflict with "--foo" (in this case that's maybe OK because --foo comes first, but as a general rule it's dangerous to existing options arrays).

Quoted 4 lines

Having said that, I am not sure if we want to go the route of "existing line that begins with 'no-' behaves magical". For boolean, I suspect we may be get away with such a magic without confusing ourselves too much, though.

Yeah, at which point we might as well ask callers to explicitly ask for the behavior with OPT_NEGBOOL.

-Peff

Ævar Arnfjörð Bjarmason

author

2017-04-19 07:03 UTC #5 lore raw

Quoted 14 lines · Jeff King

Quoted 11 lines · Junio C Hamano
Quoted 4 lines
It might even be possible to detect the existing line and have parse-options automatically respect "--foo" when "--no-foo" is present. But that may run afoul of callers that add both "--foo" and "--no-foo" manually.
True but wouldn't that something we would want to avoid anyway? That is, "git cmd [--OPT | --no-OPT | --no-no-OPT]" from the end user's point of view should be an error because it is unclear what difference there are between --OPT and --no-no-OPT. And we should be able to add a rule to parse_options_check() to catch such an error.

Quoted 4 lines

I meant that if you have something like this in your options array:
{ 0, "foo", OPTION_INTEGER, &foo, 1 },
{ 0, "no-foo", OPTION_INTEGER, &foo, 2 },

I may be missing something, but don't we already do exactly what you're describing here? See commit f1930c587 ("parse-options: allow positivation of options starting, with no-", 2012-02-25) from René Scharfe.

Jacob Keller

2017-04-19 09:08 UTC #6 lore raw

Quoted 17 lines · Ævar Arnfjörð Bjarmason

Quoted 16 lines · Jeff King
Quoted 11 lines · Junio C Hamano
Quoted 4 lines
It might even be possible to detect the existing line and have parse-options automatically respect "--foo" when "--no-foo" is present. But that may run afoul of callers that add both "--foo" and "--no-foo" manually.
True but wouldn't that something we would want to avoid anyway? That is, "git cmd [--OPT | --no-OPT | --no-no-OPT]" from the end user's point of view should be an error because it is unclear what difference there are between --OPT and --no-no-OPT. And we should be able to add a rule to parse_options_check() to catch such an error.
I meant that if you have something like this in your options array:

Quoted 7 lines

Quoted 2 lines
{ 0, "foo", OPTION_INTEGER, &foo, 1 },
{ 0, "no-foo", OPTION_INTEGER, &foo, 2 },
I may be missing something, but don't we already do exactly what you're describing here? See commit f1930c587 ("parse-options: allow positivation of options starting, with no-", 2012-02-25) from René Scharfe.

Correct, but if you pass the NONEG flag to the option then it will no longer perform this type of negation either.

Thanks,
Jake

Jeff King

2017-04-19 15:06 UTC #7 lore raw

In reply to Ævar Arnfjörð Bjarmason

Quoted 17 lines · Ævar Arnfjörð Bjarmason

Quoted 16 lines · Jeff King
Quoted 11 lines · Junio C Hamano
Quoted 4 lines
It might even be possible to detect the existing line and have parse-options automatically respect "--foo" when "--no-foo" is present. But that may run afoul of callers that add both "--foo" and "--no-foo" manually.
True but wouldn't that something we would want to avoid anyway? That is, "git cmd [--OPT | --no-OPT | --no-no-OPT]" from the end user's point of view should be an error because it is unclear what difference there are between --OPT and --no-no-OPT. And we should be able to add a rule to parse_options_check() to catch such an error.
I meant that if you have something like this in your options array:

Quoted 7 lines

Quoted 2 lines
{ 0, "foo", OPTION_INTEGER, &foo, 1 },
{ 0, "no-foo", OPTION_INTEGER, &foo, 2 },
I may be missing something, but don't we already do exactly what you're describing here? See commit f1930c587 ("parse-options: allow positivation of options starting, with no-", 2012-02-25) from René Scharfe.

Oh, so we do. I somehow totally missed that.

I think all is well, then. We do accept --no-no-foo still. IMHO it is not that big a deal (as long as we do not advertise it, then it does not hurt unless somebody actually tries it). But I do not mind if it goes away, as long as the positive --foo form still works (and it sounds from Jake's response that we'd need to do more than just NONEG there).

-Peff

Ævar Arnfjörð Bjarmason

author

2017-04-19 07:00 UTC #8 lore raw

In reply to René Scharfe

Quoted 5 lines · René Scharfe

Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:
Quoted 4 lines
Change various --no-OPT options which don't supply PARSE_OPT_NONEG to make --no-no-OPT an error.

Quoted 7 lines

Quoted 4 lines
All of these worked before this change, e.g. doing cloning by doing "git clone --no-no-checkout" is equivalent to just "git clone", but this was never intended, and is inconsistent with other --no-OPT options which do pass PARSE_OPT_NONEG.
First: Why does that bother you, i.e. what's the harm?

It's just unintended emergent behavior I noticed. I.e. I was hacking up clone.c and wondering if --no-no-tags for my new --no-tags would error out, it didn't, but it should.

Quoted 7 lines

Setting PARSE_OPT_NONEG takes away the ability to toggle the affected option. E.g. git clone would reject --checkout. Currently users can specify --no- options as defaults in aliases and override them on the command line if needed, with the patch that won't be possible anymore.
PARSE_OPT_NONEG should only be used for options where a negation doesn't make sense, e.g. for the --stage option of checkout-index.

That's a bad bug, I don't know whether to be surprised or not that we had no tests for this :)

I thought I was just disabling --no-no-checkout for --no-checkout, not --checkout, but didn't notice the subtleties of the special case handling for --no-* in parse-options.c, thanks.

Quoted 1 line

Also: https://public-inbox.org/git/[email protected]/

Ah, so this whole thing has been discussed before & rejected. I'll just drop it too and use OPT_BOOL() in v2 of my other patch. Thanks.

René Scharfe

2017-04-19 13:11 UTC #9 lore raw

Am 19.04.2017 um 09:00 schrieb Ævar Arnfjörð Bjarmason:

Quoted 9 lines · René Scharfe

Quoted 7 lines
Setting PARSE_OPT_NONEG takes away the ability to toggle the affected option. E.g. git clone would reject --checkout. Currently users can specify --no- options as defaults in aliases and override them on the command line if needed, with the patch that won't be possible anymore.
PARSE_OPT_NONEG should only be used for options where a negation doesn't make sense, e.g. for the --stage option of checkout-index.

Quoted 6 lines

That's a bad bug, I don't know whether to be surprised or not that we had no tests for this :)
I thought I was just disabling --no-no-checkout for --no-checkout, not --checkout, but didn't notice the subtleties of the special case handling for --no-* in parse-options.c, thanks.

I'm confused. What's the bug here?

--no-no-checkout is undocumented; Jacob's patch addresses it. --no-checkout is the documented form. Negation allows --checkout to be used as well, with the opposite meaning to --no-checkout. Turning off negation with PARSE_OPT_NONEG forbids --checkout to be used.

Perhaps the issue is that a single line of documentation is not enough ("PARSE_OPT_NONEG: says that this option cannot be negated")?

René

Ævar Arnfjörð Bjarmason

author

2017-04-19 13:20 UTC #10 lore raw

Quoted 23 lines · René Scharfe

Am 19.04.2017 um 09:00 schrieb Ævar Arnfjörð Bjarmason:
Quoted 12 lines · René Scharfe
Quoted 8 lines
Setting PARSE_OPT_NONEG takes away the ability to toggle the affected option. E.g. git clone would reject --checkout. Currently users can specify --no- options as defaults in aliases and override them on the command line if needed, with the patch that won't be possible anymore.
PARSE_OPT_NONEG should only be used for options where a negation doesn't make sense, e.g. for the --stage option of checkout-index.
Quoted 6 lines
That's a bad bug, I don't know whether to be surprised or not that we had no tests for this :)
I thought I was just disabling --no-no-checkout for --no-checkout, not --checkout, but didn't notice the subtleties of the special case handling for --no-* in parse-options.c, thanks.
I'm confused. What's the bug here?

Quoted 7 lines

--no-no-checkout is undocumented; Jacob's patch addresses it. --no-checkout is the documented form. Negation allows --checkout to be used as well, with the opposite meaning to --no-checkout. Turning off negation with PARSE_OPT_NONEG forbids --checkout to be used.
Perhaps the issue is that a single line of documentation is not enough ("PARSE_OPT_NONEG: says that this option cannot be negated")?

I mean a bug in my patch, i.e. I meant to remove --no-no-OPT in cases of --no-OPT but also removed --OPT unintentionally, but anyway, let's drop this one, Jacob's patch is better.

René Scharfe

2017-04-19 13:44 UTC #11 lore raw

Am 19.04.2017 um 15:19 schrieb Ævar Arnfjörð Bjarmason:

Quoted 3 lines

I mean a bug in my patch, i.e. I meant to remove --no-no-OPT in cases of --no-OPT but also removed --OPT unintentionally, but anyway, let's drop this one, Jacob's patch is better.

Ah, OK.

You also wondered why no tests complained. Good question. Would it make sense to test every option *and* its negation? That would double the number of tests in order to check a parseopt feature. Hmm, not sure. More test coverage would be good, but I doubt we'd ever arrive at 100% for this.

If you had converted --no-doubt in t/helper/test-parse-options.c to OPT_BOOL_NONEG instead of or in addition to adding the new flag --no-neg then you'd seen the effect in t0040.

René