* Odd broken "--date=now" behavior in current git @ 2015-04-15 4:18 Linus Torvalds 2015-04-15 4:47 ` Junio C Hamano 2015-04-15 7:07 ` Peter Krefting 0 siblings, 2 replies; 10+ messages in thread From: Linus Torvalds @ 2015-04-15 4:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List I just noticed this because I had amended some merge commits with git commit --amend --date=now to update them, and that gets some funny broken timezones. I suspect it's some silly daylight savings time issue. Lookie here, I can reproduce it trivially with current git (in the git repo itself): [torvalds@i7 git]$ date; git commit -m Test --allow-empty --date=now Tue Apr 14 21:11:03 PDT 2015 [master ec7733db5360] Test Date: Tue Apr 14 20:11:03 2015 -0800 notice how the commit date message shows something funny. It shows an hour earlier, but in -0800. And the resulting commit is broken: [torvalds@i7 git]$ git show --pretty=fuller commit ec7733db5360966434e03eab1a849e6d4227231c (HEAD -> master) Author: Linus Torvalds <torvalds@linux-foundation.org> AuthorDate: Tue Apr 14 20:11:03 2015 -0800 Commit: Linus Torvalds <torvalds@linux-foundation.org> CommitDate: Tue Apr 14 21:11:03 2015 -0700 Test notice how the AuthorDate has that "-0800", but the CommitDate has "-0700". Hmm. I can't be the only one seeing this? My guess is that there's a missing initialization of tm.tm_isdst somewhere or whatever. The above is with current git: [torvalds@i7 git]$ git version git version 2.4.0.rc2 Anybody? Linus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Odd broken "--date=now" behavior in current git 2015-04-15 4:18 Odd broken "--date=now" behavior in current git Linus Torvalds @ 2015-04-15 4:47 ` Junio C Hamano 2015-04-15 7:22 ` Eric Sunshine 2015-04-15 7:07 ` Peter Krefting 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2015-04-15 4:47 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List Linus Torvalds <torvalds@linux-foundation.org> writes: > I just noticed this because I had amended some merge commits with > > git commit --amend --date=now > > to update them, and that gets some funny broken timezones. I suspect > it's some silly daylight savings time issue. > > Lookie here, I can reproduce it trivially with current git (in the git > repo itself): > > [torvalds@i7 git]$ date; git commit -m Test --allow-empty --date=now > Tue Apr 14 21:11:03 PDT 2015 > [master ec7733db5360] Test > Date: Tue Apr 14 20:11:03 2015 -0800 > > notice how the commit date message shows something funny. It shows an > hour earlier, but in -0800. > > And the resulting commit is broken: > > [torvalds@i7 git]$ git show --pretty=fuller > commit ec7733db5360966434e03eab1a849e6d4227231c (HEAD -> master) > Author: Linus Torvalds <torvalds@linux-foundation.org> > AuthorDate: Tue Apr 14 20:11:03 2015 -0800 > Commit: Linus Torvalds <torvalds@linux-foundation.org> > CommitDate: Tue Apr 14 21:11:03 2015 -0700 > > Test > > notice how the AuthorDate has that "-0800", but the CommitDate has "-0700". > > Hmm. > > I can't be the only one seeing this? My guess is that there's a > missing initialization of tm.tm_isdst somewhere or whatever. > > The above is with current git: > > [torvalds@i7 git]$ git version > git version 2.4.0.rc2 With a quick check, the symptom exists at least at v2.1.4. v2.0.x series does not seem to have --date=now support but since there is no change to date.c between v2.0.0 to v2.1.4, older approxidate may be equally broken. Will dig tomorrow, if nobody beats me to it, that is. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Odd broken "--date=now" behavior in current git 2015-04-15 4:47 ` Junio C Hamano @ 2015-04-15 7:22 ` Eric Sunshine 2015-04-15 14:42 ` Junio C Hamano 2015-04-15 16:20 ` Odd broken "--date=now" behavior in current git Linus Torvalds 0 siblings, 2 replies; 10+ messages in thread From: Eric Sunshine @ 2015-04-15 7:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List On Tue, Apr 14, 2015 at 09:47:38PM -0700, Junio C Hamano wrote: > Linus Torvalds <torvalds@linux-foundation.org> writes: > > Lookie here, I can reproduce it trivially with current git (in the git > > repo itself): > > > > [torvalds@i7 git]$ date; git commit -m Test --allow-empty --date=now > > Tue Apr 14 21:11:03 PDT 2015 > > [master ec7733db5360] Test > > Date: Tue Apr 14 20:11:03 2015 -0800 > > > > notice how the commit date message shows something funny. It shows an > > hour earlier, but in -0800. > > > > And the resulting commit is broken: > > > > [torvalds@i7 git]$ git show --pretty=fuller > > commit ec7733db5360966434e03eab1a849e6d4227231c (HEAD -> master) > > Author: Linus Torvalds <torvalds@linux-foundation.org> > > AuthorDate: Tue Apr 14 20:11:03 2015 -0800 > > Commit: Linus Torvalds <torvalds@linux-foundation.org> > > CommitDate: Tue Apr 14 21:11:03 2015 -0700 > > > > Test > > > > notice how the AuthorDate has that "-0800", but the CommitDate has "-0700". > > With a quick check, the symptom exists at least at v2.1.4. v2.0.x > series does not seem to have --date=now support but since there is > no change to date.c between v2.0.0 to v2.1.4, older approxidate may > be equally broken. > > Will dig tomorrow, if nobody beats me to it, that is. I'm not familiar with this code, but have been studying it since reading this email, and I think I know what's going on. The problem isn't with "approxidate", but rather with the date form "@nseconds" returned by approxidate. builtin/commit.c calls fmt_ident() with the "@nseconds" date, which calls parse_date(), which finally calls parse_date_basic(), and therein lies the problem. parse_date_basic() does correctly set tm_isdst=-1, however, when it encounters a date of form "@nseconds", it invokes match_digit() which calls gmtime_r() to fill in the 'tm' structure, and gmtime_r() sets tm_isdst=0 (since DST is definitely false at GMT). Later parse_date_basic() computes the offset from GMT by comparing the values returned by tm_to_time_t() and mktime(). The existing 'tm' is passed to mktime() with the tm_isdst field already set to 0 by gmtime_r(), and mktime() respects that as a statement that DST is not in effect, rather than determining it dynamically. The fix seems to be simply: ---- >8 ---- diff --git a/date.c b/date.c index 3eba2df..99ad2a0 100644 --- a/date.c +++ b/date.c @@ -707,6 +707,7 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset) /* mktime uses local timezone */ *timestamp = tm_to_time_t(&tm); if (*offset == -1) { + tm.tm_isdst = -1; time_t temp_time = mktime(&tm); if ((time_t)*timestamp > temp_time) { *offset = ((time_t)*timestamp - temp_time) / 60; ---- >8 ---- However, I'm still digesting the code, so I haven't fully convinced myself that that's all that's needed. (It doesn't break any tests, and it does fix this issue.) ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Odd broken "--date=now" behavior in current git 2015-04-15 7:22 ` Eric Sunshine @ 2015-04-15 14:42 ` Junio C Hamano 2015-04-15 16:21 ` [PATCH 1/2] parse_date_basic(): return early when given a bogus timestamp Junio C Hamano 2015-04-15 16:24 ` [PATCH 2/2] parse_date_basic(): let the system handle DST conversion Junio C Hamano 2015-04-15 16:20 ` Odd broken "--date=now" behavior in current git Linus Torvalds 1 sibling, 2 replies; 10+ messages in thread From: Junio C Hamano @ 2015-04-15 14:42 UTC (permalink / raw) To: Eric Sunshine; +Cc: Linus Torvalds, Git Mailing List Eric Sunshine <sunshine@sunshineco.com> writes: > Later parse_date_basic() computes the offset from GMT by comparing > the values returned by tm_to_time_t() and mktime(). The existing 'tm' > is passed to mktime() with the tm_isdst field already set to 0 by > gmtime_r(), and mktime() respects that as a statement that DST is not > in effect, rather than determining it dynamically. > > The fix seems to be simply: > > ---- >8 ---- > diff --git a/date.c b/date.c > index 3eba2df..99ad2a0 100644 > --- a/date.c > +++ b/date.c > @@ -707,6 +707,7 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset) > /* mktime uses local timezone */ > *timestamp = tm_to_time_t(&tm); > if (*offset == -1) { > + tm.tm_isdst = -1; > time_t temp_time = mktime(&tm); > if ((time_t)*timestamp > temp_time) { > *offset = ((time_t)*timestamp - temp_time) / 60; > ---- >8 ---- I briefly wondered if the caller of gmtime_r() in match_digit() should be preserving the tm_isdst, though, as that codepath knows that it is handling a bare number without GMT offset. But resetting it to -1 here makes it even less error prone (we may gain other code that stomp on tm.tm_isdst before we get here, and having -1 in *offset is a sign that nobody saw GMT offset in the input). I think I see a decl-after-statment, but other than that, this looks like a good fix. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] parse_date_basic(): return early when given a bogus timestamp 2015-04-15 14:42 ` Junio C Hamano @ 2015-04-15 16:21 ` Junio C Hamano 2015-04-15 16:24 ` [PATCH 2/2] parse_date_basic(): let the system handle DST conversion Junio C Hamano 1 sibling, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2015-04-15 16:21 UTC (permalink / raw) To: Eric Sunshine; +Cc: Linus Torvalds, Git Mailing List When the input does not have GMT timezone offset, the code computes it by computing the local and GMT time for the given timestamp. But there is no point doing so if the given timestamp is known to be a bogus one. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * A simple preliminary clean-up while we are in the vicinity. We may want to use time_t throughout the codepath and turn it into ulong at the very last, but that would be a separate topic. date.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/date.c b/date.c index 782de95..01fd73f 100644 --- a/date.c +++ b/date.c @@ -696,6 +696,9 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset) /* mktime uses local timezone */ *timestamp = tm_to_time_t(&tm); + if (*timestamp == -1) + return -1; + if (*offset == -1) { time_t temp_time = mktime(&tm); if ((time_t)*timestamp > temp_time) { @@ -705,8 +708,6 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset) } } - if (*timestamp == -1) - return -1; if (!tm_gmt) *timestamp -= *offset * 60; -- 2.4.0-rc2-165-g862640d ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] parse_date_basic(): let the system handle DST conversion 2015-04-15 14:42 ` Junio C Hamano 2015-04-15 16:21 ` [PATCH 1/2] parse_date_basic(): return early when given a bogus timestamp Junio C Hamano @ 2015-04-15 16:24 ` Junio C Hamano 2015-04-15 17:23 ` Eric Sunshine 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2015-04-15 16:24 UTC (permalink / raw) To: Eric Sunshine; +Cc: Linus Torvalds, Git Mailing List The function parses the input to compute the broken-down time in "struct tm", and the GMT timezone offset. If the timezone offset does not exist in the input, the broken-down time is turned into the number of seconds since epoch both in the current timezone and in GMT and the offset is computed as their difference. However, we forgot to make sure tm.tm_isdst is set to -1 (i.e. let the system figure out if DST is in effect in the current timezone when turning the broken-down time to the number of seconds since epoch); it is done so at the beginning of the function, but a call to match_digit() in the function can lead to a call to gmtime_r() to clobber the field. Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Diagnosed-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- date.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/date.c b/date.c index 01fd73f..8ad6cef 100644 --- a/date.c +++ b/date.c @@ -700,7 +700,11 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset) return -1; if (*offset == -1) { - time_t temp_time = mktime(&tm); + time_t temp_time; + + /* gmtime_r() in match_digit() may have clobbered it */ + tm.tm_isdst = -1; + temp_time = mktime(&tm); if ((time_t)*timestamp > temp_time) { *offset = ((time_t)*timestamp - temp_time) / 60; } else { -- 2.4.0-rc2-165-g862640d ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] parse_date_basic(): let the system handle DST conversion 2015-04-15 16:24 ` [PATCH 2/2] parse_date_basic(): let the system handle DST conversion Junio C Hamano @ 2015-04-15 17:23 ` Eric Sunshine 0 siblings, 0 replies; 10+ messages in thread From: Eric Sunshine @ 2015-04-15 17:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List On Wed, Apr 15, 2015 at 12:24 PM, Junio C Hamano <gitster@pobox.com> wrote: > The function parses the input to compute the broken-down time in > "struct tm", and the GMT timezone offset. If the timezone offset > does not exist in the input, the broken-down time is turned into the > number of seconds since epoch both in the current timezone and in > GMT and the offset is computed as their difference. > > However, we forgot to make sure tm.tm_isdst is set to -1 (i.e. let > the system figure out if DST is in effect in the current timezone > when turning the broken-down time to the number of seconds since > epoch); it is done so at the beginning of the function, but a call > to match_digit() in the function can lead to a call to gmtime_r() to > clobber the field. Thanks for composing the commit message and turning this into a proper patch. > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Diagnosed-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> For what it's worth: Reviewed-by: Eric Sunshine <sunshine@sunshineco.com> > --- > date.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/date.c b/date.c > index 01fd73f..8ad6cef 100644 > --- a/date.c > +++ b/date.c > @@ -700,7 +700,11 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset) > return -1; > > if (*offset == -1) { > - time_t temp_time = mktime(&tm); > + time_t temp_time; > + > + /* gmtime_r() in match_digit() may have clobbered it */ > + tm.tm_isdst = -1; > + temp_time = mktime(&tm); > if ((time_t)*timestamp > temp_time) { > *offset = ((time_t)*timestamp - temp_time) / 60; > } else { > -- > 2.4.0-rc2-165-g862640d ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Odd broken "--date=now" behavior in current git 2015-04-15 7:22 ` Eric Sunshine 2015-04-15 14:42 ` Junio C Hamano @ 2015-04-15 16:20 ` Linus Torvalds 2015-04-15 17:04 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Linus Torvalds @ 2015-04-15 16:20 UTC (permalink / raw) To: Eric Sunshine; +Cc: Junio C Hamano, Git Mailing List On Wed, Apr 15, 2015 at 12:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote: > > The fix seems to be simply: Yup, that seems to do it for me. I'm not sure how we get to "match_digit()" with the time string "now", though. So your patch fixes things for me, but I think: - we should move the "tm.tm_isdst = -1;" up a bit and add a comment - I'd like to know why it affected the author date but not the committer date, and how it got to match_digit() with that date string that didn't contain any digits. I'd spend the time on this myself, but I'm in the middle of the kernel merge window, so.. Thanks, Linus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Odd broken "--date=now" behavior in current git 2015-04-15 16:20 ` Odd broken "--date=now" behavior in current git Linus Torvalds @ 2015-04-15 17:04 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2015-04-15 17:04 UTC (permalink / raw) To: Linus Torvalds; +Cc: Eric Sunshine, Git Mailing List Linus Torvalds <torvalds@linux-foundation.org> writes: > On Wed, Apr 15, 2015 at 12:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote: >> >> The fix seems to be simply: > > Yup, that seems to do it for me. I'm not sure how we get to > "match_digit()" with the time string "now", though. The --date=<when> parameter is read by determine_author_info() and then parse_force_date(), both in builtin/commit.c. This calls parse_date(), which first calls (and fails) parse_date_basic() when it is not a strict timestamp. "now", "4.days.ago", etc. are then passed to approxidate_careful() which in turn calls approxidate_str(). Note that the approxidate formats do not usually spell out the TZ offset. The result is turned into "@<numseconds>" by parse_force_date(). The string "@<numseconds>" without TZ offset is re-interpreted by parse_date() called by fmt_ident(); the last function prepares the author ident line "Name <email> <numseconds> <offset>". The --date=<when> does not force committer information, which goes through the usual codepath of grabbing the "current" time. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Odd broken "--date=now" behavior in current git 2015-04-15 4:18 Odd broken "--date=now" behavior in current git Linus Torvalds 2015-04-15 4:47 ` Junio C Hamano @ 2015-04-15 7:07 ` Peter Krefting 1 sibling, 0 replies; 10+ messages in thread From: Peter Krefting @ 2015-04-15 7:07 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List Linus Torvalds: > I can't be the only one seeing this? My guess is that there's a > missing initialization of tm.tm_isdst somewhere or whatever. I can confirm it if I enable DST on my machine (I usually run my machines on CET all-year, to avoid these kind of issues): $ echo $TZ Europe/Oslo $ git commit -m b [master dee7ec1] b 1 file changed, 1 insertion(+) create mode 100644 b.txt $ git show --pretty=fuller commit dee7ec1cda74a8abd7f26c60ee1e83f73bb31194 (HEAD -> master) Author: Peter Krefting <peter.krefting@bridgetech.tv> AuthorDate: 2015-04-15 09:04:34 +0200 Commit: Peter Krefting <peter.krefting@bridgetech.tv> CommitDate: 2015-04-15 09:04:34 +0200 [...] $ git commit --amend --date=now [...] $ git show --pretty=fuller commit b4561e5a077de7bbcaf9fc06350ea24407adcec0 (HEAD -> master) Author: Peter Krefting <peter.krefting@bridgetech.tv> AuthorDate: 2015-04-15 08:05:04 +0100 Commit: Peter Krefting <peter.krefting@bridgetech.tv> CommitDate: 2015-04-15 09:05:04 +0200 So the datetime is correct, it's just the timezone that is wrong (08:05+0100 = 09:05+0200). $ git --version git version 2.4.0.rc1 -- \\// Peter - http://www.softwolves.pp.se/ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-04-15 17:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-15 4:18 Odd broken "--date=now" behavior in current git Linus Torvalds 2015-04-15 4:47 ` Junio C Hamano 2015-04-15 7:22 ` Eric Sunshine 2015-04-15 14:42 ` Junio C Hamano 2015-04-15 16:21 ` [PATCH 1/2] parse_date_basic(): return early when given a bogus timestamp Junio C Hamano 2015-04-15 16:24 ` [PATCH 2/2] parse_date_basic(): let the system handle DST conversion Junio C Hamano 2015-04-15 17:23 ` Eric Sunshine 2015-04-15 16:20 ` Odd broken "--date=now" behavior in current git Linus Torvalds 2015-04-15 17:04 ` Junio C Hamano 2015-04-15 7:07 ` Peter Krefting
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.