Odd broken "--date=now" behavior in current git

Odd broken "--date=now" behavior in current git

[Linus Torvalds]

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

Re: Odd broken "--date=now" behavior in current git

[Junio C Hamano]

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.

Re: Odd broken "--date=now" behavior in current git

[Eric Sunshine]

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

Re: Odd broken "--date=now" behavior in current git

[Junio C Hamano]

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.

[PATCH 1/2] parse_date_basic(): return early when given a bogus timestamp

[Junio C Hamano]

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

[PATCH 2/2] parse_date_basic(): let the system handle DST conversion

[Junio C Hamano]

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

Re: [PATCH 2/2] parse_date_basic(): let the system handle DST conversion

[Eric Sunshine]

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

Re: Odd broken "--date=now" behavior in current git

[Linus Torvalds]

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

Re: Odd broken "--date=now" behavior in current git

[Junio C Hamano]

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.

Re: Odd broken "--date=now" behavior in current git

[Peter Krefting]

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/