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

From

Junio C Hamano <gitster@pobox.com>

Date

Sep 4, 2014, 18:07 UTC

Message-ID

<xmqqtx4ntg33.fsf@gitster.dls.corp.google.com>

In-Reply-To

<xmqqegvruwmz.fsf@gitster.dls.corp.google.com>

 - 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