Re: [PATCH v3 1/2] date.c: skip fractional second part of ISO-8601

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2020, 19:28 UTC
Message-ID: <xmqqmu72gfxu.fsf@gitster.c.googlers.com>
In-Reply-To: <20200423011812.GA1930@danh.dev>

Danh Doan <congdanhqx@gmail.com> writes:

Show 17 quoted lines

>> >  		if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) {
>> >  			tm->tm_hour = num;
>> >  			tm->tm_min = num2;
>> 
>> And after all that is done, if <num2> (and others) are within a
>> reasonable range, we use that as HH:MM:SS.  
>> 
>> OK.  If <num2> (or <num3>, or even <num> for that matter) weren't
>> reasonable, is it still sensible to discard the fractional part?
>> The answer is not immediately obvious to me.
>> 
>> To be safe, it might make sense to extract a helper function from
>> the next conditional, i.e.
>> 
>> static int is_hms(long num1, long num2, long num3)
>
> I'll make it `is_time` on par with is_date check.

That is probably a lot more readable name than is_hms().

I do not worry too much if the name is not "on par with" it, though, because is_date() does more than just "check", as you noticed below, unlike the "is hour-minute-seconds are within reasonable range?" check.

Show 13 quoted lines

> I'll look into it and check if int or long is suitable for parameter's
> type.
>
>> {
>> 	return (0 <= num1 && num1 < 25 &&
>> 		0 <= num2 && num2 < 60 &&
>> 		0 <= num3 && num3 <= 60);
>
> Does it worth to add an explicit comment that we intentionally ignore
> the old-and-defective 2nd leap seconds (i.e. second with value 61)?
>
> I saw in one of your previous email doubting if we should check for
> `num3 <= 61` or not.

I wrote that without checking anything, even what our own code does. As the existing code seems to want to work only with a second part that is 60 or less, not allowing a minute with 62 seconds, I think sticking to that and saying "0 <= num3 && num3 <= 60" is good.

Show 10 quoted lines

>> }
>> 
>> and use it in the new "else if" block like so?
>> 
>> 
>> 	} else if (*end == '.' && isdigit(end[1]) &&
>> 		   is_date(tm->tm_year, tm->tm_mon, tm->tm_mday, NULL, now, tm) &&
>
> When running into this, the code patch for non-approxidate hasn't
> initialised value for now, yet.

We may want to separate the logic that relies on the value of 'now' and 'now_tm->tm_year' out of is_date() to make it more easily reusable. In this generic codepath, for example, we do not necessarily want to say "we refuse to parse timestamp that is 10 days or more into the future".

The longer I stare at is_date(), the more I am inclined to say it is a bit of impedance mismatch, and we instead should have the is_hms() helper as I suggested in the message you are responding to, plus something like:

static int is_ymd(int year, int month, int day)
{
        /* like tm_to_time_t(), we only work between 1970-2099 */
	static const int days_in_month[] = {
		31, 28, 31, 30, ..., 31
	};

	return !(year < 1970 || 2100 <= year ||
		month <= 0 || 13 <= month ||
		day <= 0 || year / 4 + days_in_month[month-1] <= day);
}

and use it here. I am not sure if we can reuse this inside is_date(), but if we can do so that would be good (and if we cannot, that is fine, too).

Thanks.