Re: [PATCH 4/5] log: handle integer overflow in timestamps

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2014, 21:01 UTC
Message-ID: <xmqqy5106x8a.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140224203709.GA25506@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 27 quoted lines

> On Mon, Feb 24, 2014 at 12:21:33PM -0800, Junio C Hamano wrote:
>
>> >> > +	if (date_overflows(date))
>> >> > +		date = 0;
>> >> > +	else {
>> >> > +		if (ident->tz_begin && ident->tz_end)
>> >> > +			tz = strtol(ident->tz_begin, NULL, 10);
>> >> > +		if (tz == LONG_MAX || tz == LONG_MIN)
>> >> > +			tz = 0;
>> >> > +	}
>> >> 
>> >> ... don't we want to fix an input having a bogus timestamp and also
>> >> a bogus tz recorded in it?
>> >
>> > If there is a bogus timestamp, then we do not want to look at tz at all.
>> > We leave it at "0", so that we get a true sentinel:
>> 
>> Ah, OK, I missed the initialization to 0 at the beginning.
>> 
>> It might have been more clear if "int tz" declaration were left
>> uninitialized, and the variable were explicitly cleared to 0 in the
>> "date-overflows" error codepath, but it is not a big deal.
>
> It might be, but I think it would end up cumbersome.
> ...
> So I'd be in favor of keeping it as-is, but feel free to mark it up if
> you feel strongly.

I'd be in favor of keeping it as-is.