Re: [PATCH] reflog: fix expire --single-worktree

From: René Scharfe <l.s.r@web.de>
Date: Oct 31, 2023, 06:16 UTC
Message-ID: <59ea250d-364e-4a41-9511-4e2ff1a7cfb4@web.de>
In-Reply-To: <xmqqlebjbt9y.fsf@gitster.g>

Am 31.10.23 um 00:11 schrieb Junio C Hamano:

Show 29 quoted lines

> René Scharfe <l.s.r@web.de> writes:
>
>> Am 29.10.23 um 23:31 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> 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.

Show 11 quoted lines

>> 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.

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