Re: [ANNOUNCE] Git v2.9.1

From: Jeff King <peff@peff.net>
Date: Jul 14, 2016, 08:15 UTC
Message-ID: <20160714081503.GA12946@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1607140913470.6426@virtualbox>

tl;dr I don't really care which fix goes in. They are both fine with me, and in practice I cannot imagine either causing a big problem. But here are my thoughts because I know you want them.

On Thu, Jul 14, 2016 at 09:45:12AM +0200, Johannes Schindelin wrote:

Show 8 quoted lines

> > Hmm, sorry, I do not see much upside here (iow, the 2038 test you
> > came up with is as robust).
> 
> Unless you, or Peff, performed a thorough analysis whether the dates are
> always cut off at 2038 holds true, I am highly doubtful that the previous
> tes is robust at all. I certainly only tested on Windows and never
> investigated how that 2038 came about. For what I know, it might be a
> platform-dependent behavior of strtoul().

I think that when a long is 32-bit signed, you will always get 2038 from strtol. There could be systems where that is the case, though, and time_t is of a different size. I'm not sure how much it would be worth caring about them.

One nice thing about looking for "if we got 2038, we know we can skip" as opposed to "did we correctly format this to 2286" is that we err on the side of failing the test. So if we did ever find such an oddball platform, the test would fail and we could address it then.

Show 5 quoted lines

> > When the internal time representation is updated from "unsigned long" to
> > a signed and wider type [*1*], test-date has to stop reading the
> > second-from-epoch input with strtol(),
> 
> It's strtoul(), actually.

I think both you and Junio are mistaken in the quoted text. :)

The code in question is in t/helper/test-date.c:show_dates(), and _does_ call the signed strtol(). However, it is storing it not in an "unsigned long" (which would be utterly silly), but in a time_t.

And the value is clamped to LONG_MAX there, so the representation elsewhere does not matter at all, as long as it big enough to store LONG_MAX. By definition, "unsigned long" should be. In practice, I'd guess time_t is, though perhaps one could come up with a case of compiling a 64-bit program against a 32-bit ABI? I don't know if that's possible.

That also explains why we get 2038 here, and not our usual sentinel value of "(time_t)0". We _do_ have overflow checks when formatting pretty-printed dates from commits (see show_ident_date), but the test helper isn't using them.

> Please also note that ULONG_MAX is not required to be either 2^32-1 or
> 2^64-1. Which means that the 2038 test is really not robust.

Of course not; but as I mentioned above, I think the test can be written to complain in the unlikely case that it is not one of those, and we can deal with it then.

> Also note that the 640bit test is very explicit, and hence robust. As a
> consequence it would skip the absurd dates on systems switching to
> int128_t for time_t.

Actually, I think that is a bad thing. The case that the test in question was added for was not about overflowing "unsigned long", but about having a far-future date that tm_to_time_t() could not handle. And that maxes out at 2100. Testing it on a 128-bit system would be completely appropriate.

-Peff