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

[Jonathan Nieder <jrnieder@gmail.com>]

On Wed, Sep 03, 2014 at 02:46:25PM -0700, Jonathan Nieder wrote:
> Junio C Hamano wrote:
> 
> > --- 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]++)
> 
> What happens on platforms with a signed char?

Ah, I see now that the "< ' '" check would catch that.

With René's suggestions squashed in, that becomes

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

diff --git a/parse-options.c b/parse-options.c
index e7dafa8..6ad7d90 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -347,12 +347,21 @@ 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 (opts->short_name <= ' ' || 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.rc2.206.gedb03e5