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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 15, 2015, 07:22 UTC
Message-ID: <20150415072223.GA1389@flurp.local>
In-Reply-To: <xmqqzj6ayp3p.fsf@gitster.dls.corp.google.com>

On Tue, Apr 14, 2015 at 09:47:38PM -0700, Junio C Hamano wrote:

Show 31 quoted lines

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