Re: [PATCH] parse-options: detect attempt to add a duplicate short option name

["René Scharfe" <l.s.r@web.de>]

Am 03.09.2014 um 23:05 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Compact and useful, I like it.
>>
>> You might want to squash in something like this, though.  Without it
>> t1502 fails because -b is defined twice there.
>
> Thanks.  I like it to see that the check automatically propagates
> even to scripts ;-)
>
> It bugged me enough that we didn't identify which short option
> letter we were complaining about

The old code did report the short option.  E.g. for t1502 it said:

	error: BUG: switch 'b' short name already used

You can leave that to optbug(), no need for the strbuf.

> and that opts->short_name is
> defined as an "int", which may cause us to overstep char[128],
> I ended up doing it this way instead, though.   It no longer is so
> compact, even though it may still have the same usefulness.

A range check is an additional feature (increased usefulness).  I guess 
using invalid characters is not that common a mistake, though.

Space is allowed as a short option by the code; intentionally?

>
> We might want to tighten the type of the short_name member to
> unsigned char, but I didn't go that far yet, at least in this step.
>
> -- >8 --
> Subject: [PATCH] parse-options: detect attempt to add a duplicate short option name
>
> It is easy to overlook an already assigned single-letter option name
> and try to use it for a new one.  Help the developer to catch it
> before such a mistake escapes the lab.
>
> Helped-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   parse-options.c               | 15 +++++++++++++++
>   t/t1502-rev-parse-parseopt.sh |  4 ++--
>   2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index b536896..70227e9 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -345,12 +345,27 @@ static void check_typos(const char *arg, const struct option *options)
>   static void parse_options_check(const struct option *opts)
>   {
>   	int err = 0;
> +	char short_opts[128];
> +
> +	memset(short_opts, '\0', sizeof(short_opts));
>
>   	for (; opts->type != OPTION_END; opts++) {
>   		if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
>   		    (opts->flags & PARSE_OPT_OPTARG))
>   			err |= optbug(opts, "uses incompatible flags "
>   					"LASTARG_DEFAULT and OPTARG");
> +		if (opts->short_name) {
> +			struct strbuf errmsg = STRBUF_INIT;
> +			if (opts->short_name < ' ' || 0x7F <= opts->short_name)
> +				strbuf_addf(&errmsg, "invalid short name (0x%02x)",
> +					    opts->short_name);
> +			else if (short_opts[opts->short_name]++)
> +				strbuf_addf(&errmsg, "short name %c already used",
> +					    opts->short_name);
> +			if (errmsg.len)
> +				err |= optbug(opts, errmsg.buf);
> +			strbuf_release(&errmsg);
> +		}
>   		if (opts->flags & PARSE_OPT_NODASH &&
>   		    ((opts->flags & PARSE_OPT_OPTARG) ||
>   		     !(opts->flags & PARSE_OPT_NOARG) ||
> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index 922423e..ebe7c3b 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -19,7 +19,7 @@ sed -e 's/^|//' >expect <<\END_EXPECT
>   |    -d, --data[=...]      short and long option with an optional argument
>   |
>   |Argument hints
> -|    -b <arg>              short option required argument
> +|    -B <arg>              short option required argument
>   |    --bar2 <arg>          long option required argument
>   |    -e, --fuz <with-space>
>   |                          short and long option required argument
> @@ -51,7 +51,7 @@ sed -e 's/^|//' >optionspec <<\EOF
>   |d,data?   short and long option with an optional argument
>   |
>   | Argument hints
> -|b=arg     short option required argument
> +|B=arg     short option required argument
>   |bar2=arg  long option required argument
>   |e,fuz=with-space  short and long option required argument
>   |s?some    short option optional argument
>