[Monotone-devel] Re: nvm.dates review of the review

[Markus Wanner]

Hello Zack,

Zack Weinberg wrote:
> I've now pushed some revs to nvm.dates..

Very cool! Thank you.

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

Barry any objections I plan to land both tomorrow.

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

Cool, makes sense. (I've adjusted nvm.dates.statistics accordingly).

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

Looks fine.

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

Cool.

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

IIRC that's tailor, which wrote date certs with milliseconds. We
certainly should to preserve this information, yes.

Are you saying you want to default to storing milliseconds in the date
cert? I personally don't think that's necessary, but I don't mind much.

What I *do* mind is that I prefer not to see the milliseconds part in
the normal case, because it's basically noise which just clutters the
output.

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

Very good compilation. Sadly, I won't have time to look into this again
anytime soon.

Regards

Markus Wanner