{"thread":{"id":"28314","subject":"[PATCH] date.c: Parse timezone with colon as separator","startedAt":"2011-09-06T14:56:36Z","lastAt":"2011-09-10T08:29:06Z","messageCount":12,"participants":["Haitao Li","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174930","messageId":"1315320996-1997-1-git-send-email-lihaitao@gmail.com","threadId":"28314","inReplyTo":null,"subject":"[PATCH] date.c: Parse timezone with colon as separator","fromName":"Haitao Li","fromEmail":"lihaitao@gmail.com","sentAt":"2011-09-06T14:56:36Z","receivedAt":"2011-09-06T14:56:36Z","isPatch":true,"sender":{"key":"lihaitao@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19508?v=4"},"body":"Timezone designators including additional separator (`:`) are ignored.\nActually zone designators in below formats are all valid according to\nISO8601:2004, section 4.3.2:\n    [+-]hh, [+-]hhmm, [+-]hh:mm\n\nSteps to reproduce the issue this patch fixes:\n    $ mkdir /tmp/test\n    $ cd /tmp/test\n    $ git init\n    $ echo 'timezone' > file.txt\n    $ git add .\n    $ git update-index\n    $ git write-tree\n    3e168d57e1c32a4598af134430384f0587581503\n\n    # Commit the tree returned above. Give a date with colon separated\n    # timezone\n    $ echo \"Test commit\" | \\\n      TZ=UTC GIT_AUTHOR_DATE='2011-09-03T12:34:56+08:00' \\\n      git commit-tree 3e168d57e1c32a4598af134430384f0587581503 | \\\n      xargs git show  | grep Date\n    Date:   Sat Sep 3 12:34:56 2011 +0000\n\nwhile the expected result is:\n    Date:   Sat Sep 3 12:34:56 2011 +0800\n                                      ^---\n\nThis patch teaches git recognizing zone designators with hours and\nminutes separated by colon, or minutes are empty.\n\nSigned-off-by: Haitao Li <lihaitao@gmail.com>\n---\n date.c |   14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 896fbb4..8a94944 100644\n--- a/date.c\n+++ b/date.c\n@@ -556,15 +556,21 @@ static int match_tz(const char *date, int *offp)\n \tint min, hour;\n \tint n = end - date - 1;\n \n-\tmin = offset % 100;\n-\thour = offset / 100;\n+\tif (n == 2 && *end == ':') {\n+\t\thour = offset;\n+\t\toffset = strtoul(date+4, &end, 10);\n+\t\tmin = offset % 100;\n+\t} else {\n+\t\thour = offset / 100;\n+\t\tmin = offset % 100;\n+\t}\n \n \t/*\n-\t * Don't accept any random crap.. At least 3 digits, and\n+\t * Don't accept any random crap.. At least 2 digits, and\n \t * a valid minute. We might want to check that the minutes\n \t * are divisible by 30 or something too.\n \t */\n-\tif (min < 60 && n > 2) {\n+\tif (min < 60 && n > 1) {\n \t\toffset = hour*60+min;\n \t\tif (*date == '-')\n \t\t\toffset = -offset;\n-- \n1.7.5.4\n"},{"id":"174956","messageId":"20110906202440.GD14554@sigill.intra.peff.net","threadId":"28314","inReplyTo":"1315320996-1997-1-git-send-email-lihaitao@gmail.com","subject":"Re: [PATCH] date.c: Parse timezone with colon as separator","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-06T20:24:40Z","receivedAt":"2011-09-06T20:24:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2011 at 10:56:36PM +0800, Haitao Li wrote:\n\n> Timezone designators including additional separator (`:`) are ignored.\n> Actually zone designators in below formats are all valid according to\n> ISO8601:2004, section 4.3.2:\n>     [+-]hh, [+-]hhmm, [+-]hh:mm\n\nThat seems like a sensible list to support, given that it is part of\niso8601 (though I was a little surprised after reading your subject\nline, which would probably be better as \"support iso8601 timezone\nformats\").\n\n> ---\n>  date.c |   14 ++++++++++----\n>  1 files changed, 10 insertions(+), 4 deletions(-)\n\nWe should probably have new tests, too. I was going to suggest squashing\nin the ones below, but your patch doesn't seem to work with the first\none:\n\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex f87abb5..9b326cd 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -40,6 +40,8 @@ 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 -05' '2008-02-14 20:30:45 -0500'\n+check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n \n check_approxidate() {\n\nIf I am reading your commit message correctly, that should work, right?\n\n-Peff\n"},{"id":"174974","messageId":"CABk5KLh230zLfkdaEgv7SDC6ivKqwvaGJQFVEKkKtGjMyEmLog@mail.gmail.com","threadId":"28314","inReplyTo":"20110906202440.GD14554@sigill.intra.peff.net","subject":"Re: [PATCH] date.c: Parse timezone with colon as separator","fromName":"Haitao Li","fromEmail":"lihaitao@gmail.com","sentAt":"2011-09-07T03:00:59Z","receivedAt":"2011-09-07T03:00:59Z","isPatch":true,"sender":{"key":"lihaitao@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19508?v=4"},"body":"> That seems like a sensible list to support, given that it is part of\n> iso8601 (though I was a little surprised after reading your subject\n> line, which would probably be better as \"support iso8601 timezone\n> formats\").\n\nI agree this is a better one! ;)\n\n> We should probably have new tests, too. I was going to suggest squashing\n> in the ones below, but your patch doesn't seem to work with the first\n> one:\n>\n> diff --git a/t/t0006-date.sh b/t/t0006-date.sh\n> index f87abb5..9b326cd 100755\n> --- a/t/t0006-date.sh\n> +++ b/t/t0006-date.sh\n> @@ -40,6 +40,8 @@ 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 -05' '2008-02-14 20:30:45 -0500'\n> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n>  check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n>\n>  check_approxidate() {\n>\n> If I am reading your commit message correctly, that should work, right?\n>\n\nYes, you are right. I will send an updated version to get this right.\n\nThank you for reviewing and giving suggestions!\n\n/Haitao\n"},{"id":"174978","messageId":"1315374407-30828-1-git-send-email-lihaitao@gmail.com","threadId":"28314","inReplyTo":"1315320996-1997-1-git-send-email-lihaitao@gmail.com","subject":"[PATCH v2] date.c: Support iso8601 timezone formats","fromName":"Haitao Li","fromEmail":"lihaitao@gmail.com","sentAt":"2011-09-07T05:46:47Z","receivedAt":"2011-09-07T05:46:47Z","isPatch":true,"sender":{"key":"lihaitao@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19508?v=4"},"body":"Timezone designators including additional separator (`:`) are ignored.\nActually zone designators in below formats are all valid according to\nISO8601:2004, section 4.3.2:\n    [+-]hh, [+-]hhmm, [+-]hh:mm\n\nSteps to reproduce the issue this patch fixes:\n    $ mkdir /tmp/test\n    $ cd /tmp/test\n    $ git init\n    $ echo 'timezone' > file.txt\n    $ git add .\n    $ git update-index\n    $ git write-tree\n    3e168d57e1c32a4598af134430384f0587581503\n\n    # Commit the tree returned above. Give a date with colon separated\n    # timezone\n    $ echo \"Test commit\" | \\\n      TZ=UTC GIT_AUTHOR_DATE='2011-09-03T12:34:56+08:00' \\\n      git commit-tree 3e168d57e1c32a4598af134430384f0587581503 | \\\n      xargs git show  | grep Date\n    Date:   Sat Sep 3 12:34:56 2011 +0000\n\nwhile the expected result is:\n    Date:   Sat Sep 3 12:34:56 2011 +0800\n                                      ^---\n\nThis patch teaches git recognizing zone designators with hours and\nminutes separated by colon, or minutes are empty.\n\nSigned-off-by: Haitao Li <lihaitao@gmail.com>\n---\n date.c          |   32 ++++++++++++++++++++++++++------\n t/t0006-date.sh |    5 +++++\n 2 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 896fbb4..f970ea8 100644\n--- a/date.c\n+++ b/date.c\n@@ -556,15 +556,35 @@ static int match_tz(const char *date, int *offp)\n \tint min, hour;\n \tint n = end - date - 1;\n \n-\tmin = offset % 100;\n-\thour = offset / 100;\n+\t/*\n+\t * ISO8601:2004(E) allows time zone designator been separated\n+\t * by a clone in the extended format\n+\t */\n+\tif (*end == ':') {\n+\t\tif (isdigit(end[1])) {\n+\t\t\thour = offset;\n+\t\t\tmin = strtoul(end+1, &end, 10);\n+\t\t} else {\n+\t\t\t/* Mark as invalid */\n+\t\t\tn = -1;\n+\t\t}\n+\t} else {\n+\t\t/* Only hours specified */\n+\t\tif (n == 1 || n == 2) {\n+\t\t\thour = offset;\n+\t\t\tmin = 0;\n+\t\t} else {\n+\t\t\thour = offset / 100;\n+\t\t\tmin = offset % 100;\n+\t\t}\n+\t}\n \n \t/*\n-\t * Don't accept any random crap.. At least 3 digits, and\n-\t * a valid minute. We might want to check that the minutes\n-\t * are divisible by 30 or something too.\n+\t * Don't accept any random crap.. We might want to check that\n+\t * the minutes are divisible by 15 or something too. (Offset of\n+\t * Kathmandu, Nepal is UTC+5:45)\n \t */\n-\tif (min < 60 && n > 2) {\n+\tif (n > 0 && min < 60 && hour < 25) {\n \t\toffset = hour*60+min;\n \t\tif (*date == '-')\n \t\t\toffset = -offset;\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex f87abb5..5235b7a 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'\n+check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'\n+check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'\n+check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n+check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n \n check_approxidate() {\n-- \n1.7.5.4\n"},{"id":"174997","messageId":"7vaaag4794.fsf@alter.siamese.dyndns.org","threadId":"28314","inReplyTo":"1315374407-30828-1-git-send-email-lihaitao@gmail.com","subject":"Re: [PATCH v2] date.c: Support iso8601 timezone formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-07T17:00:55Z","receivedAt":"2011-09-07T17:00:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Haitao Li <lihaitao@gmail.com> writes:\n\n> diff --git a/date.c b/date.c\n> index 896fbb4..f970ea8 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -556,15 +556,35 @@ static int match_tz(const char *date, int *offp)\n>  \tint min, hour;\n>  \tint n = end - date - 1;\n>  \n> -\tmin = offset % 100;\n> -\thour = offset / 100;\n> +\t/*\n> +\t * ISO8601:2004(E) allows time zone designator been separated\n> +\t * by a clone in the extended format\n> +\t */\n> +\tif (*end == ':') {\n> +\t\tif (isdigit(end[1])) {\n> +\t\t\thour = offset;\n> +\t\t\tmin = strtoul(end+1, &end, 10);\n> +\t\t} else {\n> +\t\t\t/* Mark as invalid */\n> +\t\t\tn = -1;\n> +\t\t}\n> +\t} else {\n> +\t\t/* Only hours specified */\n\nThat comment belongs to inside the following if() {...}.\n\n> +\t\tif (n == 1 || n == 2) {\n\n... i.e. here.\n\n> +\t\t\thour = offset;\n> +\t\t\tmin = 0;\n> +\t\t} else {\n> +\t\t\thour = offset / 100;\n> +\t\t\tmin = offset % 100;\n> +\t\t}\n> +\t}\n>  \n>  \t/*\n> +\t * Don't accept any random crap.. We might want to check that\n> +\t * the minutes are divisible by 15 or something too. (Offset of\n> +\t * Kathmandu, Nepal is UTC+5:45)\n>  \t */\n> -\tif (min < 60 && n > 2) {\n> +\tif (n > 0 && min < 60 && hour < 25) {\n\nWhat is this \"hour < 25\" about? Aren't we talking about the UTC offset\nvalue that come after the [-+] sign?\n\nI do not mind adding a new check, but I do mind if it adds a check with\nnot much value.  Even at Pacific/Kiritimati, the offset is 14; the new\ncheck seems a bit too lenient.\n\n>  \t\toffset = hour*60+min;\n>  \t\tif (*date == '-')\n>  \t\t\toffset = -offset;\n> diff --git a/t/t0006-date.sh b/t/t0006-date.sh\n> index f87abb5..5235b7a 100755\n> --- a/t/t0006-date.sh\n> +++ b/t/t0006-date.sh\n> @@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'\n> +check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'\n> +check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'\n> +check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n>  check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n>  \n>  check_approxidate() {\n"},{"id":"175059","messageId":"CABk5KLhXyQOFLegdpNcHzfwxX4LwjcG_eE3TVW1eDgTP=4kYGQ@mail.gmail.com","threadId":"28314","inReplyTo":"7vaaag4794.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] date.c: Support iso8601 timezone formats","fromName":"Haitao Li","fromEmail":"lihaitao@gmail.com","sentAt":"2011-09-08T02:53:59Z","receivedAt":"2011-09-08T02:53:59Z","isPatch":true,"sender":{"key":"lihaitao@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19508?v=4"},"body":">> +     } else {\n>> +             /* Only hours specified */\n>\n> That comment belongs to inside the following if() {...}.\n>\n>> +             if (n == 1 || n == 2) {\n>\n> ... i.e. here.\n>\nGood catch!\n\n>> -     if (min < 60 && n > 2) {\n>> +     if (n > 0 && min < 60 && hour < 25) {\n>\n> What is this \"hour < 25\" about? Aren't we talking about the UTC offset\n> value that come after the [-+] sign?\n>\n> I do not mind adding a new check, but I do mind if it adds a check with\n> not much value.  Even at Pacific/Kiritimati, the offset is 14; the new\n> check seems a bit too lenient.\n>\n\nI think it's not \"too\" lenient. UTC+14 was \"invented\" in 1995 [1].\nMaybe UTC+15 would be added someday for some reason? How about\nchanging to \"hour < 24\", this is how ICU checks offset validity [2].\n\n1. http://en.wikipedia.org/wiki/UTC%2B14#History\n2. http://bugs.icu-project.org/trac/browser/icu/tags/release-4-8-1/source/i18n/timezone.cpp#L1482\n"},{"id":"175164","messageId":"1315563033-9476-1-git-send-email-lihaitao@gmail.com","threadId":"28314","inReplyTo":"1315320996-1997-1-git-send-email-lihaitao@gmail.com","subject":"[PATCH v3] date.c: Support iso8601 timezone formats","fromName":"Haitao Li","fromEmail":"lihaitao@gmail.com","sentAt":"2011-09-09T10:10:33Z","receivedAt":"2011-09-09T10:10:33Z","isPatch":true,"sender":{"key":"lihaitao@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19508?v=4"},"body":"Timezone designators including additional separator (`:`) are ignored.\nActually zone designators in below formats are all valid according to\nISO8601:2004, section 4.3.2:\n    [+-]hh, [+-]hhmm, [+-]hh:mm\n\nSteps to reproduce the issue this patch fixes:\n    $ mkdir /tmp/test\n    $ cd /tmp/test\n    $ git init\n    $ echo 'timezone' > file.txt\n    $ git add .\n    $ git update-index\n    $ git write-tree\n    3e168d57e1c32a4598af134430384f0587581503\n\n    # Commit the tree returned above. Give a date with colon separated\n    # timezone\n    $ echo \"Test commit\" | \\\n      TZ=UTC GIT_AUTHOR_DATE='2011-09-03T12:34:56+08:00' \\\n      git commit-tree 3e168d57e1c32a4598af134430384f0587581503 | \\\n      xargs git show  | grep Date\n    Date:   Sat Sep 3 12:34:56 2011 +0000\n\nwhile the expected result is:\n    Date:   Sat Sep 3 12:34:56 2011 +0800\n                                      ^---\n\nThis patch teaches git recognizing zone designators with hours and\nminutes separated by colon, or minutes are empty.\n\nSigned-off-by: Haitao Li <lihaitao@gmail.com>\n---\n date.c          |   32 ++++++++++++++++++++++++++------\n t/t0006-date.sh |    5 +++++\n 2 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 896fbb4..f8722c1 100644\n--- a/date.c\n+++ b/date.c\n@@ -556,15 +556,35 @@ static int match_tz(const char *date, int *offp)\n \tint min, hour;\n \tint n = end - date - 1;\n \n-\tmin = offset % 100;\n-\thour = offset / 100;\n+\t/*\n+\t * ISO8601:2004(E) allows time zone designator been separated\n+\t * by a clone in the extended format\n+\t */\n+\tif (*end == ':') {\n+\t\tif (isdigit(end[1])) {\n+\t\t\thour = offset;\n+\t\t\tmin = strtoul(end+1, &end, 10);\n+\t\t} else {\n+\t\t\t/* Mark as invalid */\n+\t\t\tn = -1;\n+\t\t}\n+\t} else {\n+\t\tif (n == 1 || n == 2) {\n+\t\t\t/* Only hours specified */\n+\t\t\thour = offset;\n+\t\t\tmin = 0;\n+\t\t} else {\n+\t\t\thour = offset / 100;\n+\t\t\tmin = offset % 100;\n+\t\t}\n+\t}\n \n \t/*\n-\t * Don't accept any random crap.. At least 3 digits, and\n-\t * a valid minute. We might want to check that the minutes\n-\t * are divisible by 30 or something too.\n+\t * Don't accept any random crap.. We might want to check that\n+\t * the minutes are divisible by 15 or something too. (Offset of\n+\t * Kathmandu, Nepal is UTC+5:45)\n \t */\n-\tif (min < 60 && n > 2) {\n+\tif (n > 0 && min < 60) {\n \t\toffset = hour*60+min;\n \t\tif (*date == '-')\n \t\t\toffset = -offset;\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex f87abb5..5235b7a 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'\n+check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'\n+check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'\n+check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n+check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n \n check_approxidate() {\n-- \n1.7.5.4\n"},{"id":"175214","messageId":"7vhb4lvflb.fsf@alter.siamese.dyndns.org","threadId":"28314","inReplyTo":"1315563033-9476-1-git-send-email-lihaitao@gmail.com","subject":"Re: [PATCH v3] date.c: Support iso8601 timezone formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-09T16:35:28Z","receivedAt":"2011-09-09T16:35:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Haitao Li <lihaitao@gmail.com> writes:\n\n> Timezone designators including additional separator (`:`) are ignored.\n> Actually zone designators in below formats are all valid according to\n> ISO8601:2004, section 4.3.2:\n>     [+-]hh, [+-]hhmm, [+-]hh:mm\n\nThanks for a re-roll.\n\n> This patch teaches git recognizing zone designators with hours and\n> minutes separated by colon, or minutes are empty.\n\nThe last sentence above makes it sound as if you are accepting\n\n\t\"2011-09-17 12:34:56 +09:\"\n\nbut I suspect that is not what you intend to allow.  Perhaps \"we allowed\nhh and hhmm and this teaches Git to recognize hh:mm format as well\"?\n\n> diff --git a/t/t0006-date.sh b/t/t0006-date.sh\n> index f87abb5..5235b7a 100755\n> --- a/t/t0006-date.sh\n> +++ b/t/t0006-date.sh\n> @@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'\n> +check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'\n> +check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'\n> +check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n>  check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n\nThe above are from Peff, no?  We should credit him for tests in the\nproposed log message.\n\nBecause the three formats 8601 specifies are \"hh\", \"hhmm\", or \"hh:mm\"\nafter +/-, among the above new tests, it appears to me that zone\ndesignators \"-5\" and \"-:30\" should yield \"bad\", instead of being accepted.\nThe same for \"+09:\" I mentioned above, which is not in the new test.\n"},{"id":"175215","messageId":"7vd3f9ve9m.fsf@alter.siamese.dyndns.org","threadId":"28314","inReplyTo":"7vhb4lvflb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] date.c: Support iso8601 timezone formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-09T17:04:05Z","receivedAt":"2011-09-09T17:04:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Haitao Li <lihaitao@gmail.com> writes:\n>\n>> Timezone designators including additional separator (`:`) are ignored.\n>> Actually zone designators in below formats are all valid according to\n>> ISO8601:2004, section 4.3.2:\n>>     [+-]hh, [+-]hhmm, [+-]hh:mm\n>\n> Thanks for a re-roll.\n>\n>> This patch teaches git recognizing zone designators with hours and\n>> minutes separated by colon, or minutes are empty.\n>\n> The last sentence above makes it sound as if you are accepting\n>\n> \t\"2011-09-17 12:34:56 +09:\"\n>\n> but I suspect that is not what you intend to allow.  Perhaps \"we allowed\n> hh and hhmm and this teaches Git to recognize hh:mm format as well\"?\n\nAlso, I do not quite understand why the match_tz() logic needs to be that\nlong.\n\nWouldn't something like this patch (on top of yours) easier to follow?\n\n date.c |   50 +++++++++++++++++++++-----------------------------\n 1 files changed, 21 insertions(+), 29 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex f8722c1..6079b1a 100644\n--- a/date.c\n+++ b/date.c\n@@ -551,44 +551,36 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt\n \n static int match_tz(const char *date, int *offp)\n {\n+\tint min;\n \tchar *end;\n-\tint offset = strtoul(date+1, &end, 10);\n-\tint min, hour;\n-\tint n = end - date - 1;\n+\tint hour = strtoul(date + 1, &end, 10);\n+\tint n = end - (date + 1);\n \n-\t/*\n-\t * ISO8601:2004(E) allows time zone designator been separated\n-\t * by a clone in the extended format\n-\t */\n-\tif (*end == ':') {\n-\t\tif (isdigit(end[1])) {\n-\t\t\thour = offset;\n-\t\t\tmin = strtoul(end+1, &end, 10);\n-\t\t} else {\n-\t\t\t/* Mark as invalid */\n-\t\t\tn = -1;\n-\t\t}\n-\t} else {\n-\t\tif (n == 1 || n == 2) {\n-\t\t\t/* Only hours specified */\n-\t\t\thour = offset;\n-\t\t\tmin = 0;\n-\t\t} else {\n-\t\t\thour = offset / 100;\n-\t\t\tmin = offset % 100;\n-\t\t}\n+\tif (n == 4) {\n+\t\t/* hhmm */\n+\t\tmin = hour % 100;\n+\t\thour = hour / 100;\n+\t} else if (n != 2) {\n+\t\tmin = 99; /* random crap */\n+\t} else if (*end == ':') {\n+\t\t/* hh:mm? */\n+\t\tmin = strtoul(end + 1, &end, 10);\n+\t\tif (end - (date + 1) != 5)\n+\t\t\tmin = 99; /* random crap */\n \t}\n \n \t/*\n-\t * Don't accept any random crap.. We might want to check that\n-\t * the minutes are divisible by 15 or something too. (Offset of\n+\t * Don't accept any random crap. Even though some places have\n+\t * offset larger than 12 hours (e.g. Pacific/Kiritimati is at\n+\t * UTC+14), there is something wrong if hour part is much\n+\t * larger than that. We might also want to check that the\n+\t * minutes are divisible by 15 or something too. (Offset of\n \t * Kathmandu, Nepal is UTC+5:45)\n \t */\n-\tif (n > 0 && min < 60) {\n-\t\toffset = hour*60+min;\n+\tif (min < 60 && hour < 24) {\n+\t\tint offset = hour * 60 + min;\n \t\tif (*date == '-')\n \t\t\toffset = -offset;\n-\n \t\t*offp = offset;\n \t}\n \treturn end - date;\n"},{"id":"175244","messageId":"7vipp1tpdy.fsf@alter.siamese.dyndns.org","threadId":"28314","inReplyTo":"7vd3f9ve9m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] date.c: Support iso8601 timezone formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-09T20:46:49Z","receivedAt":"2011-09-09T20:46:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Also, I do not quite understand why the match_tz() logic needs to be that\n> long.\n>\n> Wouldn't something like this patch (on top of yours) easier to follow?\n>\n>  date.c |   50 +++++++++++++++++++++-----------------------------\n>  1 files changed, 21 insertions(+), 29 deletions(-)\n>\n> diff --git a/date.c b/date.c\n> index f8722c1..6079b1a 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -551,44 +551,36 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt\n>  \n>  static int match_tz(const char *date, int *offp)\n>  {\n> +\tint min;\n\nMicronit; this should be \"int min = 0\", as parsing of \"hh\" form relies on\nthe if/elseif/else cascade to parse only the hour part and leaving min to\nthe original value.\n\n>  \tchar *end;\n> +\tint hour = strtoul(date + 1, &end, 10);\n> +\tint n = end - (date + 1);\n>  \n> +\tif (n == 4) {\n> +\t\t/* hhmm */\n> +\t\tmin = hour % 100;\n> +\t\thour = hour / 100;\n> +\t} else if (n != 2) {\n> +\t\tmin = 99; /* random crap */\n> +\t} else if (*end == ':') {\n> +\t\t/* hh:mm? */\n> +\t\tmin = strtoul(end + 1, &end, 10);\n> +\t\tif (end - (date + 1) != 5)\n> +\t\t\tmin = 99; /* random crap */\n>  \t}\n"},{"id":"175270","messageId":"CABk5KLgyk_MRd6DLkdaK2s_5pxLQNipKmLJtCjNO2TaLLEr6UA@mail.gmail.com","threadId":"28314","inReplyTo":"7vhb4lvflb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] date.c: Support iso8601 timezone formats","fromName":"Haitao Li","fromEmail":"lihaitao@gmail.com","sentAt":"2011-09-10T08:06:50Z","receivedAt":"2011-09-10T08:06:50Z","isPatch":true,"sender":{"key":"lihaitao@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19508?v=4"},"body":"On Sat, Sep 10, 2011 at 12:35 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Haitao Li <lihaitao@gmail.com> writes:\n>\n>> Timezone designators including additional separator (`:`) are ignored.\n>> Actually zone designators in below formats are all valid according to\n>> ISO8601:2004, section 4.3.2:\n>>     [+-]hh, [+-]hhmm, [+-]hh:mm\n>\n> Thanks for a re-roll.\n>\n>> This patch teaches git recognizing zone designators with hours and\n>> minutes separated by colon, or minutes are empty.\n>\n> The last sentence above makes it sound as if you are accepting\n>\n>        \"2011-09-17 12:34:56 +09:\"\n>\n> but I suspect that is not what you intend to allow.  Perhaps \"we allowed\n> hh and hhmm and this teaches Git to recognize hh:mm format as well\"?\n>\nYes, this is a better one. Sorry for my English, and thanks for the suggestion.\n\n>> diff --git a/t/t0006-date.sh b/t/t0006-date.sh\n>> index f87abb5..5235b7a 100755\n>> --- a/t/t0006-date.sh\n>> +++ b/t/t0006-date.sh\n>> @@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'\n>> +check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'\n>> +check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'\n>> +check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'\n>> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'\n>>  check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5\n>\n> The above are from Peff, no?  We should credit him for tests in the\n> proposed log message.\n\nYes, we should credit Peff. Sorry for not knowing log message is used for this.\n\n>\n> Because the three formats 8601 specifies are \"hh\", \"hhmm\", or \"hh:mm\"\n> after +/-, among the above new tests, it appears to me that zone\n> designators \"-5\" and \"-:30\" should yield \"bad\", instead of being accepted.\n\nYes, the spec clearly states 2 digits are mandatory. \"-5\" should be\nregarded as invalid here.\n\nThe above test *ignores* \":30\" by setting offset to \"+0000\", this is\nto conform to how it works previously, less than 3 digits in offset\nare ignored. I agree it's better to *reject* them instead.\n\n> The same for \"+09:\" I mentioned above, which is not in the new test.\n>\nWill add.\n\nThanks again for the suggestions!\n"},{"id":"175271","messageId":"CABk5KLi-Q+qWokZFJGF+uDvkxGJVvFO4y38H8B1sO6SnHOWYpA@mail.gmail.com","threadId":"28314","inReplyTo":"7vd3f9ve9m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] date.c: Support iso8601 timezone formats","fromName":"Haitao Li","fromEmail":"lihaitao@gmail.com","sentAt":"2011-09-10T08:29:06Z","receivedAt":"2011-09-10T08:29:06Z","isPatch":true,"sender":{"key":"lihaitao@gmail.com","avatar":"https://avatars.githubusercontent.com/u/19508?v=4"},"body":">\n> Also, I do not quite understand why the match_tz() logic needs to be that\n> long.\n>\n> Wouldn't something like this patch (on top of yours) easier to follow?\n\nI was wrong about accepting one digit in hours or minutes. And yes\nyour version is conciser and easier to follow. Thanks!\n\n>\n>  date.c |   50 +++++++++++++++++++++-----------------------------\n>  1 files changed, 21 insertions(+), 29 deletions(-)\n>\n> diff --git a/date.c b/date.c\n> index f8722c1..6079b1a 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -551,44 +551,36 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt\n>\n>  static int match_tz(const char *date, int *offp)\n>  {\n> +       int min;\n>        char *end;\n> -       int offset = strtoul(date+1, &end, 10);\n> -       int min, hour;\n> -       int n = end - date - 1;\n> +       int hour = strtoul(date + 1, &end, 10);\n> +       int n = end - (date + 1);\n>\n> -       /*\n> -        * ISO8601:2004(E) allows time zone designator been separated\n> -        * by a clone in the extended format\n> -        */\n> -       if (*end == ':') {\n> -               if (isdigit(end[1])) {\n> -                       hour = offset;\n> -                       min = strtoul(end+1, &end, 10);\n> -               } else {\n> -                       /* Mark as invalid */\n> -                       n = -1;\n> -               }\n> -       } else {\n> -               if (n == 1 || n == 2) {\n> -                       /* Only hours specified */\n> -                       hour = offset;\n> -                       min = 0;\n> -               } else {\n> -                       hour = offset / 100;\n> -                       min = offset % 100;\n> -               }\n> +       if (n == 4) {\n> +               /* hhmm */\n> +               min = hour % 100;\n> +               hour = hour / 100;\n> +       } else if (n != 2) {\n> +               min = 99; /* random crap */\n> +       } else if (*end == ':') {\n> +               /* hh:mm? */\n> +               min = strtoul(end + 1, &end, 10);\n> +               if (end - (date + 1) != 5)\n> +                       min = 99; /* random crap */\n>        }\n>\n>        /*\n> -        * Don't accept any random crap.. We might want to check that\n> -        * the minutes are divisible by 15 or something too. (Offset of\n> +        * Don't accept any random crap. Even though some places have\n> +        * offset larger than 12 hours (e.g. Pacific/Kiritimati is at\n> +        * UTC+14), there is something wrong if hour part is much\n> +        * larger than that. We might also want to check that the\n> +        * minutes are divisible by 15 or something too. (Offset of\n>         * Kathmandu, Nepal is UTC+5:45)\n>         */\n> -       if (n > 0 && min < 60) {\n> -               offset = hour*60+min;\n> +       if (min < 60 && hour < 24) {\n> +               int offset = hour * 60 + min;\n>                if (*date == '-')\n>                        offset = -offset;\n> -\n>                *offp = offset;\n>        }\n>        return end - date;\n>\n"}]}