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

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

Danh Doan <congdanhqx@gmail.com> writes:

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

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