Re: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp
- From
Jeff King <peff@peff.net>
- Date
- Apr 25, 2023, 05:27 UTC
- Message-ID
- <20230425052739.GB4007491@coredump.intra.peff.net>
- In-Reply-To
- <xmqqy1mhdurt.fsf@gitster.g>
On Mon, Apr 24, 2023 at 11:01:26AM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> > + /*
> > + * 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).
Show 5 quoted lines
> > /* 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