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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2020, 20:29 UTC
Message-ID: <xmqq368sd3vu.fsf@gitster.c.googlers.com>
In-Reply-To: <20200424114302.GG1949@danh.dev>

Danh Doan <congdanhqx@gmail.com> writes:

Show 6 quoted lines

> I think single line like:
>
> 	/* We accept 61st second for the single? leap second */
>
> Or something along the time, is good enough. Not sure if we want the
> word "single" there, though.

I do not particularly want to see the single but without it, the single-one comment looks perfect.

Show 20 quoted lines

>> > -		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.
>
> Yes, when I looked around date.c
>
> I saw that the only usecase for validate time is for setting it.
> And the incoming patch also has that usage.
>
> I chose to unify those code path to not buy me too much trouble.
>
> I'll take that "Nicely done" means this unification is OK for this
> series.

The OK was meant for this single place that was converted, not any other place you'd use in the remainder of the series.

And I think it was not such a good idea to use it twice, but I think with the suggested rewrite you took in v5, the other call site is also OK.

Thanks.