Re: [ANNOUNCE] Git v2.9.1

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 14, 2016, 07:45 UTC
Message-ID: <alpine.DEB.2.20.1607140913470.6426@virtualbox>
In-Reply-To: <xmqq8tx51hmx.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Wed, 13 Jul 2016, Junio C Hamano wrote:

Show 9 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > How about this one instead (which is part of the time_t-may-be-int64
> > branch on https://github.com/dscho/git which I still have to complete, as
> > some unsigned longs still slipped out of my previous net)? It strikes me
> > as much more robust:
> 
> 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().

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

> whose property that overflow will always result in LONG_MAX gives the
> robustness of the 2038 test, and needs to be updated.

So I got curious and looked at the man page. It says indeed that strtoul()
returns ULONG_MAX, which happens to translate into a date in the year
2038. It seems that this behavior is standardized:

	http://pubs.opengroup.org/onlinepubs/007908775/xsh/strtoul.html

although it does not say that ANSI C requires that behavior.

I also could not fail to notice that negative values will be parsed and simply negated, and that return values 0 and ULONG_MAX *can* denote errors (in which case errno is set, otherwise it is *not* set). Two rather surprising facts, at least surprising to me, and facts that our code does not deal with.

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.

> With this "is64bit" patch, you explicitly measure "unsigned long",
> knowing that our internal time representation currently is that type,
> and that would need to be updated when widening happens.  So both need
> to be updated anyway in the future.

Yes, I already update that in my topic branch.

Please note, however, that it is much more natural to update yet another instance of "unsigned long" to "time_t" than having to explain how that 2038 test is affected.

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.

> The prerequisite name 64BITTIME that lost an underscore is harder to
> read, so there is a slight downside.

It is not a downside. It is something easily fixed.

Show 5 quoted lines

> Moving of lazy_prereq to test-lib might be an upside if we were
> planning to add a test that depends on the system having or not
> having 64-bit timestamp elsewhere, but I do not think of a reason
> why such a new test cannot go to t0006-date, which has the perfect
> name for such a test and is not overly long right now (114 lines).

Happenstance. And I was merely imitating the patch of Peff thar I found on gmane.

> So, unless you have a more solid reason to reject the updated t0006
> earlier in the thread, I am not sure what we'd gain by replacing it
> with this version.

My solid reason is that it is utterky unobvious why the magic number 2038 should do the work. You would have to spend quite some time to convince the average programmer that it is correct.

Contrast that to the 64-bit test.

Ciao, Dscho