Re: [PATCH 5/5] log: do not segfault on gmtime errors

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 26, 2014, 21:01 UTC
Message-ID: <xmqqeh1oisim.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140326190157.GB12912@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 25 quoted lines

> On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote:
>
>> > Unlike the FreeBSD thing that René brought up, this is not a problem in
>> > the code, but just in the test. So I think our options are basically:
>> >
>> >   1. Scrap the test as unportable.
>> >
>> >   2. Hard-code a few expected values. I'd be unsurprised if some other
>> >      system comes up with a slightly different date in 162396404, so we
>> >      may end up needing several of these.
>> >
>> > I think I'd lean towards (2), just because it is testing an actual
>> > feature of the code, and I'd like to continue doing so. And while we may
>> > end up with a handful of values, there's probably not _that_ many
>> > independent implementations of gmtime in the wild.
>> 
>> Or "3. Just make sure that 'git log' does not segfault"?
>
> That would not test the FreeBSD case, which does not segfault, but
> returns a bogus sentinel value.
>
> I don't know how important that is. This is such a minor feature that it
> is not worth a lot of maintenance headache in the test. But I also do
> not know if this is going to be the last report, or we will have a bunch
> of other systems that need their own special values put into the test.

I didn't quite realize that your objective of the change was to signal the failure of gmtime for all implementations, i.e. both (1) the ones that signal an error by giving NULL back to us and (2) the ones that fail to signal an error but leave bogus result (FreeBSD, but it appears AIX might be also giving us another bogus value), by using a single sane sentinel value. If we can do that consistently on every platform, that would indeed be great.

But if that is the case, we should not have to maintain special case values in the test code, I would think. The test instead should expect the output to have that single sentinel value, and if the workaround code fails to detect a breakage in the platform gmtime(), the special case logic to catch these platform-specific breakages should go that "timestamp that cannot be grokked by gmtime---replace it with a sentinel" logic, no?