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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2020, 17:05 UTC
Message-ID: <xmqqk127jvrh.fsf@gitster.c.googlers.com>
In-Reply-To: <c6d42785bb762f691b00c48b57c73a1933fadbc3.1587559135.git.congdanhqx@gmail.com>

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Show 13 quoted lines

>  	/* 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)
{
	return (0 <= num1 && num1 < 25 &&
		0 <= num2 && num2 < 60 &&
		0 <= num3 && num3 <= 60);
}

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) &&
		   is_hms(num, num2, num3)) {
		/* Discard ".<num4>" from "HH:MM:SS.<num4>" */
		(void) strtol(end + 1, &end, 10);
	}

	if (is_hms(num, num2, num3)) {
		...

Show 21 quoted lines

> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index d9fcc829a9..80917c81c3 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -81,6 +81,8 @@ check_parse 2008-02 bad
>  check_parse 2008-02-14 bad
>  check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'
>  check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'
> +check_parse '2008.02.14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'
> +check_parse '2008-02-14 20:30:45.019-04:00' '2008-02-14 20:30:45 -0400'
>  check_parse '2008-02-14 20:30:45 -0015' '2008-02-14 20:30:45 -0015'
>  check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 +0000'
>  check_parse '2008-02-14 20:30:45 -5:' '2008-02-14 20:30:45 +0000'
> @@ -103,6 +105,7 @@ check_approxidate 5.seconds.ago '2009-08-30 19:19:55'
>  check_approxidate 10.minutes.ago '2009-08-30 19:10:00'
>  check_approxidate yesterday '2009-08-29 19:20:00'
>  check_approxidate 3.days.ago '2009-08-27 19:20:00'
> +check_approxidate '12:34:56.3.days.ago' '2009-08-27 12:34:56'
>  check_approxidate 3.weeks.ago '2009-08-09 19:20:00'
>  check_approxidate 3.months.ago '2009-05-30 19:20:00'
>  check_approxidate 2.years.3.months.ago '2007-05-30 19:20:00'