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

From: Jeff King <peff@peff.net>
Date: Mar 26, 2014, 21:09 UTC
Message-ID: <20140326210929.GA16581@sigill.intra.peff.net>
In-Reply-To: <xmqqeh1oisim.fsf@gitster.dls.corp.google.com>

On Wed, Mar 26, 2014 at 02:01:21PM -0700, Junio C Hamano wrote:

Show 7 quoted lines

> > 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 didn't realize it at the time I wrote the test, either. It was my goal to recognize such failures, but I didn't now at the time that there were failures that would be signaled by anything besides a NULL return.

Show 5 quoted lines

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

Agreed. I am not sure we can do so. For FreeBSD, I think it is not hard. I am not sure what the pattern is for AIX. I am hoping Charles will reveal some pattern, but there may not be one.

> But if that is the case, we should not have to maintain special case
> values in the test code, I would think.

Right. All of my suggestions to accommodate special values in the test were assuming that the system gmtime was smarter than we are. That it produced a good value for this crazy time and _didn't_ fail.

But after having done the basic math, I don't think that is what is going on here.

Show 5 quoted lines

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

Yes, exactly. That is the preferable solution, if we can come up with such logic. Personally I am not optimistic.

The fallback is to accept that we cannot cover all cases, and just loosen the test (i.e., the second patch I posted today).

-Peff