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
