Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> Hi Junio,
>
> On Sun, 23 Apr 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>>
>> > diff --git a/date.c b/date.c
>> > index 92ab31aa441..75f6335cd09 100644
>> > --- a/date.c
>> > +++ b/date.c
>> > @@ -46,7 +46,10 @@ static time_t gm_time_t(timestamp_t time, int tz)
>> > minutes = tz < 0 ? -tz : tz;
>> > minutes = (minutes / 100)*60 + (minutes % 100);
>> > minutes = tz < 0 ? -minutes : minutes;
>> > - return time + minutes * 60;
>> > +
>> > + if (date_overflows(time + minutes * 60))
>> > + die("Timestamp too large for this system: %"PRItime, time);
>> > + return (time_t)time + minutes * 60;
>> > }
>>
>> All the other calls to date_overflows() take a variable that holds
>> timestamp_t and presumably they are checking for integer wraparound
>> when the values are computed, but this one is not.
>
> I was debating whether this extra check is necessary and had decided
> against it. Apparently I was wrong to do so.
Not so fast. Apparently you had thought about it and I didn't think
about it as long as you did, hence I asked ...
>
>> Perhaps we want to make it a bit more careful here? I wonder if
>> something like this is a good approach:
as a question, not a request to change things, and not a "do not
come back until you rewrite it this way" ;-). Getting a question is
not a sign that you were wrong at all.
If you thought it was not needed, and if you can clearly explain to
future readers of "git log" why it is the case, then I am prefectly
fine without an extra check.
In a response to another message, you seem to indicate that existing
code does not do any range checking to make sure ulong (the current
type the existing code uses) does not wrap around and you kept that
"property" of the code in this series (i.e. date_overflows() range
check was merely for downcasting timestamp_t to time_t before giving
the result to the system functions). If that is the case, then NOT
checking for integer wraparound in "+ minutes*60" computation IS the
consistent thing to do within your series, I would think.