Re: [PATCH v3 0/8] Introduce timestamp_t for timestamps

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 21, 2017, 10:44 UTC
Message-ID: <alpine.DEB.2.20.1704211219460.3480@virtualbox>
In-Reply-To: <xmqqpog647of.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Thu, 20 Apr 2017, Junio C Hamano wrote:

Show 16 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Note: while the `time_t` data type exists and is meant to be used for
> > timestamps, on 32-bit Linux it is *still* 32-bit. An earlier iteration
> > used `time_t` for that reason, but it came with a few serious
> > downsides: as `time_t` can be signed (and indeed, on Windows it is an
> > int64_t), Git's expectation that 0 is the minimal value does no longer
> > hold true, introducing its own set of interesting challenges. Besides,
> > if we *can* handle far in the future timestamps (except for formatting
> > them using the system libraries), it is more consistent to do so.
> 
> I somehow had an impression that the list consensus during the
> discussion on an earlier round of this series was that time_t is
> more appropriate, as platforms with time_t with inadequent range
> will be updated before it gets too late at around 2038 (or they will
> die off).

There was this sentiment, but that would require a change in Git's source code where it can handle unsigned *and* signed data types for timestamps, and that was too much of a change to bring in that late in the patch series.

Besides, supporting a signed timestamp data type is a separate issue from trying to support dates that are insanely far in the future.

> After all, at some point we need to interact with the
> platform functions that expect time_t as their interface and they do
> not take our own timestamp_t without casting.

No, not necessarily. When we generate a .zip archive, for example, we may never need to parse or format the timestamps. When we log with a format that does not require the system routines to format the date (e.g. Unix epoch). And when we do not parse nor format the timestamps to begin with, e.g. fetching and pushing or committing.

It is *only* when we try to parse or format timestamps, *and* the system's time_t is not large enough, that we run into trouble.

This is an improvement from before, where we would run into trouble whenever time_t was larger than unsigned long, *or* whenever time_t is too small for those timestamps.

Your comment, and my reaction, reminded me that I had planned to look into all calls to date_overflow() and remove those that are now unnecessary, or at least move them to the place where they are absolutely necessary.

> But that is provided if not introducing timestamp_t and using time_t
> results in a simpler code.  I do not know if that is the case.

As I had pointed out in my reply to Peff: removing the assumption that 0 is the minimal timestamp is something I am unwilling to tackle, it looks way too fragile/dangerous to me to do this in the time I could allocate for that.

Show 11 quoted lines

> For timestamps in the distant past, even though time_t could be
> unsigned, I do not think anybody came up with a concrete example of a
> platform with such a problem during the previous discussions, while I do
> recall people wanting to use Git to store historical documents with
> timestamps before 1970.  We do expect 0 can be used as a sentinel, which
> needs to be updated once we seriously start supporting such use cases.
> I think that (i.e. should the timestamp be signed?) is more or less
> unrelated to the focus of the discussion that stemed from this topic,
> which was "ulong that is often 32-bit does not necessarily fit the
> notion of time on a platform, which is time_t, and we need to widen it",
> which all involved in the discussion agreed.

If anybody feels strongly enough about representing timestamps earlier than the Jan 1 1970, they should feel very welcome to work on this.

My patch series would make that task slightly easier, even.

> In any case, when merged to 'pu', this had a slight conflict with
> topics in flight in builtin/name-rev.c and I think I resolved it
> correctly, but please double check.

I double-checked, there were just two conversions from `unsigned long taggerdate` to `timestamp_t taggerdate`. I repeated the merge with my latest iteration and resolved them the same way as you did.

Apart from the date_overflows() changes I mentioned above, I also fixed the problems identified by Travis CI and VSTS Build.

v4 is coming, Dscho