Johannes Schindelin [off-list ref] 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). 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(), whose property that overflow will always result in
LONG_MAX gives the robustness of the 2038 test, and needs to be
updated. 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.
The update to check_show Peff suggested is the same as the previous
one, so there is no upside nor downside.
The prerequisite name 64BITTIME that lost an underscore is harder to
read, so there is a slight downside.
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).
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.
quoted hunk
-- snipsnap --
From abe59dbb2235bb1d7aad8e78a66e196acb372ec8 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <redacted>
Date: Tue, 12 Jul 2016 13:19:53 +0200
Subject: [PATCH] t0006: dates absurdly far require a 64-bit data type
Git's source code refers to timestamps as unsigned longs. On 32-bit
platforms, as well as on Windows, unsigned long is not large enough to
capture dates that are "absurdly far in the future".
Let's skip those tests if we know they cannot succeed.
Signed-off-by: Johannes Schindelin <redacted>
---
t/helper/test-date.c | 5 ++++-
t/t0006-date.sh | 6 +++---
t/test-lib.sh | 2 ++
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index d9ab360..1e12d93 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -4,7 +4,8 @@ static const char *usage_msg = "\n"
" test-date relative [time_t]...\n"
" test-date show:<format> [time_t]...\n"
" test-date parse [date]...\n"
-" test-date approxidate [date]...\n";
+" test-date approxidate [date]...\n"
+" test-date is64bit\n";
static void show_relative_dates(char **argv, struct timeval *now)
{@@ -93,6 +94,8 @@ int main(int argc, char **argv)
parse_dates(argv+1, &now);
else if (!strcmp(*argv, "approxidate"))
parse_approxidate(argv+1, &now);
+ else if (!strcmp(*argv, "is64bit"))
+ return sizeof(unsigned long) == 8 ? 0 : 1;
else
usage(usage_msg);
return 0;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 04ce535..52f6b62 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -31,7 +31,7 @@ check_show () {
format=$1
time=$2
expect=$3
- test_expect_${4:-success} "show date ($format:$time)" '
+ test_expect_success $4 "show date ($format:$time)" '
echo "$time -> $expect" >expect &&
test-date show:$format "$time" >actual &&
test_cmp expect actual@@ -50,8 +50,8 @@ check_show iso-local "$TIME" '2016-06-15 14:13:20 +0000'
# arbitrary time absurdly far in the future
FUTURE="5758122296 -0400"
-check_show iso "$FUTURE" "2152-06-19 18:24:56 -0400"
-check_show iso-local "$FUTURE" "2152-06-19 22:24:56 +0000"
+check_show iso "$FUTURE" "2152-06-19 18:24:56 -0400" 64BITTIME
+check_show iso-local "$FUTURE" "2152-06-19 22:24:56 +0000" 64BITTIME
check_parse() {
echo "$1 -> $2" >expectdiff --git a/t/test-lib.sh b/t/test-lib.sh
index 0055ebb..4e1afb0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1111,3 +1111,5 @@ run_with_limited_cmdline () {
}
test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
+
+test_lazy_prereq 64BITTIME 'test-date is64bit'
Hi Junio,
On Wed, 13 Jul 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
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.
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
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:
quoted
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.
quoted
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
Hi Junio,
On Wed, 13 Jul 2016, Junio C Hamano wrote:
Moving of [64BIT_TIME] 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).
Turns out we *already* have another test in `master` that needs this lazy
prereq:
https://github.com/git/git/blob/79ed43c28f/t/t5000-tar-tree.sh#L378-L384
Ciao,
Dscho