Re: [PATCH v4 2/4] date.c: validate and set time in a helper function

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

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

> In a later patch, we will reuse this logic, move it to a helper, now.
>
> While we're at it, explicit states that we intentionally ignore

"explicitly state", perhaps.

Show 20 quoted lines

> old-and-defective 2nd leap second.
>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  date.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/date.c b/date.c
> index b67c5abe24..f5d5a91208 100644
> --- a/date.c
> +++ b/date.c
> @@ -539,6 +539,22 @@ static int set_date(int year, int month, int day, struct tm *now_tm, time_t now,
>  	return -1;
>  }
>  
> +static int set_time(long hour, long minute, long second, struct tm *tm)
> +{
> +	/* C90 and old POSIX accepts 2 leap seconds, it's a defect,
> +	 * ignore second number 61
> +	 */

	/*
	 * Style: our multi-line comments ought to be
	 * formatted like this.  Slash-asterisk that opens,
	 * and asterisk-slash that closes, are both on their
	 * own lines.
	 */

But I am not sure we want to even have a new comment here. After all we are extracting/reinventing exactly the same logic as the original. Why we allow "60" might be worth commenting, but if a minute that has 62 seconds is a mere historical curiosity, then is it worth explaining why "61", which we never even wrote in the code, is missing from here?

Show 10 quoted lines

> +	if (0 <= hour && hour <= 24 &&
> +	    0 <= minute && minute < 60 &&
> +	    0 <= second && second <= 60) {
> +		tm->tm_hour = hour;
> +		tm->tm_min = minute;
> +		tm->tm_sec = second;
> +		return 0;
> +	}
> +	return -1;
> +}

I am a bit surprised to see that you chose to unify with the "check and set" interface of is_date (now set_date). I was expecting to see that we'd have "check-only" helper functions.

This is not a complaint, at least not yet until we see the result of using it in new code; it may very well be possible that the "check and set" interface would make the new caller(s) clearer.

Show 15 quoted lines

>  static int match_multi_number(timestamp_t num, char c, const char *date,
>  			      char *end, struct tm *tm, time_t now)
>  {
> @@ -556,12 +572,8 @@ static int match_multi_number(timestamp_t num, char c, const char *date,
>  	case ':':
>  		if (num3 < 0)
>  			num3 = 0;
> -		if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) {
> -			tm->tm_hour = num;
> -			tm->tm_min = num2;
> -			tm->tm_sec = num3;
> +		if (set_time(num, num2, num3, tm) == 0)
>  			break;
> -		}
>  		return 0;

This caller does become easier to follow, I would say.  Nicely done.

>  	case '-':