Am 03.09.2014 um 23:05 schrieb Junio C Hamano:
René Scharfe <l.s.r@xxxxxx> 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@xxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html