Re: timezone related bug of git
From: Junio C Hamano <hidden>
Date: 2021-11-01 18:18:10
Possibly related (same subject, not in this thread)
- 2021-11-01 · Re: timezone related bug of git · Dongsheng Song <hidden>
- 2021-11-01 · Re: timezone related bug of git · Jeff King <hidden>
- 2021-10-31 · Re: timezone related bug of git · Junio C Hamano <hidden>
- 2021-10-31 · Re: timezone related bug of git · Dongsheng Song <hidden>
- 2021-10-31 · Re: timezone related bug of git · Jeff King <hidden>
Jeff King [off-list ref] writes:
I won't be at all surprised if it has funny corner cases. Our tm_to_time_t() is pretty basic and hacky. We can't use mktime() because it only handles the current system timezone. OTOH, I think the tz_offset we're undoing here originally came from comparing mktime() versus tm_to_time_t() via local_time_tzoffset(), so it could be cancelling out any bugs exactly. :) So maybe the code below is sufficient, but we'd probably at least want some tests on top. Maybe something somebody interested would like to pick up and run with?
It would be very hard to write a code that does not work correctly on a timestamp created in the same zone in the same season. It is easy to get the direction of the offset wrong and not notice with such a test, but with another test to show a timestamp from a zone in a different zone (or across season boundary in an area where daylight saving time is s thing), such an error can easily be caught.
quoted hunk
---diff --git a/cache.h b/cache.h index eba12487b9..aa6f380d10 100644 --- a/cache.h +++ b/cache.h@@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *); timestamp_t approxidate_relative(const char *date); void parse_date_format(const char *format, struct date_mode *mode); int date_overflows(timestamp_t date); +time_t tm_to_time_t(const struct tm *tm); #define IDENT_STRICT 1 #define IDENT_NO_DATE 2diff --git a/date.c b/date.c index c55ea47e96..84bb4451c1 100644 --- a/date.c +++ b/date.c@@ -9,7 +9,7 @@ /* * This is like mktime, but without normalization of tm_wday and tm_yday. */ -static time_t tm_to_time_t(const struct tm *tm) +time_t tm_to_time_t(const struct tm *tm) { static const int mdays[] = { 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334diff --git a/strbuf.c b/strbuf.c index b22e981655..8b8b1900bc 100644 --- a/strbuf.c +++ b/strbuf.c@@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, strbuf_addstr(&munged_fmt, "%%"); fmt++; break; + case 's': + strbuf_addf(&munged_fmt, "%"PRItime, + tm_to_time_t(tm) - + 3600 * (tz_offset / 100) - + 60 * (tz_offset % 100)); + fmt++; + break;
In show_date(), we start from UNIX time and go to "struct tm" using either the system gmtime_r() (after adjusting the value with the tz offset of the original timestamp) or localtime_r() (when we are trying to show the value in our local timestamp), but this codepath needs to undo that. Our tm_to_time_t() indeed is basic but should work correctly on a broken down UTC. So the caller needs to further compensate for the tz offset. I have to wonder why gm_time_t() needs to use two separate codepaths for positive and negative tz_offset, while the new code here can get away without. Does it have something to do with the direction of truncation during division and modulo operation? Thanks.