Re: [PATCH 2/2] date.c: allow compact version of ISO-8601 datetime

From: Danh Doan <congdanhqx@gmail.com>
Date: Apr 16, 2020, 11:16 UTC
Message-ID: <20200416111608.GB2285@danh.dev>
In-Reply-To: <20200415154157.GC2464307@coredump.intra.peff.net>

On 2020-04-15 11:41:57-0400, Jeff King <peff@peff.net> wrote:

Show 26 quoted lines

> On Wed, Apr 15, 2020 at 08:03:11AM -0700, Junio C Hamano wrote:
> 
> > > I worry a little this may conflict with other approxidate heuristics.
> > >
> > > The only one I can think of is an actual unix timestamp, though, and we
> > > already require that to have at least 9 digits (plus anybody wanting to
> > > use one robustly really should be using @12345678).
> > 
> > I am OK with 1/2, but I'd worry a LOT about this one, if we didn't
> > require 9 digits.  60/100 * 60/100 * 24/100 ~= 8.6% so limiting the
> > hour/min/sec to sensible values is not a useful protection against
> > ambiguity.  We'd essentially be declaring that a raw UNIX timestamp
> > without @ prefix is now hit-and-miss if we take this change, were
> > there not the "must be at least 9 digits" requirement.
> 
> Yeah, that was exactly why I poked at it.
> 
> Curiously, a1a5a6347b (Accept dates before 2000/01/01 when specified as
> seconds since the epoch, 2007-06-06), the commit which introduced the
> 9-digit rule, specifically says:
> 
>   There is now still a limit, 100000000, which is 1973/03/03 09:46:40
>   UTC, in order to allow that dates are represented as 8 digits.
> 
> but running "test-date 20100403" even back at that commit does not seem
> to work (nor does it work now).

check-parse 2010-04-03 doesn't work either. check-approxiate is going through different code path via approxidate_digit, but I'm not really sure which action should be take

> Doubly curious, some nearby code blames to 9f2b6d2936 (date/time: do not
> get confused by fractional seconds, 2008-08-16). So why don't fractional
> seconds work right now?

It doesn't work in that time, either:

	$ git checkout 9f2b6d2936
	$ make
	$ ./git --version
	git version 1.6.0.3.g9f2b6
	$ GIT_AUTHOR_DATE='2019-04-16 00:02:03.19-04:00' ./git-commit --allow-empty -m msg
	$ ./git-log --pretty=format:%aD -1
	Fri, 19 Apr 2019 00:02:03 +0700

The different is whether we stick the timezone (west of UTC) into the fractional or not. If we have both those conditions:

- Specify the timezone west of UTC
- Stick it with the fractional part
- The fractional part is parsed to  number less than 32

The the parser will think it's US style of month/day

Judging from the test code, it's likely people that write the code and the test get used to separate time and timezone.

And it's unlikely anyone will write fractional parts with only 1 digit (yield 100% positive with sticking timezone), or 2 digits (which yields 32% positive with sticking timezone), for 3 digits, the positive rate is 3.2%

Show 24 quoted lines

> 
> They sure don't seem to:
> 
>   [the 17 becomes a day-of-month]
>   $ t/helper/test-tool date approxidate 12:43:50.17
>   12:43:50.17 -> 2020-04-17 16:43:50 +0000
> 
> But I wonder if the new patch breaks the example from that commit
> message, which does work now:
> 
>   [current version; the 17 attaches to "days ago"]
>   $ t/helper/test-tool date approxidate 12:43:50.17.days.ago
>   12:43:50.17.days.ago -> 2020-03-29 16:43:50 +0000
> 
> It looks like the answer is yes:
> 
>   [with patch 1/2 applied; now it's a fractional second]
>   $ t/helper/test-tool date approxidate 12:43:50.17.days.ago
>   12:43:50.17.days.ago -> 2020-04-15 16:43:50 +0000
> 
> TBH I'm not sure how big a deal it is. The input is rather ambiguous for
> a strict left-to-right parser, and I'm not sure this case is worth doing
> more look-ahead parsing. But it's worth making a conscious decision
> about it.

Hm, I don't think "12:43:50.17.days.ago" is a normal input from user but I think "12:43:50.3.days.ago" on Monday wouldn't have much less user when compare with "yesterday at 12:43:50.17"

Show 33 quoted lines

> One alternative would be to restrict the fractional second handling only
> to the parse_date_basic(), which is quite a bit stricter. It shares
> match_digit() with approxidate(), so we'd probably have to pass in an
> extra flag to match_digit() to change the rules. It also means that:
> 
>   2020-04-14T12:43:50.17
> 
> would parse a fractional second, but:
> 
>   yesterday at 12:43:50.17
> 
> wouldn't.
> 
> > > We could probably tighten the heuristics a bit by insisting that the
> > > month and day be sensible. Or even year (see the 1900 to 2100 magic for
> > > the 4-digit year guess).
> > 
> > I do agree that we'd want 0 <= hr <= 24, 0 <= min <= 59, and 0 <=
> > sec <= 60 (or should this be 61?), but it is for correctness of the
> > new code.  It wouldn't have any value in disambiguation from other
> > formats, I would think.  So, from that point of view, I would buy
> > something like 1969 as a lower bound, but I am not sure if we
> > necessarily want an upper bound for the year.  What mistake are we
> > protecting us against?
> 
> No particular mistake. I was just suggesting to keep the heuristics in
> the new code as tight as possible to leave room for later changes (since
> approxidate by its nature is heuristics and hacks piled atop each
> other).
> 
> I do agree that year bounds are a questionable restriction. Right now
> Git's date code can only handle dates between 1970 and 2099, but it
> would be nice to change that.

2100 is used inside is_date, which is the place we try to guess what
     does those number could mean in multiple format.

I don't think we have ambiguity when parsing ISO-8601 compact date.

-- 
Danh