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

[Junio C Hamano <gitster@pobox.com>]

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) \
-- 
1.8.4-rc0-153-g9820077


... and then "git tag" may become like so.

 builtin/tag.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..d8ae5aa 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -436,18 +436,18 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_lock *lock;
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
-	int annotate = 0, force = 0, lines = -1, list = 0,
-		delete = 0, verify = 0;
+	int annotate = 0, force = 0, lines = -1;
+	int cmdmode = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
 	struct option options[] = {
-		OPT_BOOLEAN('l', "list", &list, N_("list tag names")),
+		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
 				N_("print <n> lines of each tag message"),
 				PARSE_OPT_OPTARG, NULL, 1 },
-		OPT_BOOLEAN('d', "delete", &delete, N_("delete tags")),
-		OPT_BOOLEAN('v', "verify", &verify, N_("verify tags")),
+		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'),
+		OPT_CMDMODE('v', "verify", &cmdmode, N_("verify tags"), 'v'),
 
 		OPT_GROUP(N_("Tag creation options")),
 		OPT_BOOLEAN('a', "annotate", &annotate,
@@ -489,22 +489,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	if (opt.sign)
 		annotate = 1;
-	if (argc == 0 && !(delete || verify))
-		list = 1;
+	if (argc == 0 && !cmdmode)
+		cmdmode = 'l';
 
-	if ((annotate || msg.given || msgfile || force) &&
-	    (list || delete || verify))
+	if ((annotate || msg.given || msgfile || force) && (cmdmode != 0))
 		usage_with_options(git_tag_usage, options);
 
-	if (list + delete + verify > 1)
-		usage_with_options(git_tag_usage, options);
 	finalize_colopts(&colopts, -1);
-	if (list && lines != -1) {
+	if (cmdmode == 'l' && lines != -1) {
 		if (explicitly_enable_column(colopts))
 			die(_("--column and -n are incompatible"));
 		colopts = 0;
 	}
-	if (list) {
+	if (cmdmode == 'l') {
 		int ret;
 		if (column_active(colopts)) {
 			struct column_options copts;
@@ -523,9 +520,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--contains option is only allowed with -l."));
 	if (points_at.nr)
 		die(_("--points-at option is only allowed with -l."));
-	if (delete)
+	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag);
-	if (verify)
+	if (cmdmode == 'v')
 		return for_each_tag_name(argv, verify_tag);
 
 	if (msg.given || msgfile) {