On 23/04/2020 20:28, Junio C Hamano wrote:
> Danh Doan <congdanhqx@xxxxxxxxx> writes:
>
>>>> 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 didn't immediately recognise hms and ymd (below) as abbreviations.
>
> 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.
>
>> 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.
>
>>> }
>>>
>>> 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:
Would is_hhmmss() and is_yyyymmdd() be more obvious abbreviations for
most readers?
Now that I type them, they do feel that bit too long... , as naming is
hard, maybe stick with the yms and hms, though I do keep wanting to type
ytd for the former..
>
> 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.
--
Philip
