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

From: Danh Doan <congdanhqx@gmail.com>
Date: Apr 23, 2020, 01:18 UTC
Message-ID: <20200423011812.GA1930@danh.dev>
In-Reply-To: <xmqqk127jvrh.fsf@gitster.c.googlers.com>

On 2020-04-22 10:05:54-0700, Junio C Hamano <gitster@pobox.com> wrote:

Show 44 quoted lines

> Đoàn Trần Công Danh  <congdanhqx@gmail.com> 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.

Show 7 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.

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