On 07/30/13 21:24, Junio C Hamano wrote:
> Stefan Beller <stefanbeller@xxxxxxxxxxxxxx> writes:
>
>> As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27),
>> the OPT_BOOLEAN was deprecated.
>> While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or
>> the OPT_COUNTUP to keep existing behavior, this commit is actually a
>> bug fix!
>>
>> In line 499 we have:
>> if (list + delete + verify > 1)
>> usage_with_options(git_tag_usage, options);
>> Now if we give one of the options twice, we'll get the usage information.
>> (i.e. 'git tag --verify --verify <tagname>' and
>> 'git --delete --delete <tagname>' yield usage information and do not
>> do the intended command.)
>>
>> This could have been fixed by rewriting the line to
>> if (!!list + !!delete + !!verify > 1)
>> usage_with_options(git_tag_usage, options);
>> or as it happened in this patch by having the parameters not
>> counting up for each occurrence, but the OPT_BOOL just setting the
>> variables to either 0 if the option is not given or 1 if the option is
>> given multiple times.
>
> Makes twisted sort of sense ;-).
>
>> However we could discuss if the negated options do make sense
>> here, or if we don't want to allow them here, as this seems valid
>> (before and after this patch):
>>
>> git tag --no-verify --delete <tagname>
>
> It probably does not. As you hinted in your earlier patch, we may
> want to introduce a "only can set to true" boolean used solely to
> specify these things. They are disguised as "options", but are in
> fact command operation modes that are often mutually exclusive.
>
> For these operation modes that are mutually exclusive, there are
> multiple possible implementations:
>
> * One OPT_BOOL_NONEG per option; the code ensures the mutual
> exclusion with "if (list + delete + verify > 1)";
>
> * One OPT_BIT per option in a single variable; the code ensures the
> mutual exclusion with count_bits, which may be a lot more
> cumbersome;
>
> * OPT_SET_INT that updates a single variable to enum; instead of
> making it an error to give two conflicting modes, this would give
> us the last-one-wins rule.
>
> Unlike usual "options", we generally do not want the last-one-wins
> semantics for command operation modes, I think.
>
> Perhaps we would want something like this?
>
> -- >8 --
> Subject: [PATCH] parse-options: add OPT_CMDMODE()
>
> This can be used to define a set of mutually exclusive "command
> mode" options, and automatically catch use of more than one from
> that set as an error.
>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> parse-options.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> parse-options.h | 3 +++
> 2 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index c2cbca2..62e9b1c 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -43,8 +43,42 @@ static void fix_filename(const char *prefix, const char **file)
> *file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));
> }
>
> +static int opt_command_mode_error(const struct option *opt,
> + const struct option *all_opts,
> + int flags)
> +{
> + const struct option *that;
> + struct strbuf message = STRBUF_INIT;
> + struct strbuf that_name = STRBUF_INIT;
> +
> + /*
> + * Find the other option that was used to set the variable
> + * already, and report that this is not compatible with it.
> + */
> + for (that = all_opts; that->type != OPTION_END; that++) {
> + if (that == opt ||
> + that->type != OPTION_CMDMODE ||
> + that->value != opt->value ||
> + that->defval != *(int *)opt->value)
> + continue;
> +
> + if (that->long_name)
> + strbuf_addf(&that_name, "--%s", that->long_name);
> + else
> + strbuf_addf(&that_name, "-%c", that->short_name);
> + strbuf_addf(&message, ": incompatible with %s", that_name.buf);
> + strbuf_release(&that_name);
> + opterror(opt, message.buf, flags);
> + strbuf_release(&message);
> + return -1;
> + }
> + return opterror(opt, ": incompatible with something else", flags);
> +}
> +
> static int get_value(struct parse_opt_ctx_t *p,
> - const struct option *opt, int flags)
> + const struct option *opt,
> + const struct option *all_opts,
> + int flags)
> {
> const char *s, *arg;
> const int unset = flags & OPT_UNSET;
> @@ -83,6 +117,16 @@ static int get_value(struct parse_opt_ctx_t *p,
> *(int *)opt->value = unset ? 0 : opt->defval;
> return 0;
>
> + case OPTION_CMDMODE:
> + /*
> + * Giving the same mode option twice, although is unnecessary,
> + * is not a grave error, so let it pass.
> + */
> + if (*(int *)opt->value && *(int *)opt->value != opt->defval)
> + return opt_command_mode_error(opt, all_opts, flags);
> + *(int *)opt->value = opt->defval;
> + return 0;
> +
> case OPTION_SET_PTR:
> *(void **)opt->value = unset ? NULL : (void *)opt->defval;
> return 0;
> @@ -143,12 +187,13 @@ static int get_value(struct parse_opt_ctx_t *p,
>
> static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)
> {
> + const struct option *all_opts = options;
> const struct option *numopt = NULL;
>
> for (; options->type != OPTION_END; options++) {
> if (options->short_name == *p->opt) {
> p->opt = p->opt[1] ? p->opt + 1 : NULL;
> - return get_value(p, options, OPT_SHORT);
> + return get_value(p, options, all_opts, OPT_SHORT);
> }
>
> /*
> @@ -177,6 +222,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
> static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
> const struct option *options)
> {
> + const struct option *all_opts = options;
> const char *arg_end = strchr(arg, '=');
> const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
> int abbrev_flags = 0, ambiguous_flags = 0;
> @@ -253,7 +299,7 @@ is_abbreviated:
> continue;
> p->opt = rest + 1;
> }
> - return get_value(p, options, flags ^ opt_flags);
> + return get_value(p, options, all_opts, flags ^ opt_flags);
> }
>
> if (ambiguous_option)
> @@ -265,18 +311,20 @@ is_abbreviated:
> (abbrev_flags & OPT_UNSET) ? "no-" : "",
> abbrev_option->long_name);
> if (abbrev_option)
> - return get_value(p, abbrev_option, abbrev_flags);
> + return get_value(p, abbrev_option, all_opts, abbrev_flags);
> return -2;
> }
>
> static int parse_nodash_opt(struct parse_opt_ctx_t *p, const char *arg,
> const struct option *options)
> {
> + const struct option *all_opts = options;
> +
> for (; options->type != OPTION_END; options++) {
> if (!(options->flags & PARSE_OPT_NODASH))
> continue;
> if (options->short_name == arg[0] && arg[1] == '\0')
> - return get_value(p, options, OPT_SHORT);
> + return get_value(p, options, all_opts, OPT_SHORT);
> }
> return -2;
> }
> diff --git a/parse-options.h b/parse-options.h
> index c378b75..2404e06 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -13,6 +13,7 @@ enum parse_opt_type {
> OPTION_COUNTUP,
> OPTION_SET_INT,
> OPTION_SET_PTR,
> + OPTION_CMDMODE,
> /* options with arguments (usually) */
> OPTION_STRING,
> OPTION_INTEGER,
> @@ -130,6 +131,8 @@ struct option {
> #define OPT_BOOL(s, l, v, h) OPT_SET_INT(s, l, v, h, 1)
> #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \
> (h), PARSE_OPT_NOARG, NULL, (p) }
> +#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
> + (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
> #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
> #define OPT_STRING(s, l, v, a, h) { OPTION_STRING, (s), (l), (v), (a), (h) }
> #define OPT_STRING_LIST(s, l, v, a, h) \
>
Your approach seems more like what we really want, however I'd have
some points:
* Is it a good idea to have so many different OPT_MODE or
OPTION_MODE defines? In my attempts I tried to reuse existing
OPTION_s to not pollute the parsing infrastructure with more
lines of code. ;)
* You can only have one OPTION_CMDMODE in one argv vector right?
I searched through the commands (... > 1) and did not find any
places, where we'd want to have multiple 'groups' of exclusive
commands, such as (either A or B) and/or (either C or D)
This cmd_mode would just all a (either A, B, C or D), but that
should be good for now.
* This command mode could also be used for builtin/branch:
if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
usage_with_options(builtin_branch_usage, options);
as well as commit:
if (!!also + !!only + !!all + !!interactive > 1)
die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
as well as for checkout:
if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
die(_("-b, -B and --orphan are mutually exclusive"));
So if we'd introduce this command mode, I'd be happy to supply patches
for branch, commit and checkout to use the new exclusive mechanism.
So I think I like it.
Reviewed-by: Stefan Beller <stefanbeller@xxxxxxxxxxxxxx>
Attachment:
signature.asc
Description: OpenPGP digital signature