On Fri, Jun 02, 2017 at 07:25:43PM +0200, René Scharfe wrote:
> Am 02.06.2017 um 05:08 schrieb Jeff King:
> > In theory the solution is:
> >
> > 1. Start using localtime() instead of gmtime() with an adjustment when
> > we are converting to the local timezone (i.e., format-local). We
> > should be able to do this portably.
> >
> > This is easy to do, and it's better than handling %z ourselves,
> > because it makes %Z work, too.
> >
> > 2. When showing the author's timezone, do some trickery to set the
> > program's timezone, then use localtime(), then restore the program
> > timezone.
> >
> > I couldn't get this to work reliably. And anyway, we'd still have
> > nothing to put in %Z since we don't have a timezone name at all in
> > the git objects. We just have "+0400" or whatever.
> >
> > So I don't see a portable way to make (2) work.
>
> We could create a strftime wrapper that also takes a time zone offset,
> with platform-specific implementations. Is it worth the effort?
>
> What reliability issues did you run into?
My patch is below for reference. The issue is that we have to stuff a
name into $TZ that the system libc will parse into something sensible.
Just setting it to "%+05d" doesn't work at all. glibc at least seems to
accept names like FOO+4, but:
- I have no idea if that's portable
- it only allows single-hour offsets, so +0330 is out. There might be
some way to represent that, but I'm not sure if it's portable
(FOO+0300 doesn't seem to work even on glibc).
- that sets %Z to "FOO", which is obviously nonsense
> > If we do handle "%z" ourselves (either always or for just the one case),
> > what should the matching %Z say? Right now (and I think with René's
> > patch) it says GMT, which is actively misleading. We should probably
> > replace it with the same text as "%z". That's not quite what the user
> > wanted, but at least it's accurate.
>
> On Linux "%z %Z" is expanded to "+0200 CEST" for me, while on Windows I
> get "Mitteleurop▒ische Sommerzeit Mitteleurop▒ische Sommerzeit". (That
> "▒" is probably supposed to be an "ä".) POSIX requires +hhmm or -hhmm
> format for %z, and for %Z is to be "Replaced by the timezone name or
> abbreviation".
>
> I'd say "GMT+0200" etc. is a nice enough timezone name, i.e. having %Z
> resolve to the same as %z plus a literal prefix of "GMT" should at least
> not be wrong.
I thought that, too, but I think it is wrong based on my understanding
of how $TZ is parsed. There something like "EDT-4" means "call this EDT,
and by the way it is 4 hours behind GMT".
So what you're proposing isn't wrong per se, but your notation means
something totally different than what similar-looking notation looks
like on the $TZ end, which is bound to create confusion.
> Alternatively we could have a lookup table mapping a few typical offsets
> to timezone names, but e.g. handling daylight saving times would
> probably be too hard (when did that part of the world switch in the
> given year? north or south of the equator?)..
Right, I don't think the mapping of zone to offset is reversible,
because many zones map to the same offset. If I tell you I'm in -0500,
even just in the US that could mean Eastern Standard Time (winter, no
DST) or Central Daylight Time (summer, DST). Not to mention that other
political entities in the same longitude have their own zones which do
DST at different times (or were even established as zones at different
times; historical dates need to use the zones as they were at that
time).
> > As far as the patch itself goes, I'm disappointed to lose the automatic
> > "%" handling for all of the other callers. But I suspect the boilerplate
> > involved in any solution that lets callers choose whether or not to use
> > it would end up being longer than just handling it in each caller.
>
> Actually I felt uneasy when you added that forced %% handling because it
> put a policy into an otherwise neutral interpreter function. I just had
> no practical argument against it -- until now.
>
> I'd rather see strbuf_expand also lose the hard-coded percent sign, but
> again I don't have an actual user for such a flexibility (yet).
>
> Perhaps we should add a fully neutral strbuf_expand_core (or whatever),
> make strbuf_expand a wrapper with hard-coded % and %% handling and use
> the core function in the strftime wrapper. Except that the function is
> not easily stackable. Hmm..
Right, that's the boilerplate trickiness I was referring to. It's
probably not worth the effort.
Anyway, here's my patch. I've been testing it with:
./git log --format='%ai%n%ad%n' --date=format:'%Y-%m-%d %H:%M:%S %z (%Z)'
which lets you compare a variety of commits with the existing formatting
routine.
---
diff --git a/date.c b/date.c
index 63fa99685..a6214172e 100644
--- a/date.c
+++ b/date.c
@@ -196,6 +196,34 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
return timebuf.buf;
}
+ if (mode->type == DATE_STRFTIME) {
+ char *orig_tz;
+ time_t t = time;
+
+ if (!mode->local) {
+ char *new_tz = xstrfmt("GIT%+d", -tz / 100);
+ orig_tz = xstrdup_or_null(getenv("TZ"));
+ setenv("TZ", new_tz, 1);
+ free(new_tz);
+ tzset();
+ }
+
+ tm = localtime(&t);
+ strbuf_reset(&timebuf);
+ strbuf_addftime(&timebuf, mode->strftime_fmt, tm);
+
+ if (!mode->local) {
+ if (orig_tz)
+ setenv("TZ", orig_tz, 1);
+ else
+ unsetenv("TZ");
+ free(orig_tz);
+ tzset();
+ }
+
+ return timebuf.buf;
+ }
+
if (mode->local)
tz = local_tzoffset(time);
@@ -245,8 +273,6 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
weekday_names[tm->tm_wday], tm->tm_mday,
month_names[tm->tm_mon], tm->tm_year + 1900,
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
- else if (mode->type == DATE_STRFTIME)
- strbuf_addftime(&timebuf, mode->strftime_fmt, tm);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],