From: Junio C Hamano <gitster@pobox•com>
To: "René Scharfe" <l.s.r@web•de>
Cc: Git List <git@vger•kernel.org>, John Cai <johncai86@gmail•com>
Subject: Re: [PATCH] reflog: fix expire --single-worktree
Date: Tue, 31 Oct 2023 08:11:05 +0900 [thread overview]
Message-ID: <xmqqlebjbt9y.fsf@gitster.g> (raw)
In-Reply-To: <82b832cb-da0b-47cf-9b5d-e8011a222151@web.de> ("René Scharfe"'s message of "Mon, 30 Oct 2023 17:12:41 +0100")
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:
>>
>>> ... 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");
next prev parent reply other threads:[~2023-10-30 23:11 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-28 11:58 [PATCH] reflog: fix expire --single-worktree René Scharfe
2023-10-29 22:31 ` Junio C Hamano
2023-10-30 16:12 ` René Scharfe
2023-10-30 23:11 ` Junio C Hamano [this message]
2023-10-31 6:16 ` René Scharfe
2023-10-30 17:21 ` Taylor Blau
2023-10-30 23:24 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlebjbt9y.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=johncai86@gmail$(echo .)com \
--cc=l.s.r@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox