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

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

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.

This retroactively forbids any short option name (which is defined
to be of type "int") outside the ASCII printable range.  We might
want to do one of two things:

 - tighten the type of short_name member to 'char', and further
   update optbug() to protect it against doing "'%c'" on a funny
   value, e.g. negative or above 127.

 - drop the check (even the "duplicate" check) for an option whose
   short_name is either negative or above 255, to allow clever folks
   to take advantage of the fact that such a short_name cannot be
   parsed from the command line and the member can be used to store
   some extra information.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Junio C Hamano <gitster@pobox.com> writes:

  > René Scharfe <l.s.r@web.de> writes:
  >
  >>> Not quite, as an opt with long name is reported with the long name
  >>> only, which is not very nice when the problem we are reporting is
  >>> about its short variant.
  >>
  >> Perhaps something like the patch below helps, here and in general?
  >
  > Excellent.  Not just this particular case, but we would show both
  > when both are available.
  >
  > Thanks; will reroll.

 parse-options.c               | 14 +++++++++++++-
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index b536896..34a15aa 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -14,8 +14,12 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 
 int optbug(const struct option *opt, const char *reason)
 {
-	if (opt->long_name)
+	if (opt->long_name) {
+		if (opt->short_name)
+			return error("BUG: switch '%c' (--%s) %s",
+				     opt->short_name, opt->long_name, reason);
 		return error("BUG: option '%s' %s", opt->long_name, reason);
+	}
 	return error("BUG: switch '%c' %s", opt->short_name, reason);
 }
 
@@ -345,12 +349,20 @@ 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) {
+			if (0x7F <= opts->short_name)
+				err |= optbug(opts, "invalid short name");
+			else if (short_opts[opts->short_name]++)
+				err |= optbug(opts, "short name already used");
+		}
 		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-396-ga35a9df