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

[Marcus Comstedt <marcus@mc.pp.se>]

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