* [PATCH] various: disallow --no-no-OPT for --no-opt options
@ 2017-04-18 17:09 Ævar Arnfjörð Bjarmason
2017-04-18 22:29 ` René Scharfe
0 siblings, 1 reply; 12+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-18 17:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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(-)
diff --git a/apply.c b/apply.c
index e6dbab26ad..47a91d0762 100644
--- a/apply.c
+++ b/apply.c
@@ -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")),
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229025..5fe9093947 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -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()
};
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 1d73d3ca3d..cb6467946e 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -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()
};
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 07631d0c9c..cf84e31ce8 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -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", ¬_new,
+ OPT_BOOL_NONEG('n', "no-create", ¬_new,
N_("don't checkout new files")),
OPT_BOOL('u', "index", &index_opt,
N_("update stat information in the index file")),
diff --git a/builtin/clone.c b/builtin/clone.c
index de85b85254..32c5843563 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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")),
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..46830ebfaf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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 */
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e0220630d0..dfbd245842 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -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")),
diff --git a/builtin/grep.c b/builtin/grep.c
index 65070c52fc..923cc570e6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -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")),
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index bbeaf20bcc..c563f2a9ad 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -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()
diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1ed..9824e137ec 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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")),
diff --git a/builtin/push.c b/builtin/push.c
index 5c22e9f2e5..216307c0fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -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,
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 23e212ee8c..e9359ecb1e 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -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")),
diff --git a/builtin/revert.c b/builtin/revert.c
index 345d9586a7..e708fdc228 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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:")),
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 19756595d5..6827846b54 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -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,
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0b2ecf41ae..b9622b2136 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -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")),
diff --git a/parse-options.h b/parse-options.h
index af711227ae..8211d2d5f1 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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, \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index a01430c24b..f6517a9305 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -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"),
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 74d2cd76fe..e571746775 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -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'
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-18 17:09 [PATCH] various: disallow --no-no-OPT for --no-opt options Ævar Arnfjörð Bjarmason
@ 2017-04-18 22:29 ` René Scharfe
2017-04-19 1:41 ` Jeff King
2017-04-19 7:00 ` Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 12+ messages in thread
From: René Scharfe @ 2017-04-18 22:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git; +Cc: Junio C Hamano
Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:
> 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/4F4D3545.6060704@lsrfire.ath.cx/
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> 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(-)
>
> diff --git a/apply.c b/apply.c
> index e6dbab26ad..47a91d0762 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -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")),
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229025..5fe9093947 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -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()
> };
> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> index 1d73d3ca3d..cb6467946e 100644
> --- a/builtin/check-ignore.c
> +++ b/builtin/check-ignore.c
> @@ -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()
> };
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 07631d0c9c..cf84e31ce8 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -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", ¬_new,
> + OPT_BOOL_NONEG('n', "no-create", ¬_new,
> N_("don't checkout new files")),
> OPT_BOOL('u', "index", &index_opt,
> N_("update stat information in the index file")),
> diff --git a/builtin/clone.c b/builtin/clone.c
> index de85b85254..32c5843563 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -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")),
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 4e288bc513..46830ebfaf 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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 */
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index e0220630d0..dfbd245842 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -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")),
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 65070c52fc..923cc570e6 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -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")),
> diff --git a/builtin/hash-object.c b/builtin/hash-object.c
> index bbeaf20bcc..c563f2a9ad 100644
> --- a/builtin/hash-object.c
> +++ b/builtin/hash-object.c
> @@ -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()
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1ed..9824e137ec 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -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")),
> diff --git a/builtin/push.c b/builtin/push.c
> index 5c22e9f2e5..216307c0fa 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -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,
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 23e212ee8c..e9359ecb1e 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -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")),
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 345d9586a7..e708fdc228 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -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:")),
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 19756595d5..6827846b54 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -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,
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 0b2ecf41ae..b9622b2136 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -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")),
> diff --git a/parse-options.h b/parse-options.h
> index af711227ae..8211d2d5f1 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -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) }
There's a whitespace error here ^^^^^^^^ according to git am.
> #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, \
> diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
> index a01430c24b..f6517a9305 100644
> --- a/t/helper/test-parse-options.c
> +++ b/t/helper/test-parse-options.c
> @@ -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"),
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 74d2cd76fe..e571746775 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -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'
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-18 22:29 ` René Scharfe
@ 2017-04-19 1:41 ` Jeff King
2017-04-19 2:40 ` Junio C Hamano
2017-04-19 7:00 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 12+ messages in thread
From: Jeff King @ 2017-04-19 1:41 UTC (permalink / raw)
To: René Scharfe
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano
On Wed, Apr 19, 2017 at 12:29:18AM +0200, René Scharfe wrote:
> Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:
> > 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.
I think we do strive to avoid "--no-no-foo", and instead have "--no-foo"
and "--foo" to cover both sides. So for example:
> > - 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 1:41 ` Jeff King
@ 2017-04-19 2:40 ` Junio C Hamano
2017-04-19 2:50 ` Jeff King
0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2017-04-19 2:40 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, Ævar Arnfjörð Bjarmason, git
Jeff King <peff@peff.net> writes:
> On Wed, Apr 19, 2017 at 12:29:18AM +0200, René Scharfe wrote:
> ...
>> 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:
>
>> > - 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.
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.
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 2:40 ` Junio C Hamano
@ 2017-04-19 2:50 ` Jeff King
2017-04-19 7:02 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2017-04-19 2:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Ævar Arnfjörð Bjarmason, git
On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:
> > 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).
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-18 22:29 ` René Scharfe
2017-04-19 1:41 ` Jeff King
@ 2017-04-19 7:00 ` Ævar Arnfjörð Bjarmason
2017-04-19 13:11 ` René Scharfe
1 sibling, 1 reply; 12+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-19 7:00 UTC (permalink / raw)
To: René Scharfe; +Cc: Git Mailing List, Junio C Hamano
On Wed, Apr 19, 2017 at 12:29 AM, René Scharfe <l.s.r@web.de> wrote:
> Am 18.04.2017 um 19:09 schrieb Ævar Arnfjörð Bjarmason:
>>
>> 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?
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.
> 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.
> Also: https://public-inbox.org/git/4F4D3545.6060704@lsrfire.ath.cx/
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 2:50 ` Jeff King
@ 2017-04-19 7:02 ` Ævar Arnfjörð Bjarmason
2017-04-19 9:08 ` Jacob Keller
2017-04-19 15:05 ` Jeff King
0 siblings, 2 replies; 12+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-19 7:02 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, René Scharfe, Git Mailing List
On Wed, Apr 19, 2017 at 4:50 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:
>
>> > 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 },
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 7:02 ` Ævar Arnfjörð Bjarmason
@ 2017-04-19 9:08 ` Jacob Keller
2017-04-19 15:05 ` Jeff King
1 sibling, 0 replies; 12+ messages in thread
From: Jacob Keller @ 2017-04-19 9:08 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, Junio C Hamano, René Scharfe, Git Mailing List
On Wed, Apr 19, 2017 at 12:02 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Wed, Apr 19, 2017 at 4:50 AM, Jeff King <peff@peff.net> wrote:
>> On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:
>>
>>> > 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 },
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 7:00 ` Ævar Arnfjörð Bjarmason
@ 2017-04-19 13:11 ` René Scharfe
2017-04-19 13:19 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 12+ messages in thread
From: René Scharfe @ 2017-04-19 13:11 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Jacob Keller
Am 19.04.2017 um 09:00 schrieb Ævar Arnfjörð Bjarmason:
> On Wed, Apr 19, 2017 at 12:29 AM, René Scharfe <l.s.r@web.de> wrote:
>> 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.
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é
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 13:11 ` René Scharfe
@ 2017-04-19 13:19 ` Ævar Arnfjörð Bjarmason
2017-04-19 13:44 ` René Scharfe
0 siblings, 1 reply; 12+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-19 13:19 UTC (permalink / raw)
To: René Scharfe; +Cc: Git Mailing List, Junio C Hamano, Jacob Keller
On Wed, Apr 19, 2017 at 3:11 PM, René Scharfe <l.s.r@web.de> wrote:
> Am 19.04.2017 um 09:00 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Wed, Apr 19, 2017 at 12:29 AM, René Scharfe <l.s.r@web.de> wrote:
>>>
>>> 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.
>
>
> 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")?
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 13:19 ` Ævar Arnfjörð Bjarmason
@ 2017-04-19 13:44 ` René Scharfe
0 siblings, 0 replies; 12+ messages in thread
From: René Scharfe @ 2017-04-19 13:44 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano, Jacob Keller
Am 19.04.2017 um 15:19 schrieb Ævar Arnfjörð Bjarmason:
> 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é
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] various: disallow --no-no-OPT for --no-opt options
2017-04-19 7:02 ` Ævar Arnfjörð Bjarmason
2017-04-19 9:08 ` Jacob Keller
@ 2017-04-19 15:05 ` Jeff King
1 sibling, 0 replies; 12+ messages in thread
From: Jeff King @ 2017-04-19 15:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, René Scharfe, Git Mailing List
On Wed, Apr 19, 2017 at 09:02:52AM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Apr 19, 2017 at 4:50 AM, Jeff King <peff@peff.net> wrote:
> > On Tue, Apr 18, 2017 at 07:40:37PM -0700, Junio C Hamano wrote:
> >
> >> > 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 },
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-04-19 15:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18 17:09 [PATCH] various: disallow --no-no-OPT for --no-opt options Ævar Arnfjörð Bjarmason
2017-04-18 22:29 ` René Scharfe
2017-04-19 1:41 ` Jeff King
2017-04-19 2:40 ` Junio C Hamano
2017-04-19 2:50 ` Jeff King
2017-04-19 7:02 ` Ævar Arnfjörð Bjarmason
2017-04-19 9:08 ` Jacob Keller
2017-04-19 15:05 ` Jeff King
2017-04-19 7:00 ` Ævar Arnfjörð Bjarmason
2017-04-19 13:11 ` René Scharfe
2017-04-19 13:19 ` Ævar Arnfjörð Bjarmason
2017-04-19 13:44 ` René Scharfe
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).