Re: [PATCH 2/2] Accept the timezone specifiers [+-]hh:mm and [+-]hh in addition to [+-]hhmm

From: Marcus Comstedt <marcus@mc.pp.se>
Date: May 19, 2010, 17:21 UTC
Message-ID: <yf9ocgbkdsg.fsf@chiyo.mc.pp.se>
In-Reply-To: <7v632karpe.fsf@alter.siamese.dyndns.org>

Hi Junio.

Thanks for reviewing this patch.

Junio C Hamano <gitster@pobox.com> writes:

> I don't recall seeing in ISO 8601 that +hh or -hh without minute
> resolution was allowed, but I don't have my copy of ISO 8601 with me (they
> are packed and are still in transit with my household goods) so I'll take
> your word for it for now [*1*].

In the final draft of 8601:2000 (which is the only version I have), section 5.3.4.1 states that "[...] the representation of the difference can be expressed in hours and minutes, or hours only." Examples of this then follow in that section and the next one. Maybe they changed it in the final version (or it differs from another release of the standard)? I wish you could "git log -S" ISO standards... :-) Wikipedia also agrees that it is allowed by the standard though.

> But the placement of this second hunk is somewhat curious.  Why doesn't the
> updated function look like this?

[...]

I was perhaps treading a bit over-cautiously. The placement allowed me to leave the existing code both syntactically and semantically unaltered. After all, there was nothing wrong with the old code per se, I was just adding new functionality. I also wanted the two changes independent, in case you wanted one but not the other.

I can concede that your variant leaves a more appealing end result though. (Except for the fact that "n == 2" is needlessly tested in the inner if. ;)

One thing though: Shouldn't 1 be returned for bad crap rather than 0? Seems to me parse_date will get stuck otherwise, because the sign will never be consumed. In fact, the old code would consume both the sign and the initial sequence of digits in the crap case. Consuming just the sign would leave the digits to be handled by match_digit, which may or may not regard it as non-crap. Good or bad, I don't know. But it might cause regressions.

I'll play around a little with the code and perform some new unit tests, and then resubmit a new patch with the suggested structure.

  // Marcus