Re: [ANNOUNCE] Git v2.9.1

Johannes Schindelin [off-list ref] writes:

Git's source code assumes that unsigned long is at least as precise as
time_t. Well, Git's source code is wrong.
...

That is correct.  As people mentioned downthread already, "unsigned
long" has two problems, it may not be wide enough, and it cannot
represent time before the epoch.

But moving the internal time representation used in various fields
like commit->date to time_t is likely to be a wrong thing to do,
because the first problem with "unsigned long", i.e. "may not be
wide enough", is not limited to "not wide enough to hold time_t".
It also includes "it may not be wide enough to hold time somebody
else recorded in existing objects".

Since some platforms have time_t that is not wide enough, but still
have intmax_t that is wider, I think we would be better off to pick
an integral type to use for the internal representation that is the
widest throughout the API, and use time_t only at places that we
interact with the system libraries (e.g. when we ask "what is the
time now?" to time(2), when we ask "break down this timestamp" to
gmtime(3)).

Thanks for starting this, and from a brief read, the hotfix to skip
the test downthread looked good.  The places this "starting point"
patch covers look like a good set that interacts with time obtained
locally (e.g. prune that compares with filesystem timestamp); just
make sure you don't go too far and end up shoving timestamps from
other people into time_t, which may not fit.

Thanks.

On Tue, Jul 12, 2016 at 07:34:14AM -0700, Junio C Hamano wrote:

quoted

Git's source code assumes that unsigned long is at least as precise as
time_t. Well, Git's source code is wrong.
...

That is correct.  As people mentioned downthread already, "unsigned
long" has two problems, it may not be wide enough, and it cannot
represent time before the epoch.

But moving the internal time representation used in various fields
like commit->date to time_t is likely to be a wrong thing to do,
because the first problem with "unsigned long", i.e. "may not be
wide enough", is not limited to "not wide enough to hold time_t".
It also includes "it may not be wide enough to hold time somebody
else recorded in existing objects".

But that's a problem no matter what size we choose. The ascii format in
the commit objects is arbitrary-length, so somebody can always overflow
it. So even with intmax_t we have to clamp it to a sentinel value at
some point. IMHO we are better off to do so at parse time, and then have
consistent sizes through the rest of the code base, without worrying
about juggling intmax_t to time_t truncation in multiple places.

IOW, I think we probably interact with the system time libraries more
often than we parse (and it's easy to wrap the parsing in a function,
but there are a lot of system time functions).

-Peff

On Tue, Jul 12, 2016 at 8:16 AM, Jeff King [off-list ref] wrote:

quoted

But moving the internal time representation used in various fields
like commit->date to time_t is likely to be a wrong thing to do,
because the first problem with "unsigned long", i.e. "may not be
wide enough", is not limited to "not wide enough to hold time_t".
It also includes "it may not be wide enough to hold time somebody
else recorded in existing objects".

But that's a problem no matter what size we choose.

Yes, if somebody's time_t is larger than my intmax_t, the problem
cannot be solved for me, if that timestamp is too far in the future or
in the past.

But that is not the problem I am pointing out. I heard earlier in the
thread that time_t on one system was 32-bit (was it Linux?) but I think
they have "long long". Choosing time_t is strictly inferior choice when
we already know that a platform with not-wide-enough time_t need to
be supported, and a type that is wider than that is available.

I was envisioning that we would have typedef <sometime> gittime_t
with conversion helpers between it and time_t that allow us to do some
range checks while at it.

On Tue, Jul 12, 2016 at 08:25:51AM -0700, Junio C Hamano wrote:

On Tue, Jul 12, 2016 at 8:16 AM, Jeff King [off-list ref] wrote:

quoted

quoted

But moving the internal time representation used in various fields
like commit->date to time_t is likely to be a wrong thing to do,
because the first problem with "unsigned long", i.e. "may not be
wide enough", is not limited to "not wide enough to hold time_t".
It also includes "it may not be wide enough to hold time somebody
else recorded in existing objects".

But that's a problem no matter what size we choose.

Yes, if somebody's time_t is larger than my intmax_t, the problem
cannot be solved for me, if that timestamp is too far in the future or
in the past.

I am less worried about their time_t and more about whatever crap they
write in ascii into their objects. :)

But that is not the problem I am pointing out. I heard earlier in the
thread that time_t on one system was 32-bit (was it Linux?) but I think
they have "long long". Choosing time_t is strictly inferior choice when
we already know that a platform with not-wide-enough time_t need to
be supported, and a type that is wider than that is available.

I am not certain that there is a modern system with 32-bit time_t. We
know there are systems with 32-bit unsigned long, and I think that is
what produced the results people saw. I'd expect even 32-bit systems to
use "int64_t" or similar for their time_t these days.

I'm also not convinced that we would be helping much to carry around a
wider gittime_t. Most of the display code ends up touching a system time
function one way or another, so I find it unlikely it would produce much
better output.

It would help for simple cases like commit->date where we really do just
parse it into a number and never do more with it. But...

I was envisioning that we would have typedef <sometime> gittime_t
with conversion helpers between it and time_t that allow us to do some
range checks while at it.

I guess I am just willing to trust that time_t is basically that. And if
your platform has a grossly undersized time_t, then too bad, we clamp
everything it can't hold to 2038 or whatever, and hopefully your
terrible platform dies out or gets a clue sometime in the next 20 years.

-Peff