* [PATCH] date.c: Parse timezone with colon as separator
@ 2011-09-06 14:56 Haitao Li
2011-09-06 20:24 ` Jeff King
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Haitao Li @ 2011-09-06 14:56 UTC (permalink / raw)
To: git, gitster; +Cc: Haitao Li
Timezone designators including additional separator (`:`) are ignored.
Actually zone designators in below formats are all valid according to
ISO8601:2004, section 4.3.2:
[+-]hh, [+-]hhmm, [+-]hh:mm
Steps to reproduce the issue this patch fixes:
$ mkdir /tmp/test
$ cd /tmp/test
$ git init
$ echo 'timezone' > file.txt
$ git add .
$ git update-index
$ git write-tree
3e168d57e1c32a4598af134430384f0587581503
# Commit the tree returned above. Give a date with colon separated
# timezone
$ echo "Test commit" | \
TZ=UTC GIT_AUTHOR_DATE='2011-09-03T12:34:56+08:00' \
git commit-tree 3e168d57e1c32a4598af134430384f0587581503 | \
xargs git show | grep Date
Date: Sat Sep 3 12:34:56 2011 +0000
while the expected result is:
Date: Sat Sep 3 12:34:56 2011 +0800
^---
This patch teaches git recognizing zone designators with hours and
minutes separated by colon, or minutes are empty.
Signed-off-by: Haitao Li <lihaitao@gmail.com>
---
date.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/date.c b/date.c
index 896fbb4..8a94944 100644
--- a/date.c
+++ b/date.c
@@ -556,15 +556,21 @@ static int match_tz(const char *date, int *offp)
int min, hour;
int n = end - date - 1;
- min = offset % 100;
- hour = offset / 100;
+ if (n == 2 && *end == ':') {
+ hour = offset;
+ offset = strtoul(date+4, &end, 10);
+ min = offset % 100;
+ } else {
+ hour = offset / 100;
+ min = offset % 100;
+ }
/*
- * Don't accept any random crap.. At least 3 digits, and
+ * Don't accept any random crap.. At least 2 digits, and
* a valid minute. We might want to check that the minutes
* are divisible by 30 or something too.
*/
- if (min < 60 && n > 2) {
+ if (min < 60 && n > 1) {
offset = hour*60+min;
if (*date == '-')
offset = -offset;
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] date.c: Parse timezone with colon as separator
2011-09-06 14:56 [PATCH] date.c: Parse timezone with colon as separator Haitao Li
@ 2011-09-06 20:24 ` Jeff King
2011-09-07 3:00 ` Haitao Li
2011-09-07 5:46 ` [PATCH v2] date.c: Support iso8601 timezone formats Haitao Li
2011-09-09 10:10 ` [PATCH v3] " Haitao Li
2 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2011-09-06 20:24 UTC (permalink / raw)
To: Haitao Li; +Cc: git, gitster
On Tue, Sep 06, 2011 at 10:56:36PM +0800, Haitao Li wrote:
> Timezone designators including additional separator (`:`) are ignored.
> Actually zone designators in below formats are all valid according to
> ISO8601:2004, section 4.3.2:
> [+-]hh, [+-]hhmm, [+-]hh:mm
That seems like a sensible list to support, given that it is part of
iso8601 (though I was a little surprised after reading your subject
line, which would probably be better as "support iso8601 timezone
formats").
> ---
> date.c | 14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)
We should probably have new tests, too. I was going to suggest squashing
in the ones below, but your patch doesn't seem to work with the first
one:
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index f87abb5..9b326cd 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -40,6 +40,8 @@ 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 -05' '2008-02-14 20:30:45 -0500'
+check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
check_approxidate() {
If I am reading your commit message correctly, that should work, right?
-Peff
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] date.c: Parse timezone with colon as separator
2011-09-06 20:24 ` Jeff King
@ 2011-09-07 3:00 ` Haitao Li
0 siblings, 0 replies; 12+ messages in thread
From: Haitao Li @ 2011-09-07 3:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
> That seems like a sensible list to support, given that it is part of
> iso8601 (though I was a little surprised after reading your subject
> line, which would probably be better as "support iso8601 timezone
> formats").
I agree this is a better one! ;)
> We should probably have new tests, too. I was going to suggest squashing
> in the ones below, but your patch doesn't seem to work with the first
> one:
>
> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index f87abb5..9b326cd 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -40,6 +40,8 @@ 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 -05' '2008-02-14 20:30:45 -0500'
> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
> check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
>
> check_approxidate() {
>
> If I am reading your commit message correctly, that should work, right?
>
Yes, you are right. I will send an updated version to get this right.
Thank you for reviewing and giving suggestions!
/Haitao
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] date.c: Support iso8601 timezone formats
2011-09-06 14:56 [PATCH] date.c: Parse timezone with colon as separator Haitao Li
2011-09-06 20:24 ` Jeff King
@ 2011-09-07 5:46 ` Haitao Li
2011-09-07 17:00 ` Junio C Hamano
2011-09-09 10:10 ` [PATCH v3] " Haitao Li
2 siblings, 1 reply; 12+ messages in thread
From: Haitao Li @ 2011-09-07 5:46 UTC (permalink / raw)
To: git, Jeff King; +Cc: Haitao Li
Timezone designators including additional separator (`:`) are ignored.
Actually zone designators in below formats are all valid according to
ISO8601:2004, section 4.3.2:
[+-]hh, [+-]hhmm, [+-]hh:mm
Steps to reproduce the issue this patch fixes:
$ mkdir /tmp/test
$ cd /tmp/test
$ git init
$ echo 'timezone' > file.txt
$ git add .
$ git update-index
$ git write-tree
3e168d57e1c32a4598af134430384f0587581503
# Commit the tree returned above. Give a date with colon separated
# timezone
$ echo "Test commit" | \
TZ=UTC GIT_AUTHOR_DATE='2011-09-03T12:34:56+08:00' \
git commit-tree 3e168d57e1c32a4598af134430384f0587581503 | \
xargs git show | grep Date
Date: Sat Sep 3 12:34:56 2011 +0000
while the expected result is:
Date: Sat Sep 3 12:34:56 2011 +0800
^---
This patch teaches git recognizing zone designators with hours and
minutes separated by colon, or minutes are empty.
Signed-off-by: Haitao Li <lihaitao@gmail.com>
---
date.c | 32 ++++++++++++++++++++++++++------
t/t0006-date.sh | 5 +++++
2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 896fbb4..f970ea8 100644
--- a/date.c
+++ b/date.c
@@ -556,15 +556,35 @@ static int match_tz(const char *date, int *offp)
int min, hour;
int n = end - date - 1;
- min = offset % 100;
- hour = offset / 100;
+ /*
+ * ISO8601:2004(E) allows time zone designator been separated
+ * by a clone in the extended format
+ */
+ if (*end == ':') {
+ if (isdigit(end[1])) {
+ hour = offset;
+ min = strtoul(end+1, &end, 10);
+ } else {
+ /* Mark as invalid */
+ n = -1;
+ }
+ } else {
+ /* Only hours specified */
+ if (n == 1 || n == 2) {
+ hour = offset;
+ min = 0;
+ } else {
+ hour = offset / 100;
+ min = offset % 100;
+ }
+ }
/*
- * Don't accept any random crap.. At least 3 digits, and
- * a valid minute. We might want to check that the minutes
- * are divisible by 30 or something too.
+ * Don't accept any random crap.. We might want to check that
+ * the minutes are divisible by 15 or something too. (Offset of
+ * Kathmandu, Nepal is UTC+5:45)
*/
- if (min < 60 && n > 2) {
+ if (n > 0 && min < 60 && hour < 25) {
offset = hour*60+min;
if (*date == '-')
offset = -offset;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index f87abb5..5235b7a 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'
+check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'
+check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
+check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
+check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
check_approxidate() {
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2] date.c: Support iso8601 timezone formats
2011-09-07 5:46 ` [PATCH v2] date.c: Support iso8601 timezone formats Haitao Li
@ 2011-09-07 17:00 ` Junio C Hamano
2011-09-08 2:53 ` Haitao Li
0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2011-09-07 17:00 UTC (permalink / raw)
To: Haitao Li; +Cc: git, Jeff King
Haitao Li <lihaitao@gmail.com> writes:
> diff --git a/date.c b/date.c
> index 896fbb4..f970ea8 100644
> --- a/date.c
> +++ b/date.c
> @@ -556,15 +556,35 @@ static int match_tz(const char *date, int *offp)
> int min, hour;
> int n = end - date - 1;
>
> - min = offset % 100;
> - hour = offset / 100;
> + /*
> + * ISO8601:2004(E) allows time zone designator been separated
> + * by a clone in the extended format
> + */
> + if (*end == ':') {
> + if (isdigit(end[1])) {
> + hour = offset;
> + min = strtoul(end+1, &end, 10);
> + } else {
> + /* Mark as invalid */
> + n = -1;
> + }
> + } else {
> + /* Only hours specified */
That comment belongs to inside the following if() {...}.
> + if (n == 1 || n == 2) {
... i.e. here.
> + hour = offset;
> + min = 0;
> + } else {
> + hour = offset / 100;
> + min = offset % 100;
> + }
> + }
>
> /*
> + * Don't accept any random crap.. We might want to check that
> + * the minutes are divisible by 15 or something too. (Offset of
> + * Kathmandu, Nepal is UTC+5:45)
> */
> - if (min < 60 && n > 2) {
> + if (n > 0 && min < 60 && hour < 25) {
What is this "hour < 25" about? Aren't we talking about the UTC offset
value that come after the [-+] sign?
I do not mind adding a new check, but I do mind if it adds a check with
not much value. Even at Pacific/Kiritimati, the offset is 14; the new
check seems a bit too lenient.
> offset = hour*60+min;
> if (*date == '-')
> offset = -offset;
> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index f87abb5..5235b7a 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'
> +check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'
> +check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
> +check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
> check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
>
> check_approxidate() {
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] date.c: Support iso8601 timezone formats
2011-09-06 14:56 [PATCH] date.c: Parse timezone with colon as separator Haitao Li
2011-09-06 20:24 ` Jeff King
2011-09-07 5:46 ` [PATCH v2] date.c: Support iso8601 timezone formats Haitao Li
@ 2011-09-09 10:10 ` Haitao Li
2011-09-09 16:35 ` Junio C Hamano
2 siblings, 1 reply; 12+ messages in thread
From: Haitao Li @ 2011-09-09 10:10 UTC (permalink / raw)
To: gitster; +Cc: Haitao Li, git
Timezone designators including additional separator (`:`) are ignored.
Actually zone designators in below formats are all valid according to
ISO8601:2004, section 4.3.2:
[+-]hh, [+-]hhmm, [+-]hh:mm
Steps to reproduce the issue this patch fixes:
$ mkdir /tmp/test
$ cd /tmp/test
$ git init
$ echo 'timezone' > file.txt
$ git add .
$ git update-index
$ git write-tree
3e168d57e1c32a4598af134430384f0587581503
# Commit the tree returned above. Give a date with colon separated
# timezone
$ echo "Test commit" | \
TZ=UTC GIT_AUTHOR_DATE='2011-09-03T12:34:56+08:00' \
git commit-tree 3e168d57e1c32a4598af134430384f0587581503 | \
xargs git show | grep Date
Date: Sat Sep 3 12:34:56 2011 +0000
while the expected result is:
Date: Sat Sep 3 12:34:56 2011 +0800
^---
This patch teaches git recognizing zone designators with hours and
minutes separated by colon, or minutes are empty.
Signed-off-by: Haitao Li <lihaitao@gmail.com>
---
date.c | 32 ++++++++++++++++++++++++++------
t/t0006-date.sh | 5 +++++
2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 896fbb4..f8722c1 100644
--- a/date.c
+++ b/date.c
@@ -556,15 +556,35 @@ static int match_tz(const char *date, int *offp)
int min, hour;
int n = end - date - 1;
- min = offset % 100;
- hour = offset / 100;
+ /*
+ * ISO8601:2004(E) allows time zone designator been separated
+ * by a clone in the extended format
+ */
+ if (*end == ':') {
+ if (isdigit(end[1])) {
+ hour = offset;
+ min = strtoul(end+1, &end, 10);
+ } else {
+ /* Mark as invalid */
+ n = -1;
+ }
+ } else {
+ if (n == 1 || n == 2) {
+ /* Only hours specified */
+ hour = offset;
+ min = 0;
+ } else {
+ hour = offset / 100;
+ min = offset % 100;
+ }
+ }
/*
- * Don't accept any random crap.. At least 3 digits, and
- * a valid minute. We might want to check that the minutes
- * are divisible by 30 or something too.
+ * Don't accept any random crap.. We might want to check that
+ * the minutes are divisible by 15 or something too. (Offset of
+ * Kathmandu, Nepal is UTC+5:45)
*/
- if (min < 60 && n > 2) {
+ if (n > 0 && min < 60) {
offset = hour*60+min;
if (*date == '-')
offset = -offset;
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index f87abb5..5235b7a 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'
+check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'
+check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
+check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
+check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
check_approxidate() {
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3] date.c: Support iso8601 timezone formats
2011-09-09 10:10 ` [PATCH v3] " Haitao Li
@ 2011-09-09 16:35 ` Junio C Hamano
2011-09-09 17:04 ` Junio C Hamano
2011-09-10 8:06 ` Haitao Li
0 siblings, 2 replies; 12+ messages in thread
From: Junio C Hamano @ 2011-09-09 16:35 UTC (permalink / raw)
To: Haitao Li; +Cc: git, Jeff King
Haitao Li <lihaitao@gmail.com> writes:
> Timezone designators including additional separator (`:`) are ignored.
> Actually zone designators in below formats are all valid according to
> ISO8601:2004, section 4.3.2:
> [+-]hh, [+-]hhmm, [+-]hh:mm
Thanks for a re-roll.
> This patch teaches git recognizing zone designators with hours and
> minutes separated by colon, or minutes are empty.
The last sentence above makes it sound as if you are accepting
"2011-09-17 12:34:56 +09:"
but I suspect that is not what you intend to allow. Perhaps "we allowed
hh and hhmm and this teaches Git to recognize hh:mm format as well"?
> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index f87abb5..5235b7a 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'
> +check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'
> +check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
> +check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
> check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
The above are from Peff, no? We should credit him for tests in the
proposed log message.
Because the three formats 8601 specifies are "hh", "hhmm", or "hh:mm"
after +/-, among the above new tests, it appears to me that zone
designators "-5" and "-:30" should yield "bad", instead of being accepted.
The same for "+09:" I mentioned above, which is not in the new test.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] date.c: Support iso8601 timezone formats
2011-09-09 16:35 ` Junio C Hamano
@ 2011-09-09 17:04 ` Junio C Hamano
2011-09-09 20:46 ` Junio C Hamano
2011-09-10 8:29 ` Haitao Li
2011-09-10 8:06 ` Haitao Li
1 sibling, 2 replies; 12+ messages in thread
From: Junio C Hamano @ 2011-09-09 17:04 UTC (permalink / raw)
To: Haitao Li; +Cc: git, Jeff King
Junio C Hamano <gitster@pobox.com> writes:
> Haitao Li <lihaitao@gmail.com> writes:
>
>> Timezone designators including additional separator (`:`) are ignored.
>> Actually zone designators in below formats are all valid according to
>> ISO8601:2004, section 4.3.2:
>> [+-]hh, [+-]hhmm, [+-]hh:mm
>
> Thanks for a re-roll.
>
>> This patch teaches git recognizing zone designators with hours and
>> minutes separated by colon, or minutes are empty.
>
> The last sentence above makes it sound as if you are accepting
>
> "2011-09-17 12:34:56 +09:"
>
> but I suspect that is not what you intend to allow. Perhaps "we allowed
> hh and hhmm and this teaches Git to recognize hh:mm format as well"?
Also, I do not quite understand why the match_tz() logic needs to be that
long.
Wouldn't something like this patch (on top of yours) easier to follow?
date.c | 50 +++++++++++++++++++++-----------------------------
1 files changed, 21 insertions(+), 29 deletions(-)
diff --git a/date.c b/date.c
index f8722c1..6079b1a 100644
--- a/date.c
+++ b/date.c
@@ -551,44 +551,36 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt
static int match_tz(const char *date, int *offp)
{
+ int min;
char *end;
- int offset = strtoul(date+1, &end, 10);
- int min, hour;
- int n = end - date - 1;
+ int hour = strtoul(date + 1, &end, 10);
+ int n = end - (date + 1);
- /*
- * ISO8601:2004(E) allows time zone designator been separated
- * by a clone in the extended format
- */
- if (*end == ':') {
- if (isdigit(end[1])) {
- hour = offset;
- min = strtoul(end+1, &end, 10);
- } else {
- /* Mark as invalid */
- n = -1;
- }
- } else {
- if (n == 1 || n == 2) {
- /* Only hours specified */
- hour = offset;
- min = 0;
- } else {
- hour = offset / 100;
- min = offset % 100;
- }
+ if (n == 4) {
+ /* hhmm */
+ min = hour % 100;
+ hour = hour / 100;
+ } else if (n != 2) {
+ min = 99; /* random crap */
+ } else if (*end == ':') {
+ /* hh:mm? */
+ min = strtoul(end + 1, &end, 10);
+ if (end - (date + 1) != 5)
+ min = 99; /* random crap */
}
/*
- * Don't accept any random crap.. We might want to check that
- * the minutes are divisible by 15 or something too. (Offset of
+ * Don't accept any random crap. Even though some places have
+ * offset larger than 12 hours (e.g. Pacific/Kiritimati is at
+ * UTC+14), there is something wrong if hour part is much
+ * larger than that. We might also want to check that the
+ * minutes are divisible by 15 or something too. (Offset of
* Kathmandu, Nepal is UTC+5:45)
*/
- if (n > 0 && min < 60) {
- offset = hour*60+min;
+ if (min < 60 && hour < 24) {
+ int offset = hour * 60 + min;
if (*date == '-')
offset = -offset;
-
*offp = offset;
}
return end - date;
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3] date.c: Support iso8601 timezone formats
2011-09-09 17:04 ` Junio C Hamano
@ 2011-09-09 20:46 ` Junio C Hamano
2011-09-10 8:29 ` Haitao Li
1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2011-09-09 20:46 UTC (permalink / raw)
To: Haitao Li; +Cc: git, Jeff King
Junio C Hamano <gitster@pobox.com> writes:
> Also, I do not quite understand why the match_tz() logic needs to be that
> long.
>
> Wouldn't something like this patch (on top of yours) easier to follow?
>
> date.c | 50 +++++++++++++++++++++-----------------------------
> 1 files changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/date.c b/date.c
> index f8722c1..6079b1a 100644
> --- a/date.c
> +++ b/date.c
> @@ -551,44 +551,36 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt
>
> static int match_tz(const char *date, int *offp)
> {
> + int min;
Micronit; this should be "int min = 0", as parsing of "hh" form relies on
the if/elseif/else cascade to parse only the hour part and leaving min to
the original value.
> char *end;
> + int hour = strtoul(date + 1, &end, 10);
> + int n = end - (date + 1);
>
> + if (n == 4) {
> + /* hhmm */
> + min = hour % 100;
> + hour = hour / 100;
> + } else if (n != 2) {
> + min = 99; /* random crap */
> + } else if (*end == ':') {
> + /* hh:mm? */
> + min = strtoul(end + 1, &end, 10);
> + if (end - (date + 1) != 5)
> + min = 99; /* random crap */
> }
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3] date.c: Support iso8601 timezone formats
2011-09-09 17:04 ` Junio C Hamano
2011-09-09 20:46 ` Junio C Hamano
@ 2011-09-10 8:29 ` Haitao Li
1 sibling, 0 replies; 12+ messages in thread
From: Haitao Li @ 2011-09-10 8:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
>
> Also, I do not quite understand why the match_tz() logic needs to be that
> long.
>
> Wouldn't something like this patch (on top of yours) easier to follow?
I was wrong about accepting one digit in hours or minutes. And yes
your version is conciser and easier to follow. Thanks!
>
> date.c | 50 +++++++++++++++++++++-----------------------------
> 1 files changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/date.c b/date.c
> index f8722c1..6079b1a 100644
> --- a/date.c
> +++ b/date.c
> @@ -551,44 +551,36 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt
>
> static int match_tz(const char *date, int *offp)
> {
> + int min;
> char *end;
> - int offset = strtoul(date+1, &end, 10);
> - int min, hour;
> - int n = end - date - 1;
> + int hour = strtoul(date + 1, &end, 10);
> + int n = end - (date + 1);
>
> - /*
> - * ISO8601:2004(E) allows time zone designator been separated
> - * by a clone in the extended format
> - */
> - if (*end == ':') {
> - if (isdigit(end[1])) {
> - hour = offset;
> - min = strtoul(end+1, &end, 10);
> - } else {
> - /* Mark as invalid */
> - n = -1;
> - }
> - } else {
> - if (n == 1 || n == 2) {
> - /* Only hours specified */
> - hour = offset;
> - min = 0;
> - } else {
> - hour = offset / 100;
> - min = offset % 100;
> - }
> + if (n == 4) {
> + /* hhmm */
> + min = hour % 100;
> + hour = hour / 100;
> + } else if (n != 2) {
> + min = 99; /* random crap */
> + } else if (*end == ':') {
> + /* hh:mm? */
> + min = strtoul(end + 1, &end, 10);
> + if (end - (date + 1) != 5)
> + min = 99; /* random crap */
> }
>
> /*
> - * Don't accept any random crap.. We might want to check that
> - * the minutes are divisible by 15 or something too. (Offset of
> + * Don't accept any random crap. Even though some places have
> + * offset larger than 12 hours (e.g. Pacific/Kiritimati is at
> + * UTC+14), there is something wrong if hour part is much
> + * larger than that. We might also want to check that the
> + * minutes are divisible by 15 or something too. (Offset of
> * Kathmandu, Nepal is UTC+5:45)
> */
> - if (n > 0 && min < 60) {
> - offset = hour*60+min;
> + if (min < 60 && hour < 24) {
> + int offset = hour * 60 + min;
> if (*date == '-')
> offset = -offset;
> -
> *offp = offset;
> }
> return end - date;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] date.c: Support iso8601 timezone formats
2011-09-09 16:35 ` Junio C Hamano
2011-09-09 17:04 ` Junio C Hamano
@ 2011-09-10 8:06 ` Haitao Li
1 sibling, 0 replies; 12+ messages in thread
From: Haitao Li @ 2011-09-10 8:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
On Sat, Sep 10, 2011 at 12:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Haitao Li <lihaitao@gmail.com> writes:
>
>> Timezone designators including additional separator (`:`) are ignored.
>> Actually zone designators in below formats are all valid according to
>> ISO8601:2004, section 4.3.2:
>> [+-]hh, [+-]hhmm, [+-]hh:mm
>
> Thanks for a re-roll.
>
>> This patch teaches git recognizing zone designators with hours and
>> minutes separated by colon, or minutes are empty.
>
> The last sentence above makes it sound as if you are accepting
>
> "2011-09-17 12:34:56 +09:"
>
> but I suspect that is not what you intend to allow. Perhaps "we allowed
> hh and hhmm and this teaches Git to recognize hh:mm format as well"?
>
Yes, this is a better one. Sorry for my English, and thanks for the suggestion.
>> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
>> index f87abb5..5235b7a 100755
>> --- a/t/t0006-date.sh
>> +++ b/t/t0006-date.sh
>> @@ -40,6 +40,11 @@ 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 -0015' '2008-02-14 20:30:45 -0015'
>> +check_parse '2008-02-14 20:30:45 -5' '2008-02-14 20:30:45 -0500'
>> +check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
>> +check_parse '2008-02-14 20:30:45 -:30' '2008-02-14 20:30:45 +0000'
>> +check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
>> check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
>
> The above are from Peff, no? We should credit him for tests in the
> proposed log message.
Yes, we should credit Peff. Sorry for not knowing log message is used for this.
>
> Because the three formats 8601 specifies are "hh", "hhmm", or "hh:mm"
> after +/-, among the above new tests, it appears to me that zone
> designators "-5" and "-:30" should yield "bad", instead of being accepted.
Yes, the spec clearly states 2 digits are mandatory. "-5" should be
regarded as invalid here.
The above test *ignores* ":30" by setting offset to "+0000", this is
to conform to how it works previously, less than 3 digits in offset
are ignored. I agree it's better to *reject* them instead.
> The same for "+09:" I mentioned above, which is not in the new test.
>
Will add.
Thanks again for the suggestions!
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-09-10 8:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-06 14:56 [PATCH] date.c: Parse timezone with colon as separator Haitao Li
2011-09-06 20:24 ` Jeff King
2011-09-07 3:00 ` Haitao Li
2011-09-07 5:46 ` [PATCH v2] date.c: Support iso8601 timezone formats Haitao Li
2011-09-07 17:00 ` Junio C Hamano
2011-09-08 2:53 ` Haitao Li
2011-09-09 10:10 ` [PATCH v3] " Haitao Li
2011-09-09 16:35 ` Junio C Hamano
2011-09-09 17:04 ` Junio C Hamano
2011-09-09 20:46 ` Junio C Hamano
2011-09-10 8:29 ` Haitao Li
2011-09-10 8:06 ` Haitao Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.