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). 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? 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. 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. -Peff
