Re: [PATCH] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

[Stefan Beller <stefanbeller@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 9520 bytes --]

On 07/30/13 21:24, Junio C Hamano wrote:
> Stefan Beller <stefanbeller@googlemail.com> 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@pobox.com>
> ---
>  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@googlemail.com>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]