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

From: Stefan Beller <stefanbeller@googlemail.com>
Date: Jul 30, 2013, 21:28 UTC
Message-ID: <51F83010.2060804@googlemail.com>
In-Reply-To: <7va9l3x34f.fsf@alter.siamese.dyndns.org>

On 07/30/13 21:24, Junio C Hamano wrote:

Show 74 quoted lines

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