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

[Junio C Hamano <gitster@pobox.com>]

Đ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)
{
	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)) {
		...



> 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'