Re: timezone related bug of git

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 1, 2021, 18:18 UTC
Message-ID: <xmqq1r3zd9k5.fsf@gitster.g>
In-Reply-To: <YX9nLJZXB3rOrMru@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

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

Show 41 quoted lines

> ---
> 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	       2
> diff --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, 334
> diff --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.