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

[Junio C Hamano <gitster@pobox.com>]

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;