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

From: Haitao Li <lihaitao@gmail.com>
Date: Sep 10, 2011, 08:06 UTC
Message-ID: <CABk5KLgyk_MRd6DLkdaK2s_5pxLQNipKmLJtCjNO2TaLLEr6UA@mail.gmail.com>
In-Reply-To: <7vhb4lvflb.fsf@alter.siamese.dyndns.org>

On Sat, Sep 10, 2011 at 12:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> 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.

Show 17 quoted lines

>> 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!