From: René Scharfe Date: Wed, 03 Sep 2014 21:46:20 GMT Subject: Re: [PATCH] parse-options: detect attempt to add a duplicate short option name Message-ID: <54078C2C.5020503@web.de> In-Reply-To: Am 03.09.2014 um 23:05 schrieb Junio C Hamano: > René Scharfe 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 > Signed-off-by: Junio C Hamano > --- > 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 short option required argument > +| -B short option required argument > | --bar2 long option required argument > | -e, --fuz > | 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 >