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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2023, 23:11 UTC
Message-ID: <xmqqlebjbt9y.fsf@gitster.g>
In-Reply-To: <82b832cb-da0b-47cf-9b5d-e8011a222151@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 30 quoted lines

> Am 29.10.23 um 23:31 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> ... and added a non-printable short flag for it, presumably by
>>> accident.
>>
>> Very well spotted.
>>
>> FWIW, with the following patch on top of this patch, all tests pass
>> (and without your fix, of course this notices the "\001" and breaks
>> numerous tests that use "git reflog").  So you seem to have found
>> the only one broken instance (among those that are tested, anyway).
>>
>>  parse-options.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> 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'?").

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

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