From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Thomas Bock <bockthom@cs.uni-saarland.de>,
Derrick Stolee <derrickstolee@github.com>,
git@vger.kernel.org
Subject: Re: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp
Date: Tue, 25 Apr 2023 01:27:39 -0400 [thread overview]
Message-ID: <20230425052739.GB4007491@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqy1mhdurt.fsf@gitster.g>
On Mon, Apr 24, 2023 at 11:01:26AM -0700, Junio C Hamano wrote:
> > + /*
> > + * trim leading whitespace; parse_timestamp() will do this itself, but
> > + * it will walk past the newline at eol while doing so. So we insist
> > + * that there is at least one digit here.
> > + */
>
> "one digit" -> "one non-whitespace".
>
> > + while (dateptr < eol && isspace(*dateptr))
> > + dateptr++;
>
> This is an expected change, but
>
> > + if (!strchr("0123456789", *dateptr))
> > + return 0;
>
> this is not. Isn't the only problematic case that dateptr being at
> eol? That is what the proposed log message argued.
Yes, that would be sufficient. I was moving things slightly closer to
what split_ident_line() does by actually checking for numbers. But that
led to the final paragraph in the commit message explaining how it all
ends up the same either way.
So I'll swap this out for:
if (dateptr == eol)
which I think requires less explanation, as it leaves the function more
like it was originally (and the behavior is the same either way).
> > /* dateptr < eol && *eol == '\n', so parsing will stop at eol */
>
> This comment is slightly stale. dateptr < eol, *eol == '\n', and we
> know the string starting at dateptr is not a run of whitespace and
> that is what makes the parsing stop at eol.
Yeah, I hoped the extra context of the earlier comment would be enough. ;)
But it is probably better to spell it out by expanding this comment.
The code is certainly tricky enough.
-Peff
prev parent reply other threads:[~2023-04-25 5:27 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-22 13:50 [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp Jeff King
2023-04-22 15:53 ` René Scharfe
2023-04-23 0:37 ` Jeff King
2023-04-25 5:56 ` Jeff King
2023-04-24 18:01 ` Junio C Hamano
2023-04-25 5:27 ` Jeff King [this message]
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=20230425052739.GB4007491@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=bockthom@cs.uni-saarland.de \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).