Re: [PATCH v2] date.c: Support iso8601 timezone formats

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH v2] date.c: Support iso8601 timezone formats

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:59

Haitao Li [off-list ref] writes:
quoted hunk
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.
quoted hunk
 		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() {

Re: [PATCH v2] date.c: Support iso8601 timezone formats

From: Haitao Li <hidden>
Date: 2016-06-15 22:51:59

quoted
+     } else {
+             /* Only hours specified */
That comment belongs to inside the following if() {...}.
quoted
+             if (n == 1 || n == 2) {
... i.e. here.
Good catch!
quoted
-     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.
I think it's not "too" lenient. UTC+14 was "invented" in 1995 [1].
Maybe UTC+15 would be added someday for some reason? How about
changing to "hour < 24", this is how ICU checks offset validity [2].

1. http://en.wikipedia.org/wiki/UTC%2B14#History
2. http://bugs.icu-project.org/trac/browser/icu/tags/release-4-8-1/source/i18n/timezone.cpp#L1482
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help