Re: [PATCH 0/6] Use time_t

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 28, 2017, 11:32 UTC
Message-ID: <alpine.DEB.2.20.1702281211270.3767@virtualbox>
In-Reply-To: <xmqqlgsrmesc.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Mon, 27 Feb 2017, Junio C Hamano wrote:

Show 13 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > One notable fallout of this patch series is that on 64-bit Linux (and
> > other platforms where `unsigned long` is 64-bit), we now limit the
> > range of dates to LONG_MAX (i.e. the *signed* maximum value). This
> > needs to be done as `time_t` can be signed (and indeed is at least on
> > my Ubuntu setup).
> >
> > Obviously, I think that we can live with that, and I hope that all
> > interested parties agree.
> 
> s/ulong/time_t/ is definintely a good change, and it will take us to a
> place we would want to be in in some future.

Actually. I have to take back the part where I hoped that all interested parties would agree. The problem is 32-bit Linux:

	$ cat >a1.c <<-\EOF
	#include <stdio.h>
	#include <limits.h>
	#include <time.h>

	int main(int argc, char **argv)
	{
		printf("sizeof(long): %d, sizeof(time_t): %d, ulong_max: %lu\n",
		       (int)sizeof(long), (int)sizeof(time_t), ULONG_MAX);
		return 0;
	}
	EOF

	$ gcc -m32 -Wall -o a1 a1.c

	$ ./a1
	sizeof(long): 4, sizeof(time_t): 4, ulong_max: 4294967295

So. Not only is `long` a 32-bit on 32-bit Linux, but so is `time_t`. And with that, switching from `ULONG_MAX` as the maximal time we can represent in Git to `LONG_MAX` is kind of a serious problem.

> As long as there remains no platform we care about whose time_t and long
> are still 32-bit signed integer, there will be a fallout to them with
> this change.

Sorry, I do not understand the verb "remains" in conjunction with "no platform"...

Do you mean to say that currently no platform we care about has 32-bit signed time_t/long?

If so, I just demonstrated this to be unfortunately incorrect.

> It appears that we use uint64_t in many places in our code.  So
> while philosophically time_t is the right type, uint64_t might be
> practically a safer alternative type to use at the endgame patch in
> this series.

Yes, I think you are right. We should use uint64_t instead of time_t, but *semantically* we should not even use uint64_t. We should introduce our own data type instead of repeating the mistake to use a data type that does not convey its role to the reader.

Currently, I am favoring timestamp_t.

> I haven't seen it yet, but presumably the last one 6/6 is the endgame?

Maybe it took a while to get sent out, but it made it into public inbox: http://public-inbox.org/git/75efe76cbb0636741a7c3aec9e21459bc1dc3cbe.1488231002.git.johannes.schindelin@gmx.de/

Ciao, Johannes