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

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

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

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
-- 
2.1.0-394-g3e31896