{"thread":{"id":"39080","subject":"Odd broken \"--date=now\" behavior in current git","startedAt":"2015-04-15T04:18:29Z","lastAt":"2015-04-15T17:23:51Z","messageCount":10,"participants":["Linus Torvalds","Junio C Hamano","Peter Krefting","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"259390","messageId":"CA+55aFxvcN8Dz-t6fi6etycg+AiyR0crXv5AcfCdv8ji-iNBpw@mail.gmail.com","threadId":"39080","inReplyTo":null,"subject":"Odd broken \"--date=now\" behavior in current git","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-04-15T04:18:29Z","receivedAt":"2015-04-15T04:18:29Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"I just noticed this because I had amended some merge commits with\n\n git commit --amend --date=now\n\nto update them, and that gets some funny broken timezones. I suspect\nit's some silly daylight savings time issue.\n\nLookie here, I can reproduce it trivially with current git (in the git\nrepo itself):\n\n [torvalds@i7 git]$ date; git commit -m Test --allow-empty --date=now\n Tue Apr 14 21:11:03 PDT 2015\n [master ec7733db5360] Test\n Date: Tue Apr 14 20:11:03 2015 -0800\n\nnotice how the commit date message shows something funny. It shows an\nhour earlier, but in -0800.\n\nAnd the resulting commit is broken:\n\n [torvalds@i7 git]$ git show --pretty=fuller\n commit ec7733db5360966434e03eab1a849e6d4227231c (HEAD -> master)\n Author: Linus Torvalds \n AuthorDate: Tue Apr 14 20:11:03 2015 -0800\n Commit: Linus Torvalds \n CommitDate: Tue Apr 14 21:11:03 2015 -0700\n\n Test\n\nnotice how the AuthorDate has that \"-0800\", but the CommitDate has \"-0700\".\n\nHmm.\n\nI can't be the only one seeing this? My guess is that there's a\nmissing initialization of tm.tm_isdst somewhere or whatever.\n\nThe above is with current git:\n\n [torvalds@i7 git]$ git version\n git version 2.4.0.rc2\n\nAnybody?\n\n Linus\n"},{"id":"259391","messageId":"xmqqzj6ayp3p.fsf@gitster.dls.corp.google.com","threadId":"39080","inReplyTo":"CA+55aFxvcN8Dz-t6fi6etycg+AiyR0crXv5AcfCdv8ji-iNBpw@mail.gmail.com","subject":"Re: Odd broken \"--date=now\" behavior in current git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T04:47:38Z","receivedAt":"2015-04-15T04:47:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> I just noticed this because I had amended some merge commits with\n>\n> git commit --amend --date=now\n>\n> to update them, and that gets some funny broken timezones. I suspect\n> it's some silly daylight savings time issue.\n>\n> Lookie here, I can reproduce it trivially with current git (in the git\n> repo itself):\n>\n> [torvalds@i7 git]$ date; git commit -m Test --allow-empty --date=now\n> Tue Apr 14 21:11:03 PDT 2015\n> [master ec7733db5360] Test\n> Date: Tue Apr 14 20:11:03 2015 -0800\n>\n> notice how the commit date message shows something funny. It shows an\n> hour earlier, but in -0800.\n>\n> And the resulting commit is broken:\n>\n> [torvalds@i7 git]$ git show --pretty=fuller\n> commit ec7733db5360966434e03eab1a849e6d4227231c (HEAD -> master)\n> Author: Linus Torvalds \n> AuthorDate: Tue Apr 14 20:11:03 2015 -0800\n> Commit: Linus Torvalds \n> CommitDate: Tue Apr 14 21:11:03 2015 -0700\n>\n> Test\n>\n> notice how the AuthorDate has that \"-0800\", but the CommitDate has \"-0700\".\n>\n> Hmm.\n>\n> I can't be the only one seeing this? My guess is that there's a\n> missing initialization of tm.tm_isdst somewhere or whatever.\n>\n> The above is with current git:\n>\n> [torvalds@i7 git]$ git version\n> git version 2.4.0.rc2\n\nWith a quick check, the symptom exists at least at v2.1.4. v2.0.x\nseries does not seem to have --date=now support but since there is\nno change to date.c between v2.0.0 to v2.1.4, older approxidate may\nbe equally broken.\n\nWill dig tomorrow, if nobody beats me to it, that is.\n\nThanks.\n"},{"id":"259393","messageId":"alpine.DEB.2.00.1504150803370.8606@ds9.cixit.se","threadId":"39080","inReplyTo":"CA+55aFxvcN8Dz-t6fi6etycg+AiyR0crXv5AcfCdv8ji-iNBpw@mail.gmail.com","subject":"Re: Odd broken \"--date=now\" behavior in current git","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2015-04-15T07:07:35Z","receivedAt":"2015-04-15T07:07:35Z","isPatch":false,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Linus Torvalds:\n\n> I can't be the only one seeing this? My guess is that there's a \n> missing initialization of tm.tm_isdst somewhere or whatever.\n\nI can confirm it if I enable DST on my machine (I usually run my \nmachines on CET all-year, to avoid these kind of issues):\n\n $ echo $TZ\n Europe/Oslo\n $ git commit -m b\n [master dee7ec1] b\n 1 file changed, 1 insertion(+)\n create mode 100644 b.txt\n $ git show --pretty=fuller\n commit dee7ec1cda74a8abd7f26c60ee1e83f73bb31194 (HEAD -> master)\n Author: Peter Krefting \n AuthorDate: 2015-04-15 09:04:34 +0200\n Commit: Peter Krefting \n CommitDate: 2015-04-15 09:04:34 +0200\n [...]\n $ git commit --amend --date=now\n [...]\n $ git show --pretty=fuller\n commit b4561e5a077de7bbcaf9fc06350ea24407adcec0 (HEAD -> master)\n Author: Peter Krefting \n AuthorDate: 2015-04-15 08:05:04 +0100\n Commit: Peter Krefting \n CommitDate: 2015-04-15 09:05:04 +0200\n\nSo the datetime is correct, it's just the timezone that is wrong \n(08:05+0100 = 09:05+0200).\n\n $ git --version\n git version 2.4.0.rc1\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"259395","messageId":"20150415072223.GA1389@flurp.local","threadId":"39080","inReplyTo":"xmqqzj6ayp3p.fsf@gitster.dls.corp.google.com","subject":"Re: Odd broken \"--date=now\" behavior in current git","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-15T07:22:23Z","receivedAt":"2015-04-15T07:22:23Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 14, 2015 at 09:47:38PM -0700, Junio C Hamano wrote:\n> Linus Torvalds writes:\n> > Lookie here, I can reproduce it trivially with current git (in the git\n> > repo itself):\n> >\n> > [torvalds@i7 git]$ date; git commit -m Test --allow-empty --date=now\n> > Tue Apr 14 21:11:03 PDT 2015\n> > [master ec7733db5360] Test\n> > Date: Tue Apr 14 20:11:03 2015 -0800\n> >\n> > notice how the commit date message shows something funny. It shows an\n> > hour earlier, but in -0800.\n> >\n> > And the resulting commit is broken:\n> >\n> > [torvalds@i7 git]$ git show --pretty=fuller\n> > commit ec7733db5360966434e03eab1a849e6d4227231c (HEAD -> master)\n> > Author: Linus Torvalds \n> > AuthorDate: Tue Apr 14 20:11:03 2015 -0800\n> > Commit: Linus Torvalds \n> > CommitDate: Tue Apr 14 21:11:03 2015 -0700\n> >\n> > Test\n> >\n> > notice how the AuthorDate has that \"-0800\", but the CommitDate has \"-0700\".\n> \n> With a quick check, the symptom exists at least at v2.1.4. v2.0.x\n> series does not seem to have --date=now support but since there is\n> no change to date.c between v2.0.0 to v2.1.4, older approxidate may\n> be equally broken.\n> \n> Will dig tomorrow, if nobody beats me to it, that is.\n\nI'm not familiar with this code, but have been studying it since\nreading this email, and I think I know what's going on. The problem\nisn't with \"approxidate\", but rather with the date form \"@nseconds\"\nreturned by approxidate. builtin/commit.c calls fmt_ident() with the\n\"@nseconds\" date, which calls parse_date(), which finally calls\nparse_date_basic(), and therein lies the problem.\n\nparse_date_basic() does correctly set tm_isdst=-1, however, when it\nencounters a date of form \"@nseconds\", it invokes match_digit() which\ncalls gmtime_r() to fill in the 'tm' structure, and gmtime_r() sets\ntm_isdst=0 (since DST is definitely false at GMT).\n\nLater parse_date_basic() computes the offset from GMT by comparing\nthe values returned by tm_to_time_t() and mktime(). The existing 'tm'\nis passed to mktime() with the tm_isdst field already set to 0 by\ngmtime_r(), and mktime() respects that as a statement that DST is not\nin effect, rather than determining it dynamically.\n\nThe fix seems to be simply:\n\n---- >8 ----\ndiff --git a/date.c b/date.c\nindex 3eba2df..99ad2a0 100644\n--- a/date.c\n+++ b/date.c\n@@ -707,6 +707,7 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)\n \t/* mktime uses local timezone */\n \t*timestamp = tm_to_time_t(&tm);\n \tif (*offset == -1) {\n+\t\ttm.tm_isdst = -1;\n \t\ttime_t temp_time = mktime(&tm);\n \t\tif ((time_t)*timestamp > temp_time) {\n \t\t\t*offset = ((time_t)*timestamp - temp_time) / 60;\n---- >8 ----\n\nHowever, I'm still digesting the code, so I haven't fully convinced\nmyself that that's all that's needed. (It doesn't break any tests,\nand it does fix this issue.)\n"},{"id":"259410","messageId":"xmqqk2xdmp0e.fsf@gitster.dls.corp.google.com","threadId":"39080","inReplyTo":"20150415072223.GA1389@flurp.local","subject":"Re: Odd broken \"--date=now\" behavior in current git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T14:42:41Z","receivedAt":"2015-04-15T14:42:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Later parse_date_basic() computes the offset from GMT by comparing\n> the values returned by tm_to_time_t() and mktime(). The existing 'tm'\n> is passed to mktime() with the tm_isdst field already set to 0 by\n> gmtime_r(), and mktime() respects that as a statement that DST is not\n> in effect, rather than determining it dynamically.\n>\n> The fix seems to be simply:\n>\n> ---- >8 ----\n> diff --git a/date.c b/date.c\n> index 3eba2df..99ad2a0 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -707,6 +707,7 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)\n> \t/* mktime uses local timezone */\n> \t*timestamp = tm_to_time_t(&tm);\n> \tif (*offset == -1) {\n> +\t\ttm.tm_isdst = -1;\n> \t\ttime_t temp_time = mktime(&tm);\n> \t\tif ((time_t)*timestamp > temp_time) {\n> \t\t\t*offset = ((time_t)*timestamp - temp_time) / 60;\n> ---- >8 ----\n\nI briefly wondered if the caller of gmtime_r() in match_digit()\nshould be preserving the tm_isdst, though, as that codepath knows\nthat it is handling a bare number without GMT offset.\n\nBut resetting it to -1 here makes it even less error prone (we may\ngain other code that stomp on tm.tm_isdst before we get here, and\nhaving -1 in *offset is a sign that nobody saw GMT offset in the\ninput).\n\nI think I see a decl-after-statment, but other than that, this looks\nlike a good fix.\n\nThanks.\n"},{"id":"259416","messageId":"CA+55aFwbEnneEvUDhxGaWw=fSwY2WbtGy6Uc3aQzpiUY6GH1uQ@mail.gmail.com","threadId":"39080","inReplyTo":"20150415072223.GA1389@flurp.local","subject":"Re: Odd broken \"--date=now\" behavior in current git","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-04-15T16:20:36Z","receivedAt":"2015-04-15T16:20:36Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Apr 15, 2015 at 12:22 AM, Eric Sunshine wrote:\n>\n> The fix seems to be simply:\n\nYup, that seems to do it for me. I'm not sure how we get to\n\"match_digit()\" with the time string \"now\", though.\n\nSo your patch fixes things for me, but I think:\n\n - we should move the \"tm.tm_isdst = -1;\" up a bit and add a comment\n\n - I'd like to know why it affected the author date but not the\ncommitter date, and how it got to match_digit() with that date string\nthat didn't contain any digits.\n\nI'd spend the time on this myself, but I'm in the middle of the kernel\nmerge window, so..\n\nThanks,\n Linus\n"},{"id":"259417","messageId":"xmqqa8y9mkfa.fsf_-_@gitster.dls.corp.google.com","threadId":"39080","inReplyTo":"xmqqk2xdmp0e.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/2] parse_date_basic(): return early when given a bogus timestamp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T16:21:45Z","receivedAt":"2015-04-15T16:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When the input does not have GMT timezone offset, the code computes\nit by computing the local and GMT time for the given timestamp. But\nthere is no point doing so if the given timestamp is known to be a\nbogus one.\n\nSigned-off-by: Junio C Hamano \n---\n\n * A simple preliminary clean-up while we are in the vicinity. We\n may want to use time_t throughout the codepath and turn it into\n ulong at the very last, but that would be a separate topic.\n\n date.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 782de95..01fd73f 100644\n--- a/date.c\n+++ b/date.c\n@@ -696,6 +696,9 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)\n \n \t/* mktime uses local timezone */\n \t*timestamp = tm_to_time_t(&tm);\n+\tif (*timestamp == -1)\n+\t\treturn -1;\n+\n \tif (*offset == -1) {\n \t\ttime_t temp_time = mktime(&tm);\n \t\tif ((time_t)*timestamp > temp_time) {\n@@ -705,8 +708,6 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)\n \t\t}\n \t}\n \n-\tif (*timestamp == -1)\n-\t\treturn -1;\n \n \tif (!tm_gmt)\n \t\t*timestamp -= *offset * 60;\n-- \n2.4.0-rc2-165-g862640d\n"},{"id":"259418","messageId":"xmqq618xmka9.fsf_-_@gitster.dls.corp.google.com","threadId":"39080","inReplyTo":"xmqqk2xdmp0e.fsf@gitster.dls.corp.google.com","subject":"[PATCH 2/2] parse_date_basic(): let the system handle DST conversion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T16:24:46Z","receivedAt":"2015-04-15T16:24:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function parses the input to compute the broken-down time in\n\"struct tm\", and the GMT timezone offset. If the timezone offset\ndoes not exist in the input, the broken-down time is turned into the\nnumber of seconds since epoch both in the current timezone and in\nGMT and the offset is computed as their difference.\n\nHowever, we forgot to make sure tm.tm_isdst is set to -1 (i.e. let\nthe system figure out if DST is in effect in the current timezone\nwhen turning the broken-down time to the number of seconds since\nepoch); it is done so at the beginning of the function, but a call\nto match_digit() in the function can lead to a call to gmtime_r() to\nclobber the field.\n\nReported-by: Linus Torvalds \nDiagnosed-by: Eric Sunshine \nSigned-off-by: Junio C Hamano \n---\n date.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/date.c b/date.c\nindex 01fd73f..8ad6cef 100644\n--- a/date.c\n+++ b/date.c\n@@ -700,7 +700,11 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)\n \t\treturn -1;\n \n \tif (*offset == -1) {\n-\t\ttime_t temp_time = mktime(&tm);\n+\t\ttime_t temp_time;\n+\n+\t\t/* gmtime_r() in match_digit() may have clobbered it */\n+\t\ttm.tm_isdst = -1;\n+\t\ttemp_time = mktime(&tm);\n \t\tif ((time_t)*timestamp > temp_time) {\n \t\t\t*offset = ((time_t)*timestamp - temp_time) / 60;\n \t\t} else {\n-- \n2.4.0-rc2-165-g862640d\n"},{"id":"259428","messageId":"xmqqzj69l3uw.fsf@gitster.dls.corp.google.com","threadId":"39080","inReplyTo":"CA+55aFwbEnneEvUDhxGaWw=fSwY2WbtGy6Uc3aQzpiUY6GH1uQ@mail.gmail.com","subject":"Re: Odd broken \"--date=now\" behavior in current git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-15T17:04:55Z","receivedAt":"2015-04-15T17:04:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Wed, Apr 15, 2015 at 12:22 AM, Eric Sunshine wrote:\n>>\n>> The fix seems to be simply:\n>\n> Yup, that seems to do it for me. I'm not sure how we get to\n> \"match_digit()\" with the time string \"now\", though.\n\nThe --date= parameter is read by determine_author_info() and\nthen parse_force_date(), both in builtin/commit.c. This calls\nparse_date(), which first calls (and fails) parse_date_basic() when\nit is not a strict timestamp.\n\n\"now\", \"4.days.ago\", etc. are then passed to approxidate_careful()\nwhich in turn calls approxidate_str(). Note that the approxidate\nformats do not usually spell out the TZ offset. The result is\nturned into \"@\" by parse_force_date().\n\nThe string \"@\" without TZ offset is re-interpreted by\nparse_date() called by fmt_ident(); the last function prepares the\nauthor ident line \"Name \".\n\nThe --date= does not force committer information, which goes\nthrough the usual codepath of grabbing the \"current\" time.\n"},{"id":"259429","messageId":"CAPig+cSC8RZJ-+uP=ZExVH2ZyexfQmLjzdjoBA7yuWkdYE4EGQ@mail.gmail.com","threadId":"39080","inReplyTo":"xmqq618xmka9.fsf_-_@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] parse_date_basic(): let the system handle DST conversion","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-15T17:23:51Z","receivedAt":"2015-04-15T17:23:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 15, 2015 at 12:24 PM, Junio C Hamano wrote:\n> The function parses the input to compute the broken-down time in\n> \"struct tm\", and the GMT timezone offset. If the timezone offset\n> does not exist in the input, the broken-down time is turned into the\n> number of seconds since epoch both in the current timezone and in\n> GMT and the offset is computed as their difference.\n>\n> However, we forgot to make sure tm.tm_isdst is set to -1 (i.e. let\n> the system figure out if DST is in effect in the current timezone\n> when turning the broken-down time to the number of seconds since\n> epoch); it is done so at the beginning of the function, but a call\n> to match_digit() in the function can lead to a call to gmtime_r() to\n> clobber the field.\n\nThanks for composing the commit message and turning this into a proper patch.\n\n> Reported-by: Linus Torvalds \n> Diagnosed-by: Eric Sunshine \n> Signed-off-by: Junio C Hamano \n\nFor what it's worth:\n\nReviewed-by: Eric Sunshine \n\n> ---\n> date.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/date.c b/date.c\n> index 01fd73f..8ad6cef 100644\n> --- a/date.c\n> +++ b/date.c\n> @@ -700,7 +700,11 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)\n> return -1;\n>\n> if (*offset == -1) {\n> - time_t temp_time = mktime(&tm);\n> + time_t temp_time;\n> +\n> + /* gmtime_r() in match_digit() may have clobbered it */\n> + tm.tm_isdst = -1;\n> + temp_time = mktime(&tm);\n> if ((time_t)*timestamp > temp_time) {\n> *offset = ((time_t)*timestamp - temp_time) / 60;\n> } else {\n> --\n> 2.4.0-rc2-165-g862640d\n"}]}