Re: [PATCH] parse-options: detect attempt to add a duplicate short option name
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 3, 2014, 21:05 UTC
- Message-ID
- <xmqqoauwwh2c.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <54077A3E.20703@web.de>
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