From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Dongsheng Song <dongsheng.song@gmail.com>,
Git Mailing List <git@vger.kernel.org>
Subject: Re: timezone related bug of git
Date: Mon, 01 Nov 2021 11:18:02 -0700 [thread overview]
Message-ID: <xmqq1r3zd9k5.fsf@gitster.g> (raw)
In-Reply-To: <YX9nLJZXB3rOrMru@coredump.intra.peff.net> (Jeff King's message of "Mon, 1 Nov 2021 00:03:56 -0400")
Jeff King <peff@peff.net> 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.
> ---
> 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.
next prev parent reply other threads:[~2021-11-01 18:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-31 3:23 timezone related bug of git Dongsheng Song
2021-10-31 8:53 ` Jeff King
2021-10-31 13:18 ` Dongsheng Song
2021-10-31 18:46 ` Junio C Hamano
2021-11-01 4:03 ` Jeff King
2021-11-01 14:31 ` Dongsheng Song
2021-11-01 18:18 ` Junio C Hamano [this message]
2021-11-02 1:43 ` Jeff King
2021-11-02 11:35 ` [PATCH] strbuf_addftime(): handle "%s" manually Jeff King
2021-11-02 15:43 ` Jeff King
2021-11-03 20:28 ` Junio C Hamano
2021-11-04 2:11 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq1r3zd9k5.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=dongsheng.song@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).