On 2020-04-22 10:05:54-0700, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Đoàn Trần Công Danh <congdanhqx@xxxxxxxxx> writes:
>
> > /* Time? Date? */
> > switch (c) {
> > case ':':
> > - if (num3 < 0)
> > + if (num3 < 0) {
> > num3 = 0;
> > + } else if (*end == '.' && isdigit(end[1]) &&
> > + tm->tm_year != -1 && tm->tm_mon != -1 &&
> > + tm->tm_mday != -1) {
> > + /* Attempt to guess meaning of <num> in HHMMSS.<num>
> > + * only interpret as fractional when %Y %m %d is known.
> > + */
> > + strtol(end + 1, &end, 10);
>
> OK, so we saw ':' and parsed <num2> after it, and then saw ':' and
> <num3>, which we know is a good positive number. We haven't checked
> what <num2> is at this point, but it has to be 0 or more digits
> (because we wouldn't have parsed for <num3> if it weren't terminated
> with the same c, i.e. ':').
>
> *end points at the byte that stopped <num3> and we make sure <num3>
> is followed by "." and a digit.
>
> Regardless of what <num2> is, we just discard the "fractional part
> of seconds" (assuming that <num3> is the "seconds" part).
>
> > + }
> > 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.
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.
> }
>
> 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.
And for non-approxidate, when trying to parse date, we initialise now
when guessing which value is year, month, day.
Does it make sense to initialise now from parse_date_basic, passed it
to match_multi_number via match_digit?
To me, it's too much a pain.
I /think/ it isn't worth to warrant a time(NULL) and another is_date
call here. We've checked it one when we set it before, no?
> is_hms(num, num2, num3)) {
> /* Discard ".<num4>" from "HH:MM:SS.<num4>" */
Yeah, <num4> is better comment, since num meant different thing in
this file.
--
Danh
