# [PATCH 1/2] t0006: test timezone parsing 11 messages from 2010-07-04 to 2010-07-07. Participants: Jeff King, Junio C Hamano, Brandon Casey, Alex Riesen, Andreas Schwab. Thread: https://gitlist.dev/t/24284 ## Jeff King, 2010-07-04 10:48 Subject: [PATCH 1/2] t0006: test timezone parsing Message-ID: <20100704104834.GA23070@sigill.intra.peff.net> URL: https://gitlist.dev/e/20100704104834.GA23070%40sigill.intra.peff.net ``` Previously, test-date simply ignored the parsed timezone and told show_date() to use UTC. Instead, let's print out what we actually parsed. While we're at it, let's make it easy for tests to work in a specific timezone. Signed-off-by: Jeff King --- This is a good idea anyway, but it also makes it easy to write a test for the next patch. t/t0006-date.sh | 5 +++-- test-date.c | 9 ++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/t/t0006-date.sh b/t/t0006-date.sh index 75b02af..3ea4f9e 100755 --- a/t/t0006-date.sh +++ b/t/t0006-date.sh @@ -28,8 +28,8 @@ check_show 31449600 '12 months ago' check_parse() { echo "$1 -> $2" >expect - test_expect_${3:-success} "parse date ($1)" " - test-date parse '$1' >actual && + test_expect_${4:-success} "parse date ($1${3:+ TZ=$3})" " + TZ=${3:-$TZ} test-date parse '$1' >actual && test_cmp expect actual " } @@ -38,6 +38,7 @@ check_parse 2008 bad check_parse 2008-02 bad check_parse 2008-02-14 bad check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000' +check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500' check_approxidate() { echo "$1 -> $2 +0000" >expect diff --git a/test-date.c b/test-date.c index a9e705f..ac6854a 100644 --- a/test-date.c +++ b/test-date.c @@ -21,12 +21,15 @@ static void parse_dates(char **argv, struct timeval *now) for (; *argv; argv++) { char result[100]; time_t t; + int tz; result[0] = 0; parse_date(*argv, result, sizeof(result)); - t = strtoul(result, NULL, 0); - printf("%s -> %s\n", *argv, - t ? show_date(t, 0, DATE_ISO8601) : "bad"); + if (sscanf(result, "%ld %d", &t, &tz) == 2) + printf("%s -> %s\n", + *argv, show_date(t, tz, DATE_ISO8601)); + else + printf("%s -> bad\n", *argv); } } -- 1.7.2.rc1.209.g2a36c ``` ## Jeff King, 2010-07-04 11:00 Subject: [PATCH 2/2] parse_date: fix signedness in timezone calculation Message-ID: <20100704110017.GA23121@sigill.intra.peff.net> URL: https://gitlist.dev/e/20100704110017.GA23121%40sigill.intra.peff.net In-Reply-To: <20100704104834.GA23070@sigill.intra.peff.net> ``` When no timezone is specified, we deduce the offset by subtracting the result of mktime from our calculated timestamp. However, our timestamp is stored as an unsigned integer, meaning we perform the subtraction as unsigned. For a negative offset, this means we wrap to a very high number, and our numeric timezone is in the millions of hours. You can see this bug by doing: $ TZ=EST \ GIT_AUTHOR_DATE='2010-06-01 10:00' \ git commit -a -m foo $ git cat-file -p HEAD | grep author author Jeff King 1275404416 +119304128 Instead, we should perform this subtraction as a time_t, the same type that mktime returns. Signed-off-by: Jeff King --- Are there any platforms with an unsigned time_t? In that case, we would need to use a regular "long" to get signedness. Presumably on such platforms "long" will grow with time_t to 64-bits so we don't create a Y2038 problem (though most of the rest of the code uses "unsigned long" interchangeably with time_t, so we have a Y2106 problem in that case anyway). date.c | 2 +- t/t0006-date.sh | 1 + 2 files changed, 2 insertions(+), 1 deletions(-) diff --git a/date.c b/date.c index 68cdcaa..3c981f7 100644 --- a/date.c +++ b/date.c @@ -635,7 +635,7 @@ int parse_date_toffset(const char *date, unsigned long *timestamp, int *offset) /* mktime uses local timezone */ *timestamp = tm_to_time_t(&tm); if (*offset == -1) - *offset = (*timestamp - mktime(&tm)) / 60; + *offset = ((time_t)*timestamp - mktime(&tm)) / 60; if (*timestamp == -1) return -1; diff --git a/t/t0006-date.sh b/t/t0006-date.sh index 3ea4f9e..b2df4fe 100755 --- a/t/t0006-date.sh +++ b/t/t0006-date.sh @@ -39,6 +39,7 @@ check_parse 2008-02 bad check_parse 2008-02-14 bad check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000' check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500' +check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST check_approxidate() { echo "$1 -> $2 +0000" >expect -- 1.7.2.rc1.209.g2a36c ``` ## Junio C Hamano, 2010-07-06 02:35 Subject: Re: [PATCH 2/2] parse_date: fix signedness in timezone calculation Message-ID: <7vy6dp72wh.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6dp72wh.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100704110017.GA23121@sigill.intra.peff.net> ``` Thanks; both patches make sense to me. ``` ## Junio C Hamano, 2010-07-06 07:01 Subject: Re: [PATCH 1/2] t0006: test timezone parsing Message-ID: <7vpqz13xgw.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpqz13xgw.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100704104834.GA23070@sigill.intra.peff.net> ``` Jeff King writes: > diff --git a/test-date.c b/test-date.c > index a9e705f..ac6854a 100644 > --- a/test-date.c > +++ b/test-date.c > @@ -21,12 +21,15 @@ static void parse_dates(char **argv, struct timeval *now) > for (; *argv; argv++) { > char result[100]; > time_t t; > + int tz; > > result[0] = 0; > parse_date(*argv, result, sizeof(result)); > - t = strtoul(result, NULL, 0); > - printf("%s -> %s\n", *argv, > - t ? show_date(t, 0, DATE_ISO8601) : "bad"); > + if (sscanf(result, "%ld %d", &t, &tz) == 2) Gah... On FreeBSD 8.0, we now see this. cc1: warnings being treated as errors test-date.c: In function 'parse_dates': test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *' ``` ## Jeff King, 2010-07-06 07:28 Subject: Re: [PATCH 1/2] t0006: test timezone parsing Message-ID: <20100706072849.GA3567@sigill.intra.peff.net> URL: https://gitlist.dev/e/20100706072849.GA3567%40sigill.intra.peff.net In-Reply-To: <7vpqz13xgw.fsf@alter.siamese.dyndns.org> ``` On Tue, Jul 06, 2010 at 12:01:35AM -0700, Junio C Hamano wrote: > > time_t t; > > + int tz; > [...] > > + if (sscanf(result, "%ld %d", &t, &tz) == 2) > > Gah... > > On FreeBSD 8.0, we now see this. > > cc1: warnings being treated as errors > test-date.c: In function 'parse_dates': > test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *' Meh. I was worried about that when I used sscanf. I think we can just do this: diff --git a/test-date.c b/test-date.c index ac6854a..6bcd5b0 100644 --- a/test-date.c +++ b/test-date.c @@ -20,12 +20,12 @@ static void parse_dates(char **argv, struct timeval *now) { for (; *argv; argv++) { char result[100]; - time_t t; + unsigned long t; int tz; result[0] = 0; parse_date(*argv, result, sizeof(result)); - if (sscanf(result, "%ld %d", &t, &tz) == 2) + if (sscanf(result, "%lu %d", &t, &tz) == 2) printf("%s -> %s\n", *argv, show_date(t, tz, DATE_ISO8601)); else as show_date takes an unsigned long anyway. -Peff ``` ## Jeff King, 2010-07-06 07:54 Subject: Re: [PATCH 1/2] t0006: test timezone parsing Message-ID: <20100706075433.GD3567@sigill.intra.peff.net> URL: https://gitlist.dev/e/20100706075433.GD3567%40sigill.intra.peff.net In-Reply-To: <20100706072849.GA3567@sigill.intra.peff.net> ``` On Tue, Jul 06, 2010 at 03:28:49AM -0400, Jeff King wrote: > > On FreeBSD 8.0, we now see this. > > > > cc1: warnings being treated as errors > > test-date.c: In function 'parse_dates': > > test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *' > > Meh. I was worried about that when I used sscanf. I think we can just do > this: Oh, I see this is already on maint. :) So here is my fixup in a nicer form: -- >8 -- Subject: [PATCH] test-date: fix sscanf type conversion Reading into a time_t isn't portable, since we don't know the exact type. Instead, use an unsigned long, which is what show_date wants, anyway. Signed-off-by: Jeff King --- test-date.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test-date.c b/test-date.c index ac6854a..6bcd5b0 100644 --- a/test-date.c +++ b/test-date.c @@ -20,12 +20,12 @@ static void parse_dates(char **argv, struct timeval *now) { for (; *argv; argv++) { char result[100]; - time_t t; + unsigned long t; int tz; result[0] = 0; parse_date(*argv, result, sizeof(result)); - if (sscanf(result, "%ld %d", &t, &tz) == 2) + if (sscanf(result, "%lu %d", &t, &tz) == 2) printf("%s -> %s\n", *argv, show_date(t, tz, DATE_ISO8601)); else -- 1.7.2.rc1.214.g5a9d0 ``` ## Brandon Casey, 2010-07-06 23:34 Subject: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX Message-ID: URL: https://gitlist.dev/e/upBu73z-yHJSTQ58c-Br0BxGwDzKGa1fPzZXA71AfI3kG3dMsRRPOX76JRJVhEOcDGoNaf1z9Lg%40cipher.nrlssc.navy.mil In-Reply-To: <20100704110017.GA23121@sigill.intra.peff.net> ``` From: Brandon Casey POSIX requires that both the timezone "standard" and "offset" be specified in the TZ environment variable. This causes a problem on IRIX which does not understand the timezone 'EST'. Signed-off-by: Brandon Casey --- I guess 'EST' is an alias? for EST5EDT? Linux and Solaris both grok EST just fine. POSIX says the offset is required. I don't see any mention of aliases. -brandon t/t0006-date.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/t/t0006-date.sh b/t/t0006-date.sh index b2df4fe..1d4d0a5 100755 --- a/t/t0006-date.sh +++ b/t/t0006-date.sh @@ -39,7 +39,7 @@ check_parse 2008-02 bad check_parse 2008-02-14 bad check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000' check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500' -check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST +check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5 check_approxidate() { echo "$1 -> $2 +0000" >expect -- 1.6.6.2 ``` ## Junio C Hamano, 2010-07-07 05:28 Subject: Re: [PATCH 1/2] t0006: test timezone parsing Message-ID: <7vk4p7274g.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4p7274g.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100706075433.GD3567@sigill.intra.peff.net> ``` Jeff King writes: > On Tue, Jul 06, 2010 at 03:28:49AM -0400, Jeff King wrote: > >> > On FreeBSD 8.0, we now see this. >> > >> > cc1: warnings being treated as errors >> > test-date.c: In function 'parse_dates': >> > test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *' >> >> Meh. I was worried about that when I used sscanf. I think we can just do >> this: > > Oh, I see this is already on maint. :) So here is my fixup in a nicer > form: Thanks. ``` ## Alex Riesen, 2010-07-07 09:48 Subject: Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX Message-ID: URL: https://gitlist.dev/e/AANLkTin5czoPMgqg5SsiuURjZ-1tpg-S-Die4DejjAMs%40mail.gmail.com In-Reply-To: ``` On Wed, Jul 7, 2010 at 01:34, Brandon Casey wrote: > From: Brandon Casey > > POSIX requires that both the timezone "standard" and "offset" be specified > in the TZ environment variable.  This causes a problem on IRIX which does > not understand the timezone 'EST'. > > Signed-off-by: Brandon Casey > --- > > I guess 'EST' is an alias? for EST5EDT?  Linux and Solaris both grok EST > just fine.  POSIX says the offset is required.  I don't see any mention of > aliases. Same on the Cygwin installation I happen to have here: it doesn't recongnize EST, but does know about EST5. ``` ## Jeff King, 2010-07-07 18:12 Subject: Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX Message-ID: <20100707181221.GA5299@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100707181221.GA5299%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Jul 06, 2010 at 06:34:20PM -0500, Brandon Casey wrote: > From: Brandon Casey > > POSIX requires that both the timezone "standard" and "offset" be specified > in the TZ environment variable. This causes a problem on IRIX which does > not understand the timezone 'EST'. > > Signed-off-by: Brandon Casey > --- > > I guess 'EST' is an alias? for EST5EDT? Linux and Solaris both grok EST > just fine. POSIX says the offset is required. I don't see any mention of > aliases. Yeah, I believe it is the same as EST5EDT. Which time zone isn't really important. The main thing is that it be a negative offset from UTC, and that we be able to convert the zone name into a numeric offset for the date specified in the test (which is totally arbitrary and can be tweaked if need be to avoid daylight saving issues). So I think your patch is fine, but we can look harder if somebody finds a platform that doesn't understand EST5. -Peff ``` ## Andreas Schwab, 2010-07-07 22:59 Subject: Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX Message-ID: URL: https://gitlist.dev/e/m2tyoasxtc.fsf%40igel.home In-Reply-To: ``` Brandon Casey writes: > I guess 'EST' is an alias? for EST5EDT? Linux and Solaris both grok EST > just fine. POSIX says the offset is required. I don't see any mention of > aliases. It's a backward-compatible alias in the Olsen database. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ```