{"thread":{"id":"24284","subject":"[PATCH 1/2] t0006: test timezone parsing","startedAt":"2010-07-04T10:48:35Z","lastAt":"2010-07-07T22:59:27Z","messageCount":11,"participants":["Jeff King","Junio C Hamano","Brandon Casey","Alex Riesen","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"144775","messageId":"20100704104834.GA23070@sigill.intra.peff.net","threadId":"24284","inReplyTo":null,"subject":"[PATCH 1/2] t0006: test timezone parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-07-04T10:48:35Z","receivedAt":"2010-07-04T10:48:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Previously, test-date simply ignored the parsed timezone and\ntold show_date() to use UTC. Instead, let's print out what\nwe actually parsed.\n\nWhile we're at it, let's make it easy for tests to work in a specific\ntimezone.\n\nSigned-off-by: Jeff King \n---\nThis is a good idea anyway, but it also makes it easy to write a test\nfor the next patch.\n\n t/t0006-date.sh | 5 +++--\n test-date.c | 9 ++++++---\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex 75b02af..3ea4f9e 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -28,8 +28,8 @@ check_show 31449600 '12 months ago'\n \n check_parse() {\n \techo \"$1 -> $2\" >expect\n-\ttest_expect_${3:-success} \"parse date ($1)\" \"\n-\ttest-date parse '$1' >actual &&\n+\ttest_expect_${4:-success} \"parse date ($1${3:+ TZ=$3})\" \"\n+\tTZ=${3:-$TZ} test-date parse '$1' >actual &&\n \ttest_cmp expect actual\n \t\"\n }\n@@ -38,6 +38,7 @@ check_parse 2008 bad\n check_parse 2008-02 bad\n check_parse 2008-02-14 bad\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'\n+check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'\n \n check_approxidate() {\n \techo \"$1 -> $2 +0000\" >expect\ndiff --git a/test-date.c b/test-date.c\nindex a9e705f..ac6854a 100644\n--- a/test-date.c\n+++ b/test-date.c\n@@ -21,12 +21,15 @@ static void parse_dates(char **argv, struct timeval *now)\n \tfor (; *argv; argv++) {\n \t\tchar result[100];\n \t\ttime_t t;\n+\t\tint tz;\n \n \t\tresult[0] = 0;\n \t\tparse_date(*argv, result, sizeof(result));\n-\t\tt = strtoul(result, NULL, 0);\n-\t\tprintf(\"%s -> %s\\n\", *argv,\n-\t\t\tt ? show_date(t, 0, DATE_ISO8601) : \"bad\");\n+\t\tif (sscanf(result, \"%ld %d\", &t, &tz) == 2)\n+\t\t\tprintf(\"%s -> %s\\n\",\n+\t\t\t *argv, show_date(t, tz, DATE_ISO8601));\n+\t\telse\n+\t\t\tprintf(\"%s -> bad\\n\", *argv);\n \t}\n }\n \n-- \n1.7.2.rc1.209.g2a36c\n"},{"id":"144776","messageId":"20100704110017.GA23121@sigill.intra.peff.net","threadId":"24284","inReplyTo":"20100704104834.GA23070@sigill.intra.peff.net","subject":"[PATCH 2/2] parse_date: fix signedness in timezone calculation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-07-04T11:00:17Z","receivedAt":"2010-07-04T11:00:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When no timezone is specified, we deduce the offset by\nsubtracting the result of mktime from our calculated\ntimestamp.\n\nHowever, our timestamp is stored as an unsigned integer,\nmeaning we perform the subtraction as unsigned. For a\nnegative offset, this means we wrap to a very high number,\nand our numeric timezone is in the millions of hours. You\ncan see this bug by doing:\n\n $ TZ=EST \\\n GIT_AUTHOR_DATE='2010-06-01 10:00' \\\n git commit -a -m foo\n $ git cat-file -p HEAD | grep author\n author Jeff King 1275404416 +119304128\n\nInstead, we should perform this subtraction as a time_t, the\nsame type that mktime returns.\n\nSigned-off-by: Jeff King \n---\nAre there any platforms with an unsigned time_t? In that case, we would\nneed to use a regular \"long\" to get signedness. Presumably on such\nplatforms \"long\" will grow with time_t to 64-bits so we don't create a\nY2038 problem (though most of the rest of the code uses \"unsigned long\"\ninterchangeably with time_t, so we have a Y2106 problem in that case\nanyway).\n\n date.c | 2 +-\n t/t0006-date.sh | 1 +\n 2 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 68cdcaa..3c981f7 100644\n--- a/date.c\n+++ b/date.c\n@@ -635,7 +635,7 @@ int parse_date_toffset(const char *date, unsigned long *timestamp, int *offset)\n \t/* mktime uses local timezone */\n \t*timestamp = tm_to_time_t(&tm);\n \tif (*offset == -1)\n-\t\t*offset = (*timestamp - mktime(&tm)) / 60;\n+\t\t*offset = ((time_t)*timestamp - mktime(&tm)) / 60;\n \n \tif (*timestamp == -1)\n \t\treturn -1;\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex 3ea4f9e..b2df4fe 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -39,6 +39,7 @@ check_parse 2008-02 bad\n check_parse 2008-02-14 bad\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'\n check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'\n+check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST\n \n check_approxidate() {\n \techo \"$1 -> $2 +0000\" >expect\n-- \n1.7.2.rc1.209.g2a36c\n"},{"id":"144861","messageId":"7vy6dp72wh.fsf@alter.siamese.dyndns.org","threadId":"24284","inReplyTo":"20100704110017.GA23121@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] parse_date: fix signedness in timezone calculation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-06T02:35:58Z","receivedAt":"2010-07-06T02:35:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; both patches make sense to me.\n"},{"id":"144883","messageId":"7vpqz13xgw.fsf@alter.siamese.dyndns.org","threadId":"24284","inReplyTo":"20100704104834.GA23070@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t0006: test timezone parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-06T07:01:35Z","receivedAt":"2010-07-06T07:01:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/test-date.c b/test-date.c\n> index a9e705f..ac6854a 100644\n> --- a/test-date.c\n> +++ b/test-date.c\n> @@ -21,12 +21,15 @@ static void parse_dates(char **argv, struct timeval *now)\n> \tfor (; *argv; argv++) {\n> \t\tchar result[100];\n> \t\ttime_t t;\n> +\t\tint tz;\n> \n> \t\tresult[0] = 0;\n> \t\tparse_date(*argv, result, sizeof(result));\n> -\t\tt = strtoul(result, NULL, 0);\n> -\t\tprintf(\"%s -> %s\\n\", *argv,\n> -\t\t\tt ? show_date(t, 0, DATE_ISO8601) : \"bad\");\n> +\t\tif (sscanf(result, \"%ld %d\", &t, &tz) == 2)\n\nGah...\n\nOn FreeBSD 8.0, we now see this.\n\n cc1: warnings being treated as errors\n test-date.c: In function 'parse_dates':\n test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *'\n"},{"id":"144884","messageId":"20100706072849.GA3567@sigill.intra.peff.net","threadId":"24284","inReplyTo":"7vpqz13xgw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t0006: test timezone parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-07-06T07:28:49Z","receivedAt":"2010-07-06T07:28:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 06, 2010 at 12:01:35AM -0700, Junio C Hamano wrote:\n\n> > \t\ttime_t t;\n> > +\t\tint tz;\n> [...]\n> > +\t\tif (sscanf(result, \"%ld %d\", &t, &tz) == 2)\n> \n> Gah...\n> \n> On FreeBSD 8.0, we now see this.\n> \n> cc1: warnings being treated as errors\n> test-date.c: In function 'parse_dates':\n> test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *'\n\nMeh. I was worried about that when I used sscanf. I think we can just do\nthis:\n\ndiff --git a/test-date.c b/test-date.c\nindex ac6854a..6bcd5b0 100644\n--- a/test-date.c\n+++ b/test-date.c\n@@ -20,12 +20,12 @@ static void parse_dates(char **argv, struct timeval *now)\n {\n \tfor (; *argv; argv++) {\n \t\tchar result[100];\n-\t\ttime_t t;\n+\t\tunsigned long t;\n \t\tint tz;\n \n \t\tresult[0] = 0;\n \t\tparse_date(*argv, result, sizeof(result));\n-\t\tif (sscanf(result, \"%ld %d\", &t, &tz) == 2)\n+\t\tif (sscanf(result, \"%lu %d\", &t, &tz) == 2)\n \t\t\tprintf(\"%s -> %s\\n\",\n \t\t\t *argv, show_date(t, tz, DATE_ISO8601));\n \t\telse\n\nas show_date takes an unsigned long anyway.\n\n-Peff\n"},{"id":"144886","messageId":"20100706075433.GD3567@sigill.intra.peff.net","threadId":"24284","inReplyTo":"20100706072849.GA3567@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t0006: test timezone parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-07-06T07:54:33Z","receivedAt":"2010-07-06T07:54:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 06, 2010 at 03:28:49AM -0400, Jeff King wrote:\n\n> > On FreeBSD 8.0, we now see this.\n> > \n> > cc1: warnings being treated as errors\n> > test-date.c: In function 'parse_dates':\n> > test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *'\n> \n> Meh. I was worried about that when I used sscanf. I think we can just do\n> this:\n\nOh, I see this is already on maint. :) So here is my fixup in a nicer\nform:\n\n-- >8 --\nSubject: [PATCH] test-date: fix sscanf type conversion\n\nReading into a time_t isn't portable, since we don't know\nthe exact type. Instead, use an unsigned long, which is what\nshow_date wants, anyway.\n\nSigned-off-by: Jeff King \n---\n test-date.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/test-date.c b/test-date.c\nindex ac6854a..6bcd5b0 100644\n--- a/test-date.c\n+++ b/test-date.c\n@@ -20,12 +20,12 @@ static void parse_dates(char **argv, struct timeval *now)\n {\n \tfor (; *argv; argv++) {\n \t\tchar result[100];\n-\t\ttime_t t;\n+\t\tunsigned long t;\n \t\tint tz;\n \n \t\tresult[0] = 0;\n \t\tparse_date(*argv, result, sizeof(result));\n-\t\tif (sscanf(result, \"%ld %d\", &t, &tz) == 2)\n+\t\tif (sscanf(result, \"%lu %d\", &t, &tz) == 2)\n \t\t\tprintf(\"%s -> %s\\n\",\n \t\t\t *argv, show_date(t, tz, DATE_ISO8601));\n \t\telse\n-- \n1.7.2.rc1.214.g5a9d0\n"},{"id":"144970","messageId":"upBu73z-yHJSTQ58c-Br0BxGwDzKGa1fPzZXA71AfI3kG3dMsRRPOX76JRJVhEOcDGoNaf1z9Lg@cipher.nrlssc.navy.mil","threadId":"24284","inReplyTo":"20100704110017.GA23121@sigill.intra.peff.net","subject":"[PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2010-07-06T23:34:20Z","receivedAt":"2010-07-06T23:34:20Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey \n\nPOSIX requires that both the timezone \"standard\" and \"offset\" be specified\nin the TZ environment variable. This causes a problem on IRIX which does\nnot understand the timezone 'EST'.\n\nSigned-off-by: Brandon Casey \n---\n\n\nI guess 'EST' is an alias? for EST5EDT? Linux and Solaris both grok EST\njust fine. POSIX says the offset is required. I don't see any mention of\naliases.\n\n-brandon\n\n\n t/t0006-date.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex b2df4fe..1d4d0a5 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -39,7 +39,7 @@ check_parse 2008-02 bad\n check_parse 2008-02-14 bad\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'\n check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'\n-check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST\n+check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n \n check_approxidate() {\n \techo \"$1 -> $2 +0000\" >expect\n-- \n1.6.6.2\n"},{"id":"144998","messageId":"7vk4p7274g.fsf@alter.siamese.dyndns.org","threadId":"24284","inReplyTo":"20100706075433.GD3567@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t0006: test timezone parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-07T05:28:15Z","receivedAt":"2010-07-07T05:28:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 06, 2010 at 03:28:49AM -0400, Jeff King wrote:\n>\n>> > On FreeBSD 8.0, we now see this.\n>> > \n>> > cc1: warnings being treated as errors\n>> > test-date.c: In function 'parse_dates':\n>> > test-date.c:28: warning: format '%ld' expects type 'long int *', but argument 3 has type 'time_t *'\n>> \n>> Meh. I was worried about that when I used sscanf. I think we can just do\n>> this:\n>\n> Oh, I see this is already on maint. :) So here is my fixup in a nicer\n> form:\n\nThanks.\n"},{"id":"145013","messageId":"AANLkTin5czoPMgqg5SsiuURjZ-1tpg-S-Die4DejjAMs@mail.gmail.com","threadId":"24284","inReplyTo":"upBu73z-yHJSTQ58c-Br0BxGwDzKGa1fPzZXA71AfI3kG3dMsRRPOX76JRJVhEOcDGoNaf1z9Lg@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-07-07T09:48:41Z","receivedAt":"2010-07-07T09:48:41Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Jul 7, 2010 at 01:34, Brandon Casey wrote:\n> From: Brandon Casey \n>\n> POSIX requires that both the timezone \"standard\" and \"offset\" be specified\n> in the TZ environment variable.  This causes a problem on IRIX which does\n> not understand the timezone 'EST'.\n>\n> Signed-off-by: Brandon Casey \n> ---\n>\n> I guess 'EST' is an alias? for EST5EDT?  Linux and Solaris both grok EST\n> just fine.  POSIX says the offset is required.  I don't see any mention of\n> aliases.\n\nSame on the Cygwin installation I happen to have here: it doesn't\nrecongnize EST,\nbut does know about EST5.\n"},{"id":"145041","messageId":"20100707181221.GA5299@coredump.intra.peff.net","threadId":"24284","inReplyTo":"upBu73z-yHJSTQ58c-Br0BxGwDzKGa1fPzZXA71AfI3kG3dMsRRPOX76JRJVhEOcDGoNaf1z9Lg@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-07-07T18:12:21Z","receivedAt":"2010-07-07T18:12:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 06, 2010 at 06:34:20PM -0500, Brandon Casey wrote:\n\n> From: Brandon Casey \n> \n> POSIX requires that both the timezone \"standard\" and \"offset\" be specified\n> in the TZ environment variable. This causes a problem on IRIX which does\n> not understand the timezone 'EST'.\n> \n> Signed-off-by: Brandon Casey \n> ---\n> \n> I guess 'EST' is an alias? for EST5EDT? Linux and Solaris both grok EST\n> just fine. POSIX says the offset is required. I don't see any mention of\n> aliases.\n\nYeah, I believe it is the same as EST5EDT. Which time zone isn't really\nimportant. The main thing is that it be a negative offset from UTC, and\nthat we be able to convert the zone name into a numeric offset for the\ndate specified in the test (which is totally arbitrary and can be\ntweaked if need be to avoid daylight saving issues).\n\nSo I think your patch is fine, but we can look harder if somebody finds\na platform that doesn't understand EST5.\n\n-Peff\n"},{"id":"145065","messageId":"m2tyoasxtc.fsf@igel.home","threadId":"24284","inReplyTo":"upBu73z-yHJSTQ58c-Br0BxGwDzKGa1fPzZXA71AfI3kG3dMsRRPOX76JRJVhEOcDGoNaf1z9Lg@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] t/t0006: specify timezone as EST5 not EST to comply with POSIX","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2010-07-07T22:59:27Z","receivedAt":"2010-07-07T22:59:27Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Brandon Casey writes:\n\n> I guess 'EST' is an alias? for EST5EDT? Linux and Solaris both grok EST\n> just fine. POSIX says the offset is required. I don't see any mention of\n> aliases.\n\nIt's a backward-compatible alias in the Olsen database.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"}]}