[PATCH 1/2] t0006: test timezone parsing

[PATCH 1/2] t0006: test timezone parsing

[Jeff King]

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 <peff@peff.net>
---
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

[PATCH 2/2] parse_date: fix signedness in timezone calculation

[Jeff King]

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 <peff@peff.net> 1275404416 +119304128

Instead, we should perform this subtraction as a time_t, the
same type that mktime returns.

Signed-off-by: Jeff King <peff@peff.net>
---
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

Re: [PATCH 2/2] parse_date: fix signedness in timezone calculation

[Junio C Hamano]

Thanks; both patches make sense to me.

[PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

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 <casey@nrlssc.navy.mil>
---


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

Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX

[Alex Riesen]

On Wed, Jul 7, 2010 at 01:34, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> From: Brandon Casey <drafnel@gmail.com>
>
> 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 <casey@nrlssc.navy.mil>
> ---
>
> 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.

Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX

[Jeff King]

On Tue, Jul 06, 2010 at 06:34:20PM -0500, Brandon Casey wrote:

> From: Brandon Casey <drafnel@gmail.com>
> 
> 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 <casey@nrlssc.navy.mil>
> ---
> 
> 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

Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX

[Andreas Schwab]

Brandon Casey <casey@nrlssc.navy.mil> 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."

Re: [PATCH 1/2] t0006: test timezone parsing

[Junio C Hamano]

Jeff King <peff@peff.net> 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 *'

Re: [PATCH 1/2] t0006: test timezone parsing

[Jeff King]

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

Re: [PATCH 1/2] t0006: test timezone parsing

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH 1/2] t0006: test timezone parsing

[Junio C Hamano]

Jeff King <peff@peff.net> 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.