On Tue, Apr 14, 2015 at 09:47:38PM -0700, Junio C Hamano wrote:
> Linus Torvalds <[email protected]> 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 <[email protected]>
> > AuthorDate: Tue Apr 14 20:11:03 2015 -0800
> > Commit: Linus Torvalds <[email protected]>
> > 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.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
