Date handling.

The date handling is somewhat unreliable. We render dates into textual representation using the committer's locale (day names, etc), then later attempt to interpret that in some other locale. And we were just using localtime without even specifying the timezone so the timestamp was fairly randomised anyway. In fact, an $AUTHOR_DATE environment variable was making its way into the database entirely unchecked.

I see two possible solutions:
	1. Just store seconds-since-GMT-epoch and if we really want, the
	   timezone as auxiliary information.
	2. Store dates in RFC2822 form.

Unless someone convincingly expresses a preference before I get to work and start playing with it, I'll implement the latter.

On Thu, 14 Apr 2005, David Woodhouse wrote:

Yeah, I think this is the right thing to do. I can change "commit" to do it.

I used to think that the date was purely a "enforced comment" (like the committer info is, as far as git is concerned), which is why I used the simple textual representation. But yes, when I wrote that "rev-tree" thing I did curse that and consider just changing it.

It's still just technically a "hint", since time isn't synchronized in any way (and in a distributed system, time _cannot_ be synchronized). But it's a useful hint, so ..

I do like text output, but if it is painful, the "unix seconds" format is certainly a hell of a lot simpler. And quite frankly, if we change it, we might as well just change it all the way. So I'd almost prefer (1).

But "He who does the work gets to choose the implementation". And I do agree that this is a bad format decision, and that we should change it. It shouldn't even be that painful. Only "rev-tree" cares, and even rev-tree doesn't care _that_ deeply.

		Linus

On Thu, 14 Apr 2005, Linus Torvalds wrote:

I take that back. I'd be much happier with you doing and testing it, because now I'm crashing.

		Linus

On Thu, 2005-04-14 at 02:12 -0700, Linus Torvalds wrote:

OK. commit-tree now eats RFC2822 dates as AUTHOR_DATE because that's what you're going to want to feed it. We store seconds since UTC epoch, we add the author's or committer's timezone as auxiliary data so that dates can be pretty-printed in the original timezone later if anyone cares. I left the date parsing in rev-tree.c for backward compatibility but it can be dropped when we change to base64 :)

Yes, glibc sucks and strptime is a pile of crap. We have to parse it ourselves.

With a UTC date, why would anyone care in which timezone the commit was made? Any pretty printing would most likely be prettiest if it is done relative to the timezone of the person looking at the commit record, not the person who created the record.

If we do need the timezone, then I think we also need the latitude of the committer too, so that we know whether to interpret "July" as summer or winter :-)

-Tony

On Thu, 2005-04-14 at 12:19 -0700, tony.luck@intel.com wrote:

I'd prefer not to lose the information. If someone has committed a change at 2am, I like to know that it was 2am for _them_. It helps me decide where to look first for the cause of problems. :)

It also helps disambiguate certain comments, especially those involving words or phrases such as "yesterday" or "this afternoon".

On Thu, Apr 14, 2005 at 06:38:36PM +0100, David Woodhouse wrote:

...

I noticed that some commit timestamps seemed to be off, looking into it a bit more it seems like mktime is influenced by the setting of the local TZ environment. However in parse_rfc2822_date we are trying to interpret a time in the timezone of the original author not in the timezone of the committer.

Here is a short test program that I believe shows the problem.

The question is, do we want to just calculate the time_t offset ourselves without using mktime, or force the TZ environment to UTC.

Jan

/* cc -o mktime mktime.c ; ./mktime
 *
 * I get the following output,
 *   current 18000
 *   TZ=EST 18000
 *   TZ=UTC 0
 *   TZ=CET -3600
 */

#include <time.h> #include <stdlib.h> #include <stdio.h>

int main(int argc, char **argv)
{
    struct tm tm = { 0, };
    time_t zero;

    /* 1970-01-01 00:00:00 UTC, should map to 'time_t 0' */
    tm.tm_mday = 1;
    tm.tm_year = 70;
			    zero = mktime(&tm); printf("current %d\n", zero);
    setenv("TZ", "EST", 1); zero = mktime(&tm); printf("TZ=EST %d\n", zero);
    setenv("TZ", "UTC", 1); zero = mktime(&tm); printf("TZ=UTC %d\n", zero);
    setenv("TZ", "CET", 1); zero = mktime(&tm); printf("TZ=CET %d\n", zero);
}

Wouldn't it be easier to force GMT or UTC as the base timezone for the application. This would remove any confusion between different timezones.

On Sat, 2005-04-23 at 23:04 -0400, Jan Harkes wrote:

Ewww. I missed that in the documentation. I suppose I should have worked it out having empirically determined that it ignores the tm_gmtoff field.

I don't think we want to be in the business of counting leap seconds; we need to let the system do it. I don't much like setting TZ to UTC though -- how about we use your test case to find the offset and subtract that?

Does this work?

David Woodhouse <dwmw2@infradead.org> writes:

Nope, daylight savings time breaks this, since you may or may not be in the same time zone on January 1st as you are at the current time.

However, you don't need to count leap seconds when you implement your own mktime, since mktime doesn't have to take leap seconds into account. Unix timestamps, unless you're using TAI, don't include leap seconds.

On Sun, Apr 24, 2005 at 04:38:49PM +1000, David Woodhouse wrote:

As Russ mentioned, that probably doesn't work with daylight savings time. However I did some testing and it looks like the following lines around mktime make it work as we would expect.

    tm.tm_isdst = -1;
    then = mktime(&tm);
    then += tm.tm_gmtoff;

Attached is the program I used to test it, it seems pretty much unfazed by changes to the TZ environment variable. Although I tested around a daylight savings time switch, I'm still not 100% sure if it doesn't mess up in some corner case.

Jan

#include <time.h> #include <stdlib.h> #include <stdio.h>

time_t mkutctime(struct tm *tm, int offset)
{
    time_t time;

    /* we don't know whether our timezone happens to be dst or not, let libc
     * figure that one out. */
    tm->tm_isdst = -1;

    /* interpret struct tm in the local timezone */
    time = mktime(tm);
    if (time == -1) return -1;

    /* libc lets us know how many seconds our local time differs from UTC
     * this is a non-standard BSD extension, which is probably not as
     * portable, but it seems to work. */
    time += tm->tm_gmtoff;

    /* However as the passed in struct tm was not UTC but in some other
     * timezone, we still have subtract the offset that came with the
     * RFC2822 date */
    time -= offset;

    return time;
}

int main(int argc, char **argv)
{
    struct tm tm = { 0, };
    time_t time;

    tm.tm_year = 70;
    tm.tm_mday = 1;
    time = mkutctime(&tm, 0);
    printf("1970-01-01 00:00:00 UTC = 0 (%d)\n", time);

    tm.tm_year = 105;
    tm.tm_mon = 2;
    tm.tm_mday = 17;
    tm.tm_hour = 20;
    tm.tm_min = 58;
    tm.tm_sec = 31;
    time = mkutctime(&tm, -5 * 3600);
    printf("2005-03-17 20:58:31 EST = 1111111111 (%d)\n", time);

    tm.tm_mon = 3;
    tm.tm_mday = 3;
    tm.tm_hour = 1;
    tm.tm_min = 59;
    tm.tm_sec = 59;
    time = mkutctime(&tm, -5 * 3600);
    printf("2005-04-03 01:59:59 EST = 1112511599 (%d)\n", time);

    tm.tm_hour = 3;
    tm.tm_min = 0;
    tm.tm_sec = 0;
    time = mkutctime(&tm, -4 * 3600);
    printf("2005-04-03 03:00:00 EDT = 1112511600 (%d)\n", time);
}

Jan Harkes <jaharkes@cs.cmu.edu> writes:

I don't know what sort of portability you're striving for, but many platforms don't have tm.tm_gmtoff. But reimplementing mktime from scratch isn't particularly hard so long as you don't need some of the "extra" features of mktime (canonicalizing a struct tm or accepting out of range values and doing the "right thing").

I came in a little late to this discussion, but I gather that the overall goal here is parsing RFC 2822 dates. You're all certainly welcome to take the code that I wrote for INN to do this if you wish, although it parses the full RFC 2822 syntax and therefore may accept things you consider insane (comments, newlines, etc.) Or you're welcome to cherry-pick bits and pieces out of it (like mktime_utc). This code has a fairly extensive test suite and has also been tested against the old INN parsedate function on ~2M Usenet articles.

All of this code is my own work, and as far as I'm concerned it's in the public domain or as close of an approximation that one can get to that in your local legal environment.

The code is largish and needs some Autoconf support, so I won't just send it to the list unless someone wants it, but let me know if you do. You can also get it by downloading INN from:

    <ftp://ftp.isc.org/isc/inn/snapshots/>

(getting the latest CURRENT snapshot) and looking in lib/date.c. You don't need the parsedate_nntp stuff, and you probably don't care about parsedate_rfc2822_lax, which accepts common violations of RFC 2822 syntax found in Usenet messages. The test suite is in tests/lib/date-t.c.

On Thu, 2005-04-14 at 02:00 -0700, Linus Torvalds wrote:

Text _output_ is easy to generate; we don't need to store text in the database for that. So I've changed my mind -- I prefer (1) too.