{"thread":{"id":"56819","subject":"timezone related bug of git","startedAt":"2021-10-31T03:23:45Z","lastAt":"2021-11-04T02:11:09Z","messageCount":12,"participants":["Dongsheng Song","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"440186","messageId":"CAE8XmWpK0ubcTXOaxBKGKh1qU+73Rr181wMAM7KAAX_A5PEYOw@mail.gmail.com","threadId":"56819","inReplyTo":null,"subject":"timezone related bug of git","fromName":"Dongsheng Song","fromEmail":"dongsheng.song@gmail.com","sentAt":"2021-10-31T03:23:24Z","receivedAt":"2021-10-31T03:23:45Z","isPatch":false,"sender":{"key":"dongsheng.song@gmail.com","avatar":null},"body":"Hi，\n\n I found a timezone related bug in the git:\n\n1. git log 11990eba -1 --date=format:%s\n\ncommit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,\norigin/trunk, origin/HEAD)\nAuthor: rillig \nDate: 1635604878\n\n indent: move debugging functions to a separate section\n\n2. git cat-file -p 11990eba\n\ntree 5d62150f5e2bafd3db76641450ca5d902302a039\nparent 892557a74bd49983fac28366b772b53c9216ca73\nauthor rillig 1635633678 +0000\ncommitter rillig 1635633678 +0000\n\nindent: move debugging functions to a separate section\n\n3. conclusion\n\nThe unix time stored in git repository not same as the git log output,\nthen there must be a timezone offset bug:\n\n1635633678 - 1635604878 = 28800 = 8 hours (local timezone offset)\n\nBest regards,\nCauchy Song\n"},{"id":"440193","messageId":"YX5Zo9uV7qG73p6R@coredump.intra.peff.net","threadId":"56819","inReplyTo":"CAE8XmWpK0ubcTXOaxBKGKh1qU+73Rr181wMAM7KAAX_A5PEYOw@mail.gmail.com","subject":"Re: timezone related bug of git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-31T08:53:55Z","receivedAt":"2021-10-31T08:53:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 31, 2021 at 11:23:24AM +0800, Dongsheng Song wrote:\n\n> I found a timezone related bug in the git:\n> \n> 1. git log 11990eba -1 --date=format:%s\n> \n> commit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,\n> origin/trunk, origin/HEAD)\n> Author: rillig \n> Date: 1635604878\n> \n> indent: move debugging functions to a separate section\n> \n> 2. git cat-file -p 11990eba\n> \n> tree 5d62150f5e2bafd3db76641450ca5d902302a039\n> parent 892557a74bd49983fac28366b772b53c9216ca73\n> author rillig 1635633678 +0000\n> committer rillig 1635633678 +0000\n> \n> indent: move debugging functions to a separate section\n> \n> 3. conclusion\n> \n> The unix time stored in git repository not same as the git log output,\n> then there must be a timezone offset bug:\n> \n> 1635633678 - 1635604878 = 28800 = 8 hours (local timezone offset)\n\nThe short answer is: don't do that. Use --date=unix instead.\n\nThe longer one is:\n\nThe problem is that the strftime() \"%s\" specifier is a bit broken.\nThat function (which is what is interpreting your format) takes a\nbroken-down \"struct tm\", which can only be converted back to an epoch\ntime if you know which time zone it's in.\n\nBut we have no way to tell the function that; the standard indicates\nthat it always assumes the local system timezone, and there's no\nprovision at all for formatting times in other zones (which is what we\nusually try to do, showing the date in the author's zone). There's no\nfield in the \"struct tm\" to carry any zone information[1].\n\nEven when you're in the same timezone, there's a similar problem with\nthe is_dst field. There's some discussion in [2], including the\npossibility of intercepting \"%s\" and handling it ourselves, like we do\nfor \"%z\". I don't think anybody has cared enough to work on it.\n\n-Peff\n\n[1] Some implementations (like glibc) actually _do_ carry this\n information in private fields of \"struct tm\". But we can't rely on\n it, and even where it's available, it's confusing (e.g., mktime()\n ignores it!). If you're a real masochist, you can read all of:\n\n https://lore.kernel.org/git/22824.29946.305300.380299@a1i15.kph.uni-mainz.de/\n\n[2] This is a similar bug report from 2020:\n\n https://lore.kernel.org/git/CAGqZTUu2U6FFXGTXihC64O0gB5Bz_Z3MbD750kMoJWMciAGH6w@mail.gmail.com/\n"},{"id":"440195","messageId":"CAE8XmWqexT89v0R+iVcjOHF+WsF1caMu+toY_gyNmJ6BU_L=ZQ@mail.gmail.com","threadId":"56819","inReplyTo":"YX5Zo9uV7qG73p6R@coredump.intra.peff.net","subject":"Re: timezone related bug of git","fromName":"Dongsheng Song","fromEmail":"dongsheng.song@gmail.com","sentAt":"2021-10-31T13:18:27Z","receivedAt":"2021-10-31T13:18:55Z","isPatch":false,"sender":{"key":"dongsheng.song@gmail.com","avatar":null},"body":"Thank you for the clarification, it's really a disappointing answer.\n\nPerhaps the manual needs to be clearer about this limitation.\n\nOn Sun, Oct 31, 2021 at 4:53 PM Jeff King wrote:\n>\n> On Sun, Oct 31, 2021 at 11:23:24AM +0800, Dongsheng Song wrote:\n>\n> > I found a timezone related bug in the git:\n> >\n> > 1. git log 11990eba -1 --date=format:%s\n> >\n> > commit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,\n> > origin/trunk, origin/HEAD)\n> > Author: rillig \n> > Date: 1635604878\n> >\n> > indent: move debugging functions to a separate section\n> >\n> > 2. git cat-file -p 11990eba\n> >\n> > tree 5d62150f5e2bafd3db76641450ca5d902302a039\n> > parent 892557a74bd49983fac28366b772b53c9216ca73\n> > author rillig 1635633678 +0000\n> > committer rillig 1635633678 +0000\n> >\n> > indent: move debugging functions to a separate section\n> >\n> > 3. conclusion\n> >\n> > The unix time stored in git repository not same as the git log output,\n> > then there must be a timezone offset bug:\n> >\n> > 1635633678 - 1635604878 = 28800 = 8 hours (local timezone offset)\n>\n> The short answer is: don't do that. Use --date=unix instead.\n>\n> The longer one is:\n>\n> The problem is that the strftime() \"%s\" specifier is a bit broken.\n> That function (which is what is interpreting your format) takes a\n> broken-down \"struct tm\", which can only be converted back to an epoch\n> time if you know which time zone it's in.\n>\n> But we have no way to tell the function that; the standard indicates\n> that it always assumes the local system timezone, and there's no\n> provision at all for formatting times in other zones (which is what we\n> usually try to do, showing the date in the author's zone). There's no\n> field in the \"struct tm\" to carry any zone information[1].\n>\n> Even when you're in the same timezone, there's a similar problem with\n> the is_dst field. There's some discussion in [2], including the\n> possibility of intercepting \"%s\" and handling it ourselves, like we do\n> for \"%z\". I don't think anybody has cared enough to work on it.\n>\n> -Peff\n>\n> [1] Some implementations (like glibc) actually _do_ carry this\n> information in private fields of \"struct tm\". But we can't rely on\n> it, and even where it's available, it's confusing (e.g., mktime()\n> ignores it!). If you're a real masochist, you can read all of:\n>\n> https://lore.kernel.org/git/22824.29946.305300.380299@a1i15.kph.uni-mainz.de/\n>\n> [2] This is a similar bug report from 2020:\n>\n> https://lore.kernel.org/git/CAGqZTUu2U6FFXGTXihC64O0gB5Bz_Z3MbD750kMoJWMciAGH6w@mail.gmail.com/\n"},{"id":"440203","messageId":"xmqqcznldobz.fsf@gitster.g","threadId":"56819","inReplyTo":"CAE8XmWqexT89v0R+iVcjOHF+WsF1caMu+toY_gyNmJ6BU_L=ZQ@mail.gmail.com","subject":"Re: timezone related bug of git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-31T18:46:40Z","receivedAt":"2021-10-31T18:46:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dongsheng Song writes:\n\n> Thank you for the clarification, it's really a disappointing answer.\n\nThe situation may be disappointing, but I found the answer eminently\nclear and helpful.\n\n> Perhaps the manual needs to be clearer about this limitation.\n\nSounds like we have a volunteer ;-)?\n\nThanks for a report and discussion.\n"},{"id":"440208","messageId":"YX9nLJZXB3rOrMru@coredump.intra.peff.net","threadId":"56819","inReplyTo":"xmqqcznldobz.fsf@gitster.g","subject":"Re: timezone related bug of git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-01T04:03:56Z","receivedAt":"2021-11-01T04:04:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 31, 2021 at 11:46:40AM -0700, Junio C Hamano wrote:\n\n> Dongsheng Song writes:\n> \n> > Thank you for the clarification, it's really a disappointing answer.\n> \n> The situation may be disappointing, but I found the answer eminently\n> clear and helpful.\n\nThe most disappointing thing IMHO is the lousy state of system-level\ndate routines. ;)\n\nI have some patches working towards allowing timestamps before 1970, and\nthe system routines are quite unreliable (both in giving insufficient\nportable interfaces, but also just doing weird things with negative\nvalues).\n\n> > Perhaps the manual needs to be clearer about this limitation.\n> \n> Sounds like we have a volunteer ;-)?\n\nYeah, I'd be happy if somebody wanted to note this in the manual. But if\nanybody wants to pursue manually intercepting %s, I think the patch\nbelow might point them in the right direction.\n\nI won't be at all surprised if it has funny corner cases. Our\ntm_to_time_t() is pretty basic and hacky. We can't use mktime() because\nit only handles the current system timezone. OTOH, I think the tz_offset\nwe're undoing here originally came from comparing mktime() versus\ntm_to_time_t() via local_time_tzoffset(), so it could be cancelling out\nany bugs exactly. :)\n\nSo maybe the code below is sufficient, but we'd probably at least want\nsome tests on top. Maybe something somebody interested would like to\npick up and run with?\n\n---\ndiff --git a/cache.h b/cache.h\nindex eba12487b9..aa6f380d10 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);\n timestamp_t approxidate_relative(const char *date);\n void parse_date_format(const char *format, struct date_mode *mode);\n int date_overflows(timestamp_t date);\n+time_t tm_to_time_t(const struct tm *tm);\n \n #define IDENT_STRICT\t 1\n #define IDENT_NO_DATE\t 2\ndiff --git a/date.c b/date.c\nindex c55ea47e96..84bb4451c1 100644\n--- a/date.c\n+++ b/date.c\n@@ -9,7 +9,7 @@\n /*\n * This is like mktime, but without normalization of tm_wday and tm_yday.\n */\n-static time_t tm_to_time_t(const struct tm *tm)\n+time_t tm_to_time_t(const struct tm *tm)\n {\n \tstatic const int mdays[] = {\n \t 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334\ndiff --git a/strbuf.c b/strbuf.c\nindex b22e981655..8b8b1900bc 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n \t\t\tstrbuf_addstr(&munged_fmt, \"%%\");\n \t\t\tfmt++;\n \t\t\tbreak;\n+\t\tcase 's':\n+\t\t\tstrbuf_addf(&munged_fmt, \"%\"PRItime,\n+\t\t\t\t tm_to_time_t(tm) -\n+\t\t\t\t 3600 * (tz_offset / 100) -\n+\t\t\t\t 60 * (tz_offset % 100));\n+\t\t\tfmt++;\n+\t\t\tbreak;\n \t\tcase 'z':\n \t\t\tstrbuf_addf(&munged_fmt, \"%+05d\", tz_offset);\n \t\t\tfmt++;\n"},{"id":"440220","messageId":"CAE8XmWoHpESNK9QJ3d0u0x+Q-OfJEu7117rmV9YZpQjv7JF5ZQ@mail.gmail.com","threadId":"56819","inReplyTo":"YX9nLJZXB3rOrMru@coredump.intra.peff.net","subject":"Re: timezone related bug of git","fromName":"Dongsheng Song","fromEmail":"dongsheng.song@gmail.com","sentAt":"2021-11-01T14:31:18Z","receivedAt":"2021-11-01T14:31:39Z","isPatch":false,"sender":{"key":"dongsheng.song@gmail.com","avatar":null},"body":"Hi Jeff,\n\nYour patch looks good to me. Here is my test result:\n\n$ patch -p1 < x.patch\npatching file cache.h\npatching file date.c\npatching file strbuf.c\n\n$ make\n\n$ ~/git-testing/bin/git --version\n\n$ ~/git-testing/bin/git log 11990eba -1 --date=format:%s\ncommit 11990eba0be50d1ad0655ede4062b7130326c41f (HEAD -> trunk,\norigin/trunk, origin/HEAD)\nAuthor: rillig \nDate: 1635633678\n\n indent: move debugging functions to a separate section\n\n\n$ ~/git-testing/bin/git cat-file -p 11990eba\ntree 5d62150f5e2bafd3db76641450ca5d902302a039\nparent 892557a74bd49983fac28366b772b53c9216ca73\nauthor rillig 1635633678 +0000\ncommitter rillig 1635633678 +0000\n\nindent: move debugging functions to a separate section\n\n\nPS:\nThanks Junio for clarification, Jeff's explanation is very clear,\nI'm just frustrated with the limitations of the system, I greatly\nappreciate Jeff's work.\n\nBest regards,\nDongsheng Song\n\nOn Mon, Nov 1, 2021 at 12:03 PM Jeff King wrote:\n>\n> On Sun, Oct 31, 2021 at 11:46:40AM -0700, Junio C Hamano wrote:\n>\n> > Dongsheng Song writes:\n> >\n> > > Thank you for the clarification, it's really a disappointing answer.\n> >\n> > The situation may be disappointing, but I found the answer eminently\n> > clear and helpful.\n>\n> The most disappointing thing IMHO is the lousy state of system-level\n> date routines. ;)\n>\n> I have some patches working towards allowing timestamps before 1970, and\n> the system routines are quite unreliable (both in giving insufficient\n> portable interfaces, but also just doing weird things with negative\n> values).\n>\n> > > Perhaps the manual needs to be clearer about this limitation.\n> >\n> > Sounds like we have a volunteer ;-)?\n>\n> Yeah, I'd be happy if somebody wanted to note this in the manual. But if\n> anybody wants to pursue manually intercepting %s, I think the patch\n> below might point them in the right direction.\n>\n> I won't be at all surprised if it has funny corner cases. Our\n> tm_to_time_t() is pretty basic and hacky. We can't use mktime() because\n> it only handles the current system timezone. OTOH, I think the tz_offset\n> we're undoing here originally came from comparing mktime() versus\n> tm_to_time_t() via local_time_tzoffset(), so it could be cancelling out\n> any bugs exactly. :)\n>\n> So maybe the code below is sufficient, but we'd probably at least want\n> some tests on top. Maybe something somebody interested would like to\n> pick up and run with?\n>\n> ---\n> diff --git a/cache.h b/cache.h\n> index eba12487b9..aa6f380d10 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);\n> timestamp_t approxidate_relative(const char *date);\n> void parse_date_format(const char *format, struct date_mode *mode);\n> int date_overflows(timestamp_t date);\n> +time_t tm_to_time_t(const struct tm *tm);\n>\n> #define IDENT_STRICT 1\n> #define IDENT_NO_DATE 2\n> diff --git a/date.c b/date.c\n> index c55ea47e96..84bb4451c1 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -9,7 +9,7 @@\n> /*\n> * This is like mktime, but without normalization of tm_wday and tm_yday.\n> */\n> -static time_t tm_to_time_t(const struct tm *tm)\n> +time_t tm_to_time_t(const struct tm *tm)\n> {\n> static const int mdays[] = {\n> 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334\n> diff --git a/strbuf.c b/strbuf.c\n> index b22e981655..8b8b1900bc 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n> strbuf_addstr(&munged_fmt, \"%%\");\n> fmt++;\n> break;\n> + case 's':\n> + strbuf_addf(&munged_fmt, \"%\"PRItime,\n> + tm_to_time_t(tm) -\n> + 3600 * (tz_offset / 100) -\n> + 60 * (tz_offset % 100));\n> + fmt++;\n> + break;\n> case 'z':\n> strbuf_addf(&munged_fmt, \"%+05d\", tz_offset);\n> fmt++;\n"},{"id":"440224","messageId":"xmqq1r3zd9k5.fsf@gitster.g","threadId":"56819","inReplyTo":"YX9nLJZXB3rOrMru@coredump.intra.peff.net","subject":"Re: timezone related bug of git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-01T18:18:02Z","receivedAt":"2021-11-01T18:18:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I won't be at all surprised if it has funny corner cases. Our\n> tm_to_time_t() is pretty basic and hacky. We can't use mktime() because\n> it only handles the current system timezone. OTOH, I think the tz_offset\n> we're undoing here originally came from comparing mktime() versus\n> tm_to_time_t() via local_time_tzoffset(), so it could be cancelling out\n> any bugs exactly. :)\n>\n> So maybe the code below is sufficient, but we'd probably at least want\n> some tests on top. Maybe something somebody interested would like to\n> pick up and run with?\n\nIt would be very hard to write a code that does not work correctly\non a timestamp created in the same zone in the same season. It is\neasy to get the direction of the offset wrong and not notice with\nsuch a test, but with another test to show a timestamp from a zone\nin a different zone (or across season boundary in an area where\ndaylight saving time is s thing), such an error can easily be\ncaught.\n\n> ---\n> diff --git a/cache.h b/cache.h\n> index eba12487b9..aa6f380d10 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);\n> timestamp_t approxidate_relative(const char *date);\n> void parse_date_format(const char *format, struct date_mode *mode);\n> int date_overflows(timestamp_t date);\n> +time_t tm_to_time_t(const struct tm *tm);\n> \n> #define IDENT_STRICT\t 1\n> #define IDENT_NO_DATE\t 2\n> diff --git a/date.c b/date.c\n> index c55ea47e96..84bb4451c1 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -9,7 +9,7 @@\n> /*\n> * This is like mktime, but without normalization of tm_wday and tm_yday.\n> */\n> -static time_t tm_to_time_t(const struct tm *tm)\n> +time_t tm_to_time_t(const struct tm *tm)\n> {\n> \tstatic const int mdays[] = {\n> \t 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334\n> diff --git a/strbuf.c b/strbuf.c\n> index b22e981655..8b8b1900bc 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n> \t\t\tstrbuf_addstr(&munged_fmt, \"%%\");\n> \t\t\tfmt++;\n> \t\t\tbreak;\n> +\t\tcase 's':\n> +\t\t\tstrbuf_addf(&munged_fmt, \"%\"PRItime,\n> +\t\t\t\t tm_to_time_t(tm) -\n> +\t\t\t\t 3600 * (tz_offset / 100) -\n> +\t\t\t\t 60 * (tz_offset % 100));\n> +\t\t\tfmt++;\n> +\t\t\tbreak;\n\nIn show_date(), we start from UNIX time and go to \"struct tm\" using\neither the system gmtime_r() (after adjusting the value with the tz\noffset of the original timestamp) or localtime_r() (when we are\ntrying to show the value in our local timestamp), but this codepath\nneeds to undo that. Our tm_to_time_t() indeed is basic but should\nwork correctly on a broken down UTC. So the caller needs to further\ncompensate for the tz offset.\n\nI have to wonder why gm_time_t() needs to use two separate codepaths\nfor positive and negative tz_offset, while the new code here can get\naway without. Does it have something to do with the direction of\ntruncation during division and modulo operation?\n\nThanks.\n"},{"id":"440286","messageId":"YYCXu/lpp0jtaIMk@coredump.intra.peff.net","threadId":"56819","inReplyTo":"xmqq1r3zd9k5.fsf@gitster.g","subject":"Re: timezone related bug of git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-02T01:43:23Z","receivedAt":"2021-11-02T01:43:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 01, 2021 at 11:18:02AM -0700, Junio C Hamano wrote:\n\n> I have to wonder why gm_time_t() needs to use two separate codepaths\n> for positive and negative tz_offset, while the new code here can get\n> away without. Does it have something to do with the direction of\n> truncation during division and modulo operation?\n\nHmm. Unless I am missing something, this part of gm_time_t() is simply\nover-complicating things:\n\n minutes = tz < 0 ? -tz : tz;\n minutes = (minutes / 100)*60 + (minutes % 100);\n minutes = tz < 0 ? -minutes : minutes;\n\nWe switch to doing the computation in absolute-value units, but then\nrestore the sign. But just:\n\n minutes = (tz / 100) * 60 + (tz % 100);\n\nis equivalent and shorter. If tz is negative, then both terms will be\nnegative, which is what you want (they sum to a larger absolute-value\nnegative number). This comes from f80cd783c6 (date.c: add \"show_date()\"\nfunction., 2005-05-06), so I don't see any sign that there was specific\nthought given to some obscure handling. And indeed later fixes like\nfbab835c03 ([PATCH] fix show_date() for positive timezones, 2005-05-18)\nimply to me that the original was just confused.\n\nLater we do:\n\n if (minutes > 0) {\n if (unsigned_add_overflows(time, minutes * 60))\n die(\"Timestamp+tz too large: %\"PRItime\" +%04d\",\n time, tz);\n } else if (time < -minutes * 60)\n die(\"Timestamp before Unix epoch: %\"PRItime\" %04d\", time, tz);\n\nAnd that does need separate paths for the overflow check, since we're\nchecking different boundaries. I suspect for the strftime() code that we\nwouldn't need similar checks, because the earlier ones would have caught\nany problems (i.e., we would not get as far as having a \"struct tm\" that\nrepresented something outside the range of our time_t).\n\n-Peff\n"},{"id":"440291","messageId":"YYEihoLbEGi44dDb@coredump.intra.peff.net","threadId":"56819","inReplyTo":"xmqq1r3zd9k5.fsf@gitster.g","subject":"[PATCH] strbuf_addftime(): handle \"%s\" manually","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-02T11:35:34Z","receivedAt":"2021-11-02T11:35:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 01, 2021 at 11:18:02AM -0700, Junio C Hamano wrote:\n\n> > diff --git a/strbuf.c b/strbuf.c\n> > index b22e981655..8b8b1900bc 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -1019,6 +1019,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n> > \t\t\tstrbuf_addstr(&munged_fmt, \"%%\");\n> > \t\t\tfmt++;\n> > \t\t\tbreak;\n> > +\t\tcase 's':\n> > +\t\t\tstrbuf_addf(&munged_fmt, \"%\"PRItime,\n> > +\t\t\t\t tm_to_time_t(tm) -\n> > +\t\t\t\t 3600 * (tz_offset / 100) -\n> > +\t\t\t\t 60 * (tz_offset % 100));\n> > +\t\t\tfmt++;\n> > +\t\t\tbreak;\n> \n> In show_date(), we start from UNIX time and go to \"struct tm\" using\n> either the system gmtime_r() (after adjusting the value with the tz\n> offset of the original timestamp) or localtime_r() (when we are\n> trying to show the value in our local timestamp), but this codepath\n> needs to undo that. Our tm_to_time_t() indeed is basic but should\n> work correctly on a broken down UTC. So the caller needs to further\n> compensate for the tz offset.\n\nThis made me wonder how things interact with format-local. There's some\nsubtlety, but I think all is well.\n\nI hadn't planned to work further on this, but now having thought about\nit some more, it seemed worth capturing that and putting the finishing\ntouches on a real commit.\n\n-- >8 --\nSubject: [PATCH] strbuf_addftime(): handle \"%s\" manually\n\nThe strftime() function has a non-standard \"%s\" extension, which prints\nthe number of seconds since the epoch. But the \"struct tm\" we get has\nalready been adjusted for a particular time zone; going back to an epoch\ntime requires knowing that zone offset. Since strftime() doesn't take\nsuch an argument, round-tripping to a \"struct tm\" and back to the \"%s\"\nformat may produce the wrong value (off by tz_offset seconds).\n\nSince we're already passing in the zone offset courtesy of c3fbf81a85\n(strbuf: let strbuf_addftime handle %z and %Z itself, 2017-06-15), we\ncan use that same value to adjust our epoch seconds accordingly.\n\nNote that the description above makes it sound like strftime()'s \"%s\" is\nuseless (and really, the issue is shared by mktime(), which is what\nstrftime() would use under the hood). But it gets the two cases for\nwhich it's designed correct:\n\n - the result of gmtime() will have a zero offset, so no adjustment is\n necessary\n\n - the result of localtime() will be offset by the local zone offset,\n and mktime() and strftime() are defined to assume this offset when\n converting back (there's actually some magic here; some\n implementations record this in the \"struct tm\", but we can't\n portably access or manipulate it. But they somehow \"know\" whether a\n \"struct tm\" is from gmtime() or localtime()).\n\nThis latter point means that \"format-local:%s\" actually works correctly\nalready, because in that case we rely on the system routines due to\n6eced3ec5e (date: use localtime() for \"-local\" time formats,\n2017-06-15). Our problem comes when trying to show times in the author's\nzone, as the system routines provide no mechanism for converting in\nnon-local zones. So in those cases we have a \"struct tm\" that came from\ngmtime(), but has been manipulated according to our offset.\n\nThe tests cover the broken round-trip by formatting \"%s\" for a time in a\nnon-system timezone. We use the made-up \"+1234\" here, which has two\nadvantages. One, we know it won't ever be the real system zone (and so\nwe're actually testing a case that would break). And two, since it has a\nminute component, we're testing the full decoding of the +HHMM zone into\na number of seconds. Likewise, we test the \"-1234\" variant to make sure\nthere aren't any sign mistakes.\n\nThere's one final test, which covers \"format-local:%s\". As noted, this\nalready passes, but it's important to check that we didn't regress this\ncase. In particular, the caller in show_date() is relying on localtime()\nto have done the zone adjustment, independent of any tz_offset we\ncompute ourselves. These should match up, since our local_tzoffset() is\nlikewise built around localtime(). But it would be easy for a caller to\nforget to pass in a correct tz_offset to strbuf_addftime(). Fortunately\nshow_date() does this correctly (it has to because of the existing\nhandling of %z), and the test continues to pass. So this one is just\nfuture-proofing against a change in our assumptions.\n\nSigned-off-by: Jeff King \n---\n cache.h | 1 +\n date.c | 2 +-\n strbuf.c | 14 +++++++++++++-\n t/t0006-date.sh | 4 ++++\n 4 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex eba12487b9..aa6f380d10 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1588,6 +1588,7 @@ timestamp_t approxidate_careful(const char *, int *);\n timestamp_t approxidate_relative(const char *date);\n void parse_date_format(const char *format, struct date_mode *mode);\n int date_overflows(timestamp_t date);\n+time_t tm_to_time_t(const struct tm *tm);\n \n #define IDENT_STRICT\t 1\n #define IDENT_NO_DATE\t 2\ndiff --git a/date.c b/date.c\nindex c55ea47e96..84bb4451c1 100644\n--- a/date.c\n+++ b/date.c\n@@ -9,7 +9,7 @@\n /*\n * This is like mktime, but without normalization of tm_wday and tm_yday.\n */\n-static time_t tm_to_time_t(const struct tm *tm)\n+time_t tm_to_time_t(const struct tm *tm)\n {\n \tstatic const int mdays[] = {\n \t 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334\ndiff --git a/strbuf.c b/strbuf.c\nindex b22e981655..a569b99ab9 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1006,7 +1006,12 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n \n \t/*\n \t * There is no portable way to pass timezone information to\n-\t * strftime, so we handle %z and %Z here.\n+\t * strftime, so we handle %z and %Z here. Likewise '%s', because\n+\t * going back to an epoch time requires knowing the zone.\n+\t *\n+\t * Note that tz_offset is in the \"[-+]HHMM\" decimal form; this is what\n+\t * we want for %z, but the computation for %s has to convert to number\n+\t * of seconds.\n \t */\n \tfor (;;) {\n \t\tconst char *percent = strchrnul(fmt, '%');\n@@ -1019,6 +1024,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n \t\t\tstrbuf_addstr(&munged_fmt, \"%%\");\n \t\t\tfmt++;\n \t\t\tbreak;\n+\t\tcase 's':\n+\t\t\tstrbuf_addf(&munged_fmt, \"%\"PRItime,\n+\t\t\t\t tm_to_time_t(tm) -\n+\t\t\t\t 3600 * (tz_offset / 100) -\n+\t\t\t\t 60 * (tz_offset % 100));\n+\t\t\tfmt++;\n+\t\t\tbreak;\n \t\tcase 'z':\n \t\t\tstrbuf_addf(&munged_fmt, \"%+05d\", tz_offset);\n \t\t\tfmt++;\ndiff --git a/t/t0006-date.sh b/t/t0006-date.sh\nindex 6b757d7169..794186961e 100755\n--- a/t/t0006-date.sh\n+++ b/t/t0006-date.sh\n@@ -63,6 +63,10 @@ check_show 'format-local:%%z' \"$TIME\" '%z'\n check_show 'format:%Y-%m-%d %H:%M:%S' \"$TIME\" '2016-06-15 16:13:20'\n check_show 'format-local:%Y-%m-%d %H:%M:%S' \"$TIME\" '2016-06-15 09:13:20' '' EST5\n \n+check_show 'format:%s' '123456789 +1234' 123456789\n+check_show 'format:%s' '123456789 -1234' 123456789\n+check_show 'format-local:%s' '123456789 -1234' 123456789\n+\n # arbitrary time absurdly far in the future\n FUTURE=\"5758122296 -0400\"\n check_show iso \"$FUTURE\" \"2152-06-19 18:24:56 -0400\" TIME_IS_64BIT,TIME_T_IS_64BIT\n-- \n2.34.0.rc0.612.g98bfd90890\n\n"},{"id":"440323","messageId":"YYFcj0Y9ApBU9j+V@coredump.intra.peff.net","threadId":"56819","inReplyTo":"YYEihoLbEGi44dDb@coredump.intra.peff.net","subject":"Re: [PATCH] strbuf_addftime(): handle \"%s\" manually","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-02T15:43:11Z","receivedAt":"2021-11-02T15:43:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 02, 2021 at 07:35:35AM -0400, Jeff King wrote:\n\n> @@ -1019,6 +1024,13 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n> \t\t\tstrbuf_addstr(&munged_fmt, \"%%\");\n> \t\t\tfmt++;\n> \t\t\tbreak;\n> +\t\tcase 's':\n> +\t\t\tstrbuf_addf(&munged_fmt, \"%\"PRItime,\n> +\t\t\t\t tm_to_time_t(tm) -\n> +\t\t\t\t 3600 * (tz_offset / 100) -\n> +\t\t\t\t 60 * (tz_offset % 100));\n> +\t\t\tfmt++;\n> +\t\t\tbreak;\n\nLooks like we may need something like this squashed in:\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 33015b33df..995394f38e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1026,7 +1026,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,\n \t\t\tbreak;\n \t\tcase 's':\n \t\t\tstrbuf_addf(&munged_fmt, \"%\"PRItime,\n-\t\t\t\t tm_to_time_t(tm) -\n+\t\t\t\t (timestamp_t)tm_to_time_t(tm) -\n \t\t\t\t 3600 * (tz_offset / 100) -\n \t\t\t\t 60 * (tz_offset % 100));\n \t\t\tfmt++;\n\nbecause tm_to_time_t() returns an actual time_t, which will vary in\nsize. The 32-bit CI job complains:\n\n strbuf.c:1028:29: error: format '%lld' expects argument of type 'long long int', but argument 3 has type 'long int' [-Werror=format=]\n\n-Peff\n"},{"id":"440402","messageId":"xmqqtugt56i7.fsf@gitster.g","threadId":"56819","inReplyTo":"YYEihoLbEGi44dDb@coredump.intra.peff.net","subject":"Re: [PATCH] strbuf_addftime(): handle \"%s\" manually","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-03T20:28:00Z","receivedAt":"2021-11-03T20:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think this also needs squashing in?\n\n Documentation/rev-list-options.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/Documentation/rev-list-options.txt w/Documentation/rev-list-options.txt\nindex 24569b06d1..43a86fa562 100644\n--- c/Documentation/rev-list-options.txt\n+++ w/Documentation/rev-list-options.txt\n@@ -1047,7 +1047,7 @@ omitted.\n has no effect.\n \n `--date=format:...` feeds the format `...` to your system `strftime`,\n-except for %z and %Z, which are handled internally.\n+except for %s, %z, and %Z, which are handled internally.\n Use `--date=format:%c` to show the date in your system locale's\n preferred format. See the `strftime` manual for a complete list of\n format placeholders. When using `-local`, the correct syntax is\n\n"},{"id":"440423","messageId":"YYNBOp4QDwG9ctHE@coredump.intra.peff.net","threadId":"56819","inReplyTo":"xmqqtugt56i7.fsf@gitster.g","subject":"Re: [PATCH] strbuf_addftime(): handle \"%s\" manually","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T02:11:06Z","receivedAt":"2021-11-04T02:11:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 03, 2021 at 01:28:00PM -0700, Junio C Hamano wrote:\n\n> I think this also needs squashing in?\n> \n> Documentation/rev-list-options.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git c/Documentation/rev-list-options.txt w/Documentation/rev-list-options.txt\n> index 24569b06d1..43a86fa562 100644\n> --- c/Documentation/rev-list-options.txt\n> +++ w/Documentation/rev-list-options.txt\n> @@ -1047,7 +1047,7 @@ omitted.\n> has no effect.\n> \n> `--date=format:...` feeds the format `...` to your system `strftime`,\n> -except for %z and %Z, which are handled internally.\n> +except for %s, %z, and %Z, which are handled internally.\n> Use `--date=format:%c` to show the date in your system locale's\n> preferred format. See the `strftime` manual for a complete list of\n> format placeholders. When using `-local`, the correct syntax is\n\nAh, thanks. I didn't even think to look in the documentation, because I\ndidn't imagine that we would expose these implementation details. But\nsince we do mention %z there, I think adding %s makes sense.\n\nBTW, I also noticed that stftime supports some locale modifiers. So\n\"%Es\" ends up printing the epoch seconds, but eludes our manual\nintervention (and so does the old, wrong thing). I'm fine with stopping\nhere, though. There's no reason to use %Es over %s (from what I gather,\nthe %E is about handling year eras for locales that support them, but\nthat's meaningless for an epoch time), and I'm not sure it is even a\nportable thing (glibc does not mention it in the manpage along with\nother %E values, but it does work; POSIX does not even define %s, so of\ncourse does not mention %Es).\n\n-Peff\n"}]}