Re: [PATCH v4 3/4] date.c: skip fractional second part of ISO-8601

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2020, 20:29 UTC
Message-ID: <xmqqv9lqeyji.fsf@gitster.c.googlers.com>
In-Reply-To: <8b18d0ee5d6f08394e54e16ca7618c687791a509.1587644889.git.congdanhqx@gmail.com>

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

Show 10 quoted lines

> -		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 &&
> +			   set_time(num, num2, num3, tm) == 0) {
> +			/* %Y%m%d is known, ignore fractional <num4> in HHMMSS.<num4> */
> +			strtol(end + 1, &end, 10);
> +		}
>  		if (set_time(num, num2, num3, tm) == 0)

Hmmm.

While calling set_time() on the same 4-tuple might be idempotent when it succeeds, the call with a potential new side effect in the conditional part leaves a bad taste in my mouth.

I am wondering if the follwoing would be a more readable alternative.

 date.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/date.c b/date.c
index f5d5a91208..3eb9b6032c 100644
--- a/date.c
+++ b/date.c
@@ -572,8 +572,16 @@ static int match_multi_number(timestamp_t num, char c, const char *date,
 	case ':':
 		if (num3 < 0)
 			num3 = 0;
-		if (set_time(num, num2, num3, tm) == 0)
+		if (set_time(num, num2, num3, tm) == 0) {
+			/* 
+			 * Is the "HH:MM:SS" we just parsed followed by
+			 *  ".<num4>" (fractinal second)?  Discard it
+			 * only when we already have YY/MM/DD.
+			 */
+			if (*end == '.' && isdigit(end[1]) && date_known(tm))
+				(void) strtol(end + 1, &end, 10);
 			break;
+		}
 		return 0;
 
 	case '-':