* [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-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: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
* 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 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
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).