[Monotone-devel] Re: nvm.dates review

[Zack Weinberg]

I've now pushed some revs to nvm.dates in which I made many of the
changes you said you would rather not do.  I feel I ought to explain
why I'm insisting on those changes.

I have some further changes in mind for date handling, but I'm not
going to get to them for a couple days.  If you would like to merge
nvm.dates and nvm.dates.statistics into mainline now, please go ahead.

On Mon, Oct 27, 2008 at 8:42 AM, Markus Wanner <address@hidden> wrote:
> Zack Weinberg wrote:
>> +  // initialize from a unix timestamp
>> +  date_t(u64 d);
>>
>> I am not sure I like having this be a constructor instead of a factory
>> function.  I don't feel strongly about it, but it was more
>> self-documenting the other way.  Also, you still have the ::now() and
>> ::from_string() factories, so now there's both factories and public
>> constructors, which is confusing.
>
> I felt the factory function to be overly verbose. Guess that's a matter
> of taste. I didn't change that for now.

I made the ::from_string() factory into a constructor but left ::now()
and the other constructors alone.  That's enough consistency for me.

>> +  void gmtime(struct tm & tm) const;
>> +  void mktime(struct tm const & tm);
>
> To keep it clear that these methods are about equivalent to the system
> functions, I've now prefixed them with "our_". Feel free to rename to
> whatever else you find more appropriate, but please keep it clear that
> these provide similar functionality.

I changed "our_mktime" to "our_timegm".  The system function mktime()
operates in the local timezone so it is clearer not to use that name
at all for a function that operates in GMT.  Some (unfortunately not
all) systems have timegm() that operates in GMT.

>> Here and elsewhere - since you took out the buffer that would be too
>> small in CE 10000, why are you still restricting the year to <= 9999?
>
> Because that's been the limit before in date_t::now() and I thought 9999
> is far enough in the future for our use.

Practically speaking, 9999 is far enough in the future, I imagine by
then we'll all be using something totally different.  But making the
code work as far out as you possibly can is good anyway, because it
forces you to make the algorithm more robust.  I made our_gmtime work
as far out as I could (not to 2147483647 -- it turns out that we
overflow a signed 64-bit millisecond count before then) and in the
process was forced to come up with a technique for calculating the
year that is just plain better, even in the date range we'll actually
be using.

>> Another advantage of not using struct tm is, you could report the
>> milliseconds too.
>
> Uh.. we certainly don't want that for date certs, do we? Any other use
> cases?

I haven't done this part yet, but we do actually want milliseconds in
date certs, or at least we want to accept and preserve them when they
show up.  I don't remember the exact details, but conversion from some
foreign formats produces date certs with milliseconds in them, which
is why the from-string constructor currently accepts and ignores
digits after the decimal point.

> Please bear in mind what the purpose for this change was/is: adding the
> ability to get timestamp differences, adding and subtracting differences
> to them. IMO this has already taken way too long and we should better
> spend our dev-time on more relevant things.

I want to mention four more things that we should try to get out of
our date handling.  With your changes and mine on top of them, we're
very close, but some more work is still needed.  I'm planning to do
all of this, but if you feel inspired to do 'em first, please go
ahead.

First, when we print dates, we should print 'em in local time, not
GMT, and we should offer the user the option to choose their preferred
date format.  This involves adding another as_* function that takes a
strftime() format string (which higher layers will get from a Lua
variable) and runs the internal representation through the system
localtime() function.  This is also another use for get_epoch_offset
and is what I had in mind when I requested it, btw.

Second, similarly, the --date option should operate in local time by
default.  The headache with this is that while mktime() is standard,
filling in a 'struct tm' correctly is harder than one might think (as
I mentioned before); the nonstandard 'tm_gmtoff' and 'tm_zone' fields
may affect the result.  (The Linux manpage doesn't say anything about
them; glibc does have those fields but I *think* its mktime ignores
them; the FreeBSD manpage mentions the fields but doesn't say what
mktime does with them; I would want to survey other systems before
actually doing this part.)

Third, I do think we should request millisecond-accurate timestamps in
now(), pass them through date certs, and print them when possible.
This will be tricky in the face of the first one because strftime()
and struct tm don't have millisecond fields -- I really don't want to
implement my own strftime().  Maybe we could get away with a
subsequent call to sprintf() or FL() if we see some kind of marker in
the string.

And then, of course, there is the question of more general date
parsing.  I'm actually tempted to do that in Lua, maybe translating
Mark Pilgrim's Universal Feed Parser's date parser into that language,
and leave the from-string constructor as a fast routine that only
knows the date cert format.

zw