From 9ca1169fd92c71ebbef92ff18aa5d91a2157d1bd Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Sun, 5 Dec 2010 23:57:42 -0800 Subject: parse-options: Don't call parse_options_check() so much parse_options_check() is being called for each invocation of parse_options_step which can be quite a bit for some commands. The commit introducing this function cb9d398 (parse-options: add parse_options_check to validate option specs., 2009-06-09) had the correct motivation and explicitly states that parse_options_check() should be called from parse_options_start(). However, the implementation differs from the motivation. Fix it. Signed-off-by: Stephen Boyd Signed-off-by: Junio C Hamano --- parse-options.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 0fa79bc31d..9bbbc6a5fe 100644 --- a/parse-options.c +++ b/parse-options.c @@ -338,7 +338,7 @@ static void parse_options_check(const struct option *opts) void parse_options_start(struct parse_opt_ctx_t *ctx, int argc, const char **argv, const char *prefix, - int flags) + const struct option *options, int flags) { memset(ctx, 0, sizeof(*ctx)); ctx->argc = argc - 1; @@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, if ((flags & PARSE_OPT_KEEP_UNKNOWN) && (flags & PARSE_OPT_STOP_AT_NON_OPTION)) die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together"); + parse_options_check(options); } static int usage_with_options_internal(struct parse_opt_ctx_t *, @@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx, { int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP); - parse_options_check(options); - /* we must reset ->opt, unknown short option leave it dangling */ ctx->opt = NULL; @@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const char *prefix, { struct parse_opt_ctx_t ctx; - parse_options_start(&ctx, argc, argv, prefix, flags); + parse_options_start(&ctx, argc, argv, prefix, options, flags); switch (parse_options_step(&ctx, options, usagestr)) { case PARSE_OPT_HELP: exit(129); -- cgit v1.3 From 1e5ce570ca368b97c8e3b238bb0228c5ca41b494 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Thu, 2 Dec 2010 00:01:18 -0600 Subject: parse-options: clearer reporting of API misuse The PARSE_OPT_LASTARG_DEFAULT flag is meant for options like --contains that (1) traditionally had a mandatory argument and (2) have some better behavior to use when appearing in the final position. It makes no sense to combine this with OPTARG, so ever since v1.6.4-rc0~71 (parse-options: add parse_options_check to validate option specs, 2009-07-09) this mistake is flagged with error: `--option` uses incompatible flags LASTARG_DEFAULT and OPTARG and an exit status representing an error in commandline usage. Unfortunately that which might confuse scripters calling such an erroneous program into thinking the _script_ contains an error. Clarify that it is an internal error by dying with a message beginning "error: BUG: ..." and status 128. While at it, clean up parse_options_check to prepare for more checks. Long term, it would be nicer to make such checks happen at compile time. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- parse-options.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 9bbbc6a5fe..67d1adca0a 100644 --- a/parse-options.c +++ b/parse-options.c @@ -11,6 +11,13 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx, #define OPT_SHORT 1 #define OPT_UNSET 2 +static int optbug(const struct option *opt, const char *reason) +{ + if (opt->long_name) + return error("BUG: option '%s' %s", opt->long_name, reason); + return error("BUG: switch '%c' %s", opt->short_name, reason); +} + static int opterror(const struct option *opt, const char *reason, int flags) { if (flags & OPT_SHORT) @@ -320,20 +327,12 @@ static void parse_options_check(const struct option *opts) for (; opts->type != OPTION_END; opts++) { if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && - (opts->flags & PARSE_OPT_OPTARG)) { - if (opts->long_name) { - error("`--%s` uses incompatible flags " - "LASTARG_DEFAULT and OPTARG", opts->long_name); - } else { - error("`-%c` uses incompatible flags " - "LASTARG_DEFAULT and OPTARG", opts->short_name); - } - err |= 1; - } + (opts->flags & PARSE_OPT_OPTARG)) + err |= optbug(opts, "uses incompatible flags " + "LASTARG_DEFAULT and OPTARG"); } - if (err) - exit(129); + exit(128); } void parse_options_start(struct parse_opt_ctx_t *ctx, -- cgit v1.3 From a02dd4ff7dd79eb6aa39b00c90c293e3c3d10b4c Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Thu, 2 Dec 2010 00:05:05 -0600 Subject: parse-options: move NODASH sanity checks to parse_options_check A dashless switch (like '(' passed to 'git grep') cannot be negated, cannot be attached to an argument, and cannot have a long form. Currently parse-options runs the related sanity checks when the dashless option is used; better to always check them at the start of option parsing, so mistakes can be caught more quickly. The error message at the new call site is less specific about the nature of the error, for simplicity. On the other hand, it prints which switch was problematic. Before: fatal: BUG: dashless options can't be long After: error: BUG: switch '(' uses feature not supported for dashless options Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- parse-options.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 67d1adca0a..9ff9acaabf 100644 --- a/parse-options.c +++ b/parse-options.c @@ -288,13 +288,6 @@ static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg, for (; options->type != OPTION_END; options++) { if (!(options->flags & PARSE_OPT_NODASH)) continue; - if ((options->flags & PARSE_OPT_OPTARG) || - !(options->flags & PARSE_OPT_NOARG)) - die("BUG: dashless options don't support arguments"); - if (!(options->flags & PARSE_OPT_NONEG)) - die("BUG: dashless options don't support negation"); - if (options->long_name) - die("BUG: dashless options can't be long"); if (options->short_name == arg[0] && arg[1] == '\0') return get_value(p, options, OPT_SHORT); } @@ -330,6 +323,13 @@ static void parse_options_check(const struct option *opts) (opts->flags & PARSE_OPT_OPTARG)) err |= optbug(opts, "uses incompatible flags " "LASTARG_DEFAULT and OPTARG"); + if (opts->flags & PARSE_OPT_NODASH && + ((opts->flags & PARSE_OPT_OPTARG) || + !(opts->flags & PARSE_OPT_NOARG) || + !(opts->flags & PARSE_OPT_NONEG) || + opts->long_name)) + err |= optbug(opts, "uses feature " + "not supported for dashless options"); } if (err) exit(128); -- cgit v1.3 From 5c400ed2e05070d79b6cd9438ff5607ec0a83589 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Thu, 2 Dec 2010 00:08:57 -0600 Subject: parse-options: sanity check PARSE_OPT_NOARG flag Some option types cannot use an argument --- boolean options that would set a bit or flag or increment a counter, for example. If configured in the flag word to accept an argument anyway, the result is an argument that is advertised in "program -h" output only to be rejected by parse-options::get_value. Luckily all current users of these option types use PARSE_OPT_NOARG and do not use PARSE_OPT_OPTARG. Add a check to ensure that that remains true. The check is run once for each invocation of parse_option_start(). Improved-by: Stephen Boyd Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- parse-options.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 9ff9acaabf..79c56f32f9 100644 --- a/parse-options.c +++ b/parse-options.c @@ -330,6 +330,19 @@ static void parse_options_check(const struct option *opts) opts->long_name)) err |= optbug(opts, "uses feature " "not supported for dashless options"); + switch (opts->type) { + case OPTION_BOOLEAN: + case OPTION_BIT: + case OPTION_NEGBIT: + case OPTION_SET_INT: + case OPTION_SET_PTR: + case OPTION_NUMBER: + if ((opts->flags & PARSE_OPT_OPTARG) || + !(opts->flags & PARSE_OPT_NOARG)) + err |= optbug(opts, "should not accept an argument"); + default: + ; /* ok. (usually accepts an argument) */ + } } if (err) exit(128); -- cgit v1.3 From c1f4ec9ef45232d6dbdea4c417a9d41eb8ad7f4f Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 1 Dec 2010 17:30:40 -0600 Subject: parse-options: do not infer PARSE_OPT_NOARG from option type Simplify the "takes no value" error path by relying on PARSE_OPT_NOARG being set correctly. That is: - if the PARSE_OPT_NOARG flag is set, reject --opt=value regardless of the option type; - if the PARSE_OPT_NOARG flag is unset, accept --opt=value regardless of the option type. This way, the accepted usage more closely matches the usage advertised with --help-all. No functional change intended, since the NOARG flag is only used with "boolean-only" option types in existing parse_options callers. Signed-off-by: Stephen Boyd Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- parse-options.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 79c56f32f9..578035671a 100644 --- a/parse-options.c +++ b/parse-options.c @@ -62,23 +62,8 @@ static int get_value(struct parse_opt_ctx_t *p, return opterror(opt, "takes no value", flags); if (unset && (opt->flags & PARSE_OPT_NONEG)) return opterror(opt, "isn't available", flags); - - if (!(flags & OPT_SHORT) && p->opt) { - switch (opt->type) { - case OPTION_CALLBACK: - if (!(opt->flags & PARSE_OPT_NOARG)) - break; - /* FALLTHROUGH */ - case OPTION_BOOLEAN: - case OPTION_BIT: - case OPTION_NEGBIT: - case OPTION_SET_INT: - case OPTION_SET_PTR: - return opterror(opt, "takes no value", flags); - default: - break; - } - } + if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG)) + return opterror(opt, "takes no value", flags); switch (opt->type) { case OPTION_BIT: -- cgit v1.3 From b57c68a69e028cc41eb01404dc4446a463c0e464 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 1 Dec 2010 17:31:36 -0600 Subject: parse-options: never suppress arghelp if LITERAL_ARGHELP is set The PARSE_OPT_LITERAL_ARGHELP flag allows a program to override the standard " for mandatory, [argument] for optional" markup in its help message. Extend it to override the usual "no text for disallowed", too (for the PARSE_OPT_NOARG | PARSE_OPT_LITERAL_ARGHELP case, which was previously meaningless), to be more intuitive. The motivation is to allow update-index to correctly advertise --cacheinfo add the specified entry to the index while abusing PARSE_OPT_NOARG to disallow the "sticked form" --cacheinfo= Noticed-by: Stephen Boyd Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- parse-options.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 578035671a..632c1346c6 100644 --- a/parse-options.c +++ b/parse-options.c @@ -537,7 +537,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, if (opts->type == OPTION_NUMBER) pos += fprintf(outfile, "-NUM"); - if (!(opts->flags & PARSE_OPT_NOARG)) + if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) || + !(opts->flags & PARSE_OPT_NOARG)) pos += usage_argh(opts, outfile); if (pos <= USAGE_OPTS_WIDTH) -- cgit v1.3 From b0b3a8b666ac9bcab93c9b05ca7de918d7fa18bc Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 1 Dec 2010 17:32:16 -0600 Subject: parse-options: allow git commands to invent new option types parse-options provides a variety of option behaviors, including OPTION_CALLBACK, which should take care of just about any sane behavior. All supported behaviors obey the following constraint: A --foo option can only accept (and base its behavior on) one argument, which would be the following command-line argument in the "unsticked" form. Alas, some existing git commands have options that do not obey that constraint. For example, update-index --cacheinfo takes three arguments, and update-index --resolve takes all later parameters as arguments. Introduces an OPTION_LOWLEVEL_CALLBACK backdoor to parse-options so such option types can be supported without tempting inventors of other commands through mention in the public API. Commands can set the callback field to a function accepting three arguments: the option parsing context, the option itself, and a flag indicating whether the the option was negated. When the option is encountered, that function is called to take over from get_value(). The return value should be zero for success, -1 for usage errors. Thanks to Stephen Boyd for API guidance. Improved-by: Stephen Boyd Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- parse-options.c | 3 +++ parse-options.h | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index 632c1346c6..cd92686916 100644 --- a/parse-options.c +++ b/parse-options.c @@ -66,6 +66,9 @@ static int get_value(struct parse_opt_ctx_t *p, return opterror(opt, "takes no value", flags); switch (opt->type) { + case OPTION_LOWLEVEL_CALLBACK: + return (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset); + case OPTION_BIT: if (unset) *(int *)opt->value &= ~opt->defval; diff --git a/parse-options.h b/parse-options.h index 5eb499b992..470bb33298 100644 --- a/parse-options.h +++ b/parse-options.h @@ -17,6 +17,7 @@ enum parse_opt_type { OPTION_STRING, OPTION_INTEGER, OPTION_CALLBACK, + OPTION_LOWLEVEL_CALLBACK, OPTION_FILENAME }; @@ -43,6 +44,10 @@ enum parse_opt_option_flags { struct option; typedef int parse_opt_cb(const struct option *, const char *arg, int unset); +struct parse_opt_ctx_t; +typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, + const struct option *opt, int unset); + /* * `type`:: * holds the type of the option, you must have an OPTION_END last in your @@ -87,7 +92,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset); * useful for users of OPTION_NEGBIT. * * `callback`:: - * pointer to the callback to use for OPTION_CALLBACK. + * pointer to the callback to use for OPTION_CALLBACK or + * OPTION_LOWLEVEL_CALLBACK. * * `defval`:: * default value to fill (*->value) with for PARSE_OPT_OPTARG. -- cgit v1.3 From 979240fee32628c317998f3c3fe2619cf01decc2 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 1 Dec 2010 17:32:55 -0600 Subject: parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION Introduce a PARSE_OPT_NON_OPTION state, so parse_option_step() callers can easily distinguish between non-options and other reasons for option parsing termination (like "--"). Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- parse-options.c | 3 ++- parse-options.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) (limited to 'parse-options.c') diff --git a/parse-options.c b/parse-options.c index cd92686916..42b51ef145 100644 --- a/parse-options.c +++ b/parse-options.c @@ -373,7 +373,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx, if (parse_nodash_opt(ctx, arg, options) == 0) continue; if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION) - break; + return PARSE_OPT_NON_OPTION; ctx->out[ctx->cpidx++] = ctx->argv[0]; continue; } @@ -455,6 +455,7 @@ int parse_options(int argc, const char **argv, const char *prefix, switch (parse_options_step(&ctx, options, usagestr)) { case PARSE_OPT_HELP: exit(129); + case PARSE_OPT_NON_OPTION: case PARSE_OPT_DONE: break; default: /* PARSE_OPT_UNKNOWN */ diff --git a/parse-options.h b/parse-options.h index 470bb33298..3c2ec1d092 100644 --- a/parse-options.h +++ b/parse-options.h @@ -167,6 +167,7 @@ extern NORETURN void usage_msg_opt(const char *msg, enum { PARSE_OPT_HELP = -1, PARSE_OPT_DONE, + PARSE_OPT_NON_OPTION, PARSE_OPT_UNKNOWN }; -- cgit v1.3