Re: [PATCH v3 3/4] parse_commit(): handle broken whitespace-only timestamp

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 27, 2023, 16:20 UTC
Message-ID: <xmqqpm7pl2je.fsf@gitster.g>
In-Reply-To: <a04e7950-b74e-d43f-4d19-86def079748c@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 13 quoted lines

>> I did also allow "-" here, which may be controversial, as we don't
>> currently support negative timestamps. My reasoning was two-fold. One,
>> the design of parse_timestamp() is such that we should be able to easily
>> switch it to handling signed values, and this otherwise creates a
>> hard-to-find gotcha that anybody doing that work would get tripped up
>> on. And two, the status quo is that we currently parse them, though the
>> result of course ends up as a very large unsigned value (which is likely
>> to just get clamped to "0" for display anyway, since our date routines
>> can't handle it).
>
> I think this makes a good case for accepting '-'. The commit message
> is well explained as always :-) This all looks good to me apart from a
> query about one of the tests.

I agree. I was somewhat surprised that the big comment before that code did not mention it, but hopefully those who would be tempted to remove the check for '-' would either be careful enough themselves or be stopped by reviewers who are careful enough to go back to the log message of the commit that added the check in the first place, so it is OK.