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

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

[Stefan Beller]

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.

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>

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 builtin/tag.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index b3942e4..d155c9d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -442,12 +442,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	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_BOOL('l', "list", &list, N_("list tag names")),
 		{ 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_BOOL('d', "delete", &delete, N_("delete tags")),
+		OPT_BOOL('v', "verify", &verify, N_("verify tags")),
 
 		OPT_GROUP(N_("Tag creation options")),
 		OPT_BOOL('a', "annotate", &annotate,
@@ -455,7 +455,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK('m', "message", &msg, N_("message"),
 			     N_("tag message"), parse_msg_arg),
 		OPT_FILENAME('F', "file", &msgfile, N_("read message from file")),
-		OPT_BOOLEAN('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
+		OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
 		OPT_STRING(0, "cleanup", &cleanup_arg, N_("mode"),
 			N_("how to strip spaces and #comments from message")),
 		OPT_STRING('u', "local-user", &keyid, N_("key id"),
-- 
1.8.4.rc0.1.g8f6a3e5

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

[Junio C Hamano]

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) {

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

[Stefan Beller]

[-- 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 --]

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

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> 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?

That is not what I intended, at least.

	int one = 0, two = 0;
	struct option options[] = {
        OPT_CMDMODE('a', NULL, &one, N_("set one to a"), 'a'),
        OPT_CMDMODE('b', NULL, &one, N_("set one to b"), 'b'),
        OPT_CMDMODE('c', NULL, &two, N_("set two to c"), 'c'),
        OPT_CMDMODE('d', NULL, &two, N_("set two to d"), 'd'),
        OPT_END()
        }

should give you two independent sets of modes, one and two.

The only reason I needed to add an extra parameter to get_value()
was so that I can tell the former two and the latter two belong to
different groups, and that is done by looking at the address of the
variable.  In opt_command_mode_error(), opt->value == that->value
is used as a condition to see if the other option is possibly the
one that was used previously, which conflicted with us.

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

[Stefan Beller]

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

On 07/30/13 21:24, Junio C Hamano wrote:
> 
> ... 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) {


Here is just another idea: 
	if (cmdmode == 'v')
This may be hard to read, (What is 'v'? I cannot remember 
all the alphabet ;)) So maybe we could have an enum instead of
the last parameter? 
OPT_CMDMODE( short, long, variable, description, enum)

Also the variable would then only need to be an enum accepting variable,
and not an integer accepting all integer range, so we'd also catch 
typos or wrong values in such a case:
> +	if (argc == 0 && !cmdmode)
> +		cmdmode = 'l'; // maybe 'l' is a typo and not existing?


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

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

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> Here is just another idea: 
> 	if (cmdmode == 'v')
> This may be hard to read, (What is 'v'? I cannot remember 
> all the alphabet ;)) So maybe we could have an enum instead of
> the last parameter? 
> OPT_CMDMODE( short, long, variable, description, enum)

I actually was thinking about going totally in the opposite
direction.

People who grew up with the old Unix tradition getopt(3) are used to
code like this:

        while ((opt = getopt(ac, av, "abcde")) != -1) {
                switch (opt) {
                case 'a': perform_a(); break;
                case 'b': perform_b(); break;
                ...
                }
        }

In other words, the "enum" is most convenient if it matches the
"short" option, so instead of having to repeat ourselves over and
over, like I did in that illustration patch for builtin/tag.c, e.g.

		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'),
		OPT_CMDMODE('v', "verify", &cmdmode, N_("verify tags"), 'v'),

we could just do

#define OPT_CMDMODE(s, l, v, h) \
    { OPTION_CMDMODE, (s), (l), (v), NULL, \
      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }

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

[Stefan Beller]

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

On 07/31/13 00:28, Junio C Hamano wrote:
> 
> we could just do
> 
> #define OPT_CMDMODE(s, l, v, h) \
>     { OPTION_CMDMODE, (s), (l), (v), NULL, \
>       (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }
> 

I agree that's a better proposal than mine.


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

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

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> On 07/31/13 00:28, Junio C Hamano wrote:
>> 
>> we could just do
>> 
>> #define OPT_CMDMODE(s, l, v, h) \
>>     { OPTION_CMDMODE, (s), (l), (v), NULL, \
>>       (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }
>> 
>
> I agree that's a better proposal than mine.

By the way, I haven't convinced myself that it is a good idea in
general to encourage more use of command mode options, so I am a bit
reluctant to add this before knowing which direction in the longer
term we are going.

 - Some large-ish Git subcommands, like "git submodule", use the
   mode word (e.g. "git submodule status") to specify the operation
   mode (youe could consider "status" a subsubcommand that
   "submodule" subcommand takes).  These commands typically began
   their life from day one with the mode words.

 - On the other hand, many Git subcommands, like "git tag", have
   "the primary operation mode" (e.g. "create a new one" is the
   primary operation mode for "git tag"), and use command mode
   options to specify other operation modes (e.g. "--delete").
   These commands started as single purpose commands (i.e. to
   perform their "primary operation") but have organically grown
   over time and acquired command mode options to invoke their
   secondary operations.

As an end user, you need to learn which style each command takes,
which is an unnecessary burden at the UI level.  In the longer term,
we may want to consider picking a single style, and migrating
everybody to it.  If I have to vote today, I would say we should
teach "git submodule" to also take command mode options (e.g. "git
submodule --status" will be understood the same way as "git
submodule status"), make them issue warnings when mode words are
used and encourage users to use command mode options instead, and
optionally remove the support of mode words at a large version bump
like 3.0.

One clear advantage mode words have over command mode options is
that there is no room for end user confusion.  The first word after
"git subcmd" is the mode word, and you will not even dream of asking
"what would 'git submodule add del foo' do?" as it is nonsensical.
The command mode options, on the other hand, gives too much useless
flexibility to ask for nonsense, e.g. "git tag --delete --verify",
"git tag --no-delete --delete", etc., and extra code needs to detect
and reject combinations.  But commands that took mode options cannot
be easily migrated to take mode words without hurting existing users
and scripts (e.g. "git tag delete master" can never be a request to
delete the tag 'master', as it is a request to create a tag whose
name is 'delete' that points at the same object as 'master' points
at).

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

[Stefano Lattarini]

(Going through old mail today, sorry for the late reply)

On 08/01/2013 12:10 AM, Junio C Hamano wrote:> Stefan Beller 
<stefanbeller@googlemail.com> writes:
 >
 >> On 07/31/13 00:28, Junio C Hamano wrote:
 >>>
 >>> we could just do
 >>>
 >>> #define OPT_CMDMODE(s, l, v, h) \
 >>>      { OPTION_CMDMODE, (s), (l), (v), NULL, \
 >>>        (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }
 >>>
 >>
 >> I agree that's a better proposal than mine.
 >
 > By the way, I haven't convinced myself that it is a good idea in
 > general to encourage more use of command mode options, so I am a bit
 > reluctant to add this before knowing which direction in the longer
 > term we are going.
 >
 >   - Some large-ish Git subcommands, like "git submodule", use the
 >     mode word (e.g. "git submodule status") to specify the operation
 >     mode (youe could consider "status" a subsubcommand that
 >     "submodule" subcommand takes).  These commands typically began
 >     their life from day one with the mode words.
 >
 >   - On the other hand, many Git subcommands, like "git tag", have
 >     "the primary operation mode" (e.g. "create a new one" is the
 >     primary operation mode for "git tag"), and use command mode
 >     options to specify other operation modes (e.g. "--delete").
 >     These commands started as single purpose commands (i.e. to
 >     perform their "primary operation") but have organically grown
 >     over time and acquired command mode options to invoke their
 >     secondary operations.
 >
 > As an end user, you need to learn which style each command takes,
 > which is an unnecessary burden at the UI level.  In the longer term,
 > we may want to consider picking a single style, and migrating
 > everybody to it.  If I have to vote today, I would say we should
 > teach "git submodule" to also take command mode options (e.g. "git
 > submodule --status" will be understood the same way as "git
 > submodule status"), make them issue warnings when mode words are
 > used and encourage users to use command mode options instead, and
 > optionally remove the support of mode words at a large version bump
 > like 3.0.
 >
 > One clear advantage mode words have over command mode options is
 > that there is no room for end user confusion.  The first word after
 > "git subcmd" is the mode word, and you will not even dream of asking
 > "what would 'git submodule add del foo' do?" as it is nonsensical.
 > The command mode options, on the other hand, gives too much useless
 > flexibility to ask for nonsense, e.g. "git tag --delete --verify",
 > "git tag --no-delete --delete", etc., and extra code needs to detect
 > and reject combinations.  But commands that took mode options cannot
 > be easily migrated to take mode words without hurting existing users
 > and scripts (e.g. "git tag delete master" can never be a request to
 > delete the tag 'master', as it is a request to create a tag whose
 > name is 'delete' that points at the same object as 'master' points
 > at).
 >
Why not encourage the use of a standardized '--action' option instead?
This can work with lesser compatibility headaches for both the commands
taking mode options and the commands taking mode words:

   "git submodule init"   becomes  "git submodule --action=init"
   "git tag --delete TAG" becomes  "git tag --action=delete TAGNAME"

Commands that have a "primary operation mode" can keep the use
of --action optional, while commands that have no such sensible
primary mode can make it mandatory (again, this gives more compatibility 
with the existing syntax).  And the old syntax
can be supported in parallel to the new one for a potentially
indefinite time; albeit, if I were to propose a timetable, I'd
got for:

   - Git 2.0: the new syntax is introduced
   - Git 2.x: any use the old syntax starts to trigger warnings
     (which can be silenced with a config option)
   - Git 3.0: any use the old syntax starts to trigger fatal
     errors (which can be turned into mandatory warnings with
     a config option)
   - Git 4.0: any use the old syntax starts to trigger
     mandatory fatal errors.
   - Git 4.x: Remove any handling of the old syntax.

Just my 2 cents,
   Stefano

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

[Jonathan Nieder]

Stefano Lattarini wrote:

> Why not encourage the use of a standardized '--action' option instead?

Because it's an unpleasant UI. :)

> This can work with lesser compatibility headaches for both the commands
> taking mode options and the commands taking mode words:
>
>   "git submodule init"   becomes  "git submodule --action=init"
>   "git tag --delete TAG" becomes  "git tag --action=delete TAGNAME"

That looks like a bad change in both cases --- it involves more
typing without much upside to go along with it.  But

	"git submodule init"   gains synonym "git submodule --init"
	"git tag --delete TAG" stays as      "git tag --delete TAG"

looks fine to me.

In the long run, we could require that for new commands the 'action'
option must come immediately after the git command name if that makes
things easier to learn.

Thanks for some food for thought.

My two cents,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Stefano Lattarini wrote:
>
>> Why not encourage the use of a standardized '--action' option instead?
>
> Because it's an unpleasant UI. :)
>
>> This can work with lesser compatibility headaches for both the commands
>> taking mode options and the commands taking mode words:
>>
>>   "git submodule init"   becomes  "git submodule --action=init"
>>   "git tag --delete TAG" becomes  "git tag --action=delete TAGNAME"
>
> That looks like a bad change in both cases --- it involves more
> typing without much upside to go along with it.  But
>
> 	"git submodule init"   gains synonym "git submodule --init"
> 	"git tag --delete TAG" stays as      "git tag --delete TAG"
>
> looks fine to me.

I agree 100% with the above that illustrates why --action=<name> is
a bad idea.  As I already said, adding action-option like --init, if
doing so might help people, I am not opposed to it.

Thanks.