Am 31.10.23 um 00:11 schrieb Junio C Hamano:

René Scharfe [off-list ref] writes:

quoted

Am 29.10.23 um 23:31 schrieb Junio C Hamano:

quoted

René Scharfe [off-list ref] writes:

diff --git i/parse-options.c w/parse-options.c
index 093eaf2db8..be8bedba29 100644
--- i/parse-options.c
+++ w/parse-options.c

@@ -469,7 +469,8 @@ static void parse_options_check(const struct option *opts)
 			optbug(opts, "uses incompatible flags "
 			       "LASTARG_DEFAULT and OPTARG");
 		if (opts->short_name) {
-			if (0x7F <= opts->short_name)
+			if (opts->short_name &&
+			    (opts->short_name < 0x21 || 0x7F <= opts->short_name))

Good idea.  This is equivalent to !isprint(opts->short_name), which I
find to be more readable here.

Thanks---I didn't think of using !isprint() but you are right.  It
is much shorter.

I am not absolutely certain if it is easier to read, though.  I get
always confused when asking myself if SP, HT, and LF are printables.
(in other words, I cannot immediately answer "does 'printable' mean
'can be sent to a teletype and have it do what is expected to be
done?"---the question I should be asking myself is "is 'printable'
synonym to 'when printed, some ink is consumed'?").

isprint() accepts SP, but not HT or LF.  Go figure.  And thus I made an
off-by-one error by suggesting this macro, because your version rejects
SP (0x20).  Am I unintentionally making a point here for using the
is-macros because I can't read numeric comparisons? O_o

isalnum() and ispunct() could be used instead.

quoted

Seeing why "char short_opts[128];" a
few lines up is big enough would become a bit harder, though.

Sorry, but I do not quite follow.  We used to allow anything below
0x7e; now we clip that range further to reject anything below 0x21.
If [128] was big enough, it still is big enough, no?

Because the type of .short_name member is "int", we could have had
negative number in there and access to short_opts[] on the next line
would have been out of bounds.  By clipping the lower bound, we get
rid of that risk, no?

Yes, but if the allowed range is hidden behind macro invocations then
the boundaries are no longer as obvious as in your version.

quoted

quoted

 				optbug(opts, "invalid short name");
 			else if (short_opts[opts->short_name]++)
 				optbug(opts, "short name already used");