Re: git-2.13.0: log --date=format:%z not working

René Scharfe [off-list ref] writes:

Am 27.05.2017 um 23:46 schrieb Jeff King:

quoted

On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

There's another test which breaks if we just s/gmtime/localtime/g. As
far as I can tell to make the non-local case work we'd need to do a
whole dance where we set the TZ variable to e.g. UTC$offset, then call
strftime(), then call it again. Maybe there's some way to just specify
the tz offset, but I didn't find any in a quick skimming of time.h.

There isn't.

Right.  We could handle %z internally, though.  %Z would be harder (left
as an exercise for readers..).

First we'd have to undo 0a0416a3 (strbuf_expand: convert "%%" to "%"),
though, in order to give full control back to strbuf_expand callbacks.

2-pack patch:

I think the list concensus is that handling %z ourselves like this
one does is the best we can do portably.

Anybody wants to wrap this up into a patch with log message?

Thanks.

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?

But it seems a shame
that %Z does not work for case (1) with René's patch.

I guess we could do (1) for the local cases and then handle "%z"
ourselves otherwise. That sounds even _more_ confusing, but it at least
gets the most cases right.

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.

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

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

René

On Fri, Jun 02, 2017 at 07:25:43PM +0200, René Scharfe wrote:

Am 02.06.2017 um 05:08 schrieb Jeff King:

quoted

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

quoted

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

quoted

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],

On Fri, Jun 02, 2017 at 11:23:30AM +0900, Junio C Hamano wrote:

René Scharfe [off-list ref] writes:

quoted

Am 27.05.2017 um 23:46 schrieb Jeff King:

quoted

On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

There's another test which breaks if we just s/gmtime/localtime/g. As
far as I can tell to make the non-local case work we'd need to do a
whole dance where we set the TZ variable to e.g. UTC$offset, then call
strftime(), then call it again. Maybe there's some way to just specify
the tz offset, but I didn't find any in a quick skimming of time.h.

There isn't.

Right.  We could handle %z internally, though.  %Z would be harder (left
as an exercise for readers..).

First we'd have to undo 0a0416a3 (strbuf_expand: convert "%%" to "%"),
though, in order to give full control back to strbuf_expand callbacks.

2-pack patch:

I think the list concensus is that handling %z ourselves like this
one does is the best we can do portably.

I've actually been turning it over in my head. This feels hacky, but I'm
not sure if we can do better.

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. But it seems a shame
that %Z does not work for case (1) with René's patch.

I guess we could do (1) for the local cases and then handle "%z"
ourselves otherwise. That sounds even _more_ confusing, but it at least
gets the most cases right.

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.

quoted

--- >8 ---
 builtin/cat-file.c         |  5 +++++
 convert.c                  |  1 +
 daemon.c                   |  3 +++
 date.c                     |  2 +-
 ll-merge.c                 |  5 +++--
 pretty.c                   |  3 +++
 strbuf.c                   | 39 ++++++++++++++++++++++++++++++---------
 strbuf.h                   | 11 +++++------
 t/t6006-rev-list-format.sh | 12 ++++++++++++
 9 files changed, 63 insertions(+), 18 deletions(-)

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.

-Peff

quoted

quoted

quoted

quoted

On Fri, 2 Jun 2017, Jeff King wrote:

On Fri, Jun 02, 2017 at 07:25:43PM +0200, René Scharfe wrote:

quoted

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

Actually, the POSIX definition for %Z continues: "or by no bytes if no
timezone information exists." So also returning an empty string would
be compliant (but maybe not very helpful).

quoted

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.

I agree that GMT+0200 could be misleading. But what about resolving %Z
the same as %z in the case of the author's time zone, as was suggested
earlier? It is supposed to be human-readable output, or do we expect
that someone would use the %Z output and e.g. plug it back into their
TZ?

quoted

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

IMHO maintaining such a local table of timezones won't fly.

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

Same here, my +0200 offset could be anything of CAT, CEST, EET, IST,
SAST, or WAST, according to IANA timezone data. It's a one-directional
mapping, and there's no way to get the author's /etc/localtime info
(or whatever its equivalent is on other systems) back from the offset
stored in the commit. A timezone name may not even exist at all for a
given [+-]hhmm offset.

Ulrich

On Sat, Jun 03, 2017 at 12:04:32AM +0200, Ulrich Mueller wrote:

Actually, the POSIX definition for %Z continues: "or by no bytes if no
timezone information exists." So also returning an empty string would
be compliant (but maybe not very helpful).
[...]
I agree that GMT+0200 could be misleading. But what about resolving %Z
the same as %z in the case of the author's time zone, as was suggested
earlier? It is supposed to be human-readable output, or do we expect
that someone would use the %Z output and e.g. plug it back into their
TZ?

Yeah, I think these are the only real contenders: an empty string, or
"+0200" (which _isn't_ confusing, because it doesn't have the
abbreviation in front of it, so it pretty clearly is an offset from
GMT).

I don't have a preference between the other two.

The remaining question is whether we want to care about preserving the
system %Z for the local-timezone case.

-Peff

quoted

quoted

quoted

quoted

On Fri, 2 Jun 2017, Jeff King wrote:

The remaining question is whether we want to care about preserving the
system %Z for the local-timezone case.

No strong preference here. Maybe go for consistency, and have %Z
always return the same format (either empty, or same as %z). That
would at least prevent surprises when users switch from format-local
to format.

Ulrich

On Sat, Jun 03, 2017 at 12:47:59AM +0200, Ulrich Mueller wrote:

quoted

quoted

quoted

quoted

quoted

On Fri, 2 Jun 2017, Jeff King wrote:

quoted

The remaining question is whether we want to care about preserving the
system %Z for the local-timezone case.

No strong preference here. Maybe go for consistency, and have %Z
always return the same format (either empty, or same as %z). That
would at least prevent surprises when users switch from format-local
to format.

It also a lot easier to implement, which is nice.

I agree on the least surprise thing, but the flipside of this is that
Git's use of strftime will behave differently than other programs on the
system (e.g., "date +%Z").

-Peff

There is no portable way to pass timezone information to strftime.  Add
parameters for timezone offset and name to strbuf_addftime and let it
handle the timezone-related format specifiers %z and %Z internally.
Callers can opt out by passing NULL as timezone name.

Use an empty string as timezone name in show_date (the only current
caller) for now because we only have the timezone offset in non-local
mode.  POSIX allows %Z to resolve to nothing in case of missing info.

Helped-by: Ulrich Mueller [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Rene Scharfe <redacted>
---
Duplicates strbuf_expand to a certain extent, but not too badly, I
think.  Leaves the door open for letting strftime handle the local
case.

 date.c                     |  2 +-
 strbuf.c                   | 42 ++++++++++++++++++++++++++++++++++++++----
 strbuf.h                   | 11 ++++++++---
 t/t6006-rev-list-format.sh | 12 ++++++++++++
 4 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/date.c b/date.c
index 63fa99685e..5580577334 100644
--- a/date.c
+++ b/date.c

@@ -246,7 +246,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			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);
+		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz, "");
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],

diff --git a/strbuf.c b/strbuf.c
index 00457940cf..b880107370 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -785,14 +785,47 @@ char *xstrfmt(const char *fmt, ...)
 	return ret;
 }
 
-void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
+void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
+		     int tz_offset, const char *tz_name)
 {
+	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
 	size_t len;
 
 	if (!*fmt)
 		return;
 
+	/*
+	 * There is no portable way to pass timezone information to
+	 * strftime, so we handle %z and %Z here.
+	 */
+	if (tz_name) {
+		for (;;) {
+			const char *percent = strchrnul(fmt, '%');
+			strbuf_add(&munged_fmt, fmt, percent - fmt);
+			if (!*percent)
+				break;
+			fmt = percent + 1;
+			switch (*fmt) {
+			case '%':
+				strbuf_addstr(&munged_fmt, "%%");
+				fmt++;
+				break;
+			case 'z':
+				strbuf_addf(&munged_fmt, "%+05d", tz_offset);
+				fmt++;
+				break;
+			case 'Z':
+				strbuf_addstr(&munged_fmt, tz_name);
+				fmt++;
+				break;
+			default:
+				strbuf_addch(&munged_fmt, '%');
+			}
+		}
+		fmt = munged_fmt.buf;
+	}
+
 	strbuf_grow(sb, hint);
 	len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
 

@@ -804,17 +837,18 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 		 * output contains at least one character, and then drop the extra
 		 * character before returning.
 		 */
-		struct strbuf munged_fmt = STRBUF_INIT;
-		strbuf_addf(&munged_fmt, "%s ", fmt);
+		if (fmt != munged_fmt.buf)
+			strbuf_addstr(&munged_fmt, fmt);
+		strbuf_addch(&munged_fmt, ' ');
 		while (!len) {
 			hint *= 2;
 			strbuf_grow(sb, hint);
 			len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
 				       munged_fmt.buf, tm);
 		}
-		strbuf_release(&munged_fmt);
 		len--; /* drop munged space */
 	}
+	strbuf_release(&munged_fmt);
 	strbuf_setlen(sb, sb->len + len);
 }
 

diff --git a/strbuf.h b/strbuf.h
index 80047b1bb7..39d5836abd 100644
--- a/strbuf.h
+++ b/strbuf.h

@@ -339,9 +339,14 @@ __attribute__((format (printf,2,0)))
 extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
- * Add the time specified by `tm`, as formatted by `strftime`.
- */
-extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm);
+ * Add the time specified by `tm`, as formatted by `strftime`.  `tz_offset`
+ * and `tz_name` are used to expand %z and %Z internally, unless `tz_name`
+ * is NULL.  `tz_offset` is in decimal hhmm format, e.g. -600 means six
+ * hours west of Greenwich.
+ */
+extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
+			    const struct tm *tm, int tz_offset,
+			    const char *tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index a1dcdb81d7..dc0bed8d90 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh

@@ -483,4 +483,16 @@ test_expect_success 'unused %G placeholders are passed through' '
 	test_cmp expect actual
 '
 
+test_expect_success 'date format "%F %T %z" is the same as iso' '
+	git log -1 --format="%ad" --date=iso >expect &&
+	git log -1 --format="%ad" --date="format:%F %T %z" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'date format "%%z" expands to percent zed' '
+	echo "%z" >expect &&
+	git log -1 --format="%ad" --date="format:%%z" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.13.0

quoted

quoted

quoted

quoted

On Sat, 3 Jun 2017, René Scharfe wrote:

+			case 'Z':
+				strbuf_addstr(&munged_fmt, tz_name);

Is it guaranteed that tz_name cannot contain a percent sign itself?

Ulrich

Am 03.06.2017 um 15:13 schrieb Ulrich Mueller:

quoted

quoted

quoted

quoted

quoted

On Sat, 3 Jun 2017, René Scharfe wrote:

quoted

+			case 'Z':
+				strbuf_addstr(&munged_fmt, tz_name);

Is it guaranteed that tz_name cannot contain a percent sign itself?

Currently yes, because the only caller passes an empty string.

The fact that tz_name is subject to expansion by strftime could be
mentioned explicitly in strbuf.h.  I'm not sure if that's a desirable
property, but it allows callers to expand %z internally and %Z using
strftime.

René

On Sat, Jun 03, 2017 at 12:40:34PM +0200, René Scharfe wrote:

There is no portable way to pass timezone information to strftime.  Add
parameters for timezone offset and name to strbuf_addftime and let it
handle the timezone-related format specifiers %z and %Z internally.
Callers can opt out by passing NULL as timezone name.

Use an empty string as timezone name in show_date (the only current
caller) for now because we only have the timezone offset in non-local
mode.  POSIX allows %Z to resolve to nothing in case of missing info.

This direction looks good to me overall. It's not pretty, but I think
it's the least-bad option.

---
Duplicates strbuf_expand to a certain extent, but not too badly, I
think.  Leaves the door open for letting strftime handle the local
case.

I guess you'd plan to do that like this in the caller:

  if (date->local)
	tz_name = NULL;
  else
	tz_name = "";

and then your strftime() doesn't do any %z expansion when tz_name is
NULL.

I was thinking that we would need to have it take the actual time_t, and
then it would be able to do the tzset/localtime dance itself. But since
I don't think we're planning to do that (if anything we'd just handle
the normal localtime() case), the complication it would add to the
interface isn't worth it.

-void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
+void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
+		     int tz_offset, const char *tz_name)
 {
+	struct strbuf munged_fmt = STRBUF_INIT;

Now we're doing two types of munging: sometimes handling %z, and
sometimes the extra-space hack. I had to read through it carefully to
make sure we handle all cases correctly, but I think it works.

In particular, I worried about us setting "fmt" to munged_fmt.buf for
the %z case, and then later adding the extra space to it for the
zero-length hack, which might reallocate, leaving "fmt" pointing to
unallocated memory. But it's OK because at that point we never touch the
original "fmt" again.

 /**
- * Add the time specified by `tm`, as formatted by `strftime`.
- */
-extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm);
+ * Add the time specified by `tm`, as formatted by `strftime`.  `tz_offset`
+ * and `tz_name` are used to expand %z and %Z internally, unless `tz_name`
+ * is NULL.  `tz_offset` is in decimal hhmm format, e.g. -600 means six
+ * hours west of Greenwich.
+ */
+extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
+			    const struct tm *tm, int tz_offset,
+			    const char *tz_name);

Good, documentation (the diff order put the implementation first so I
scratched my head for a moment before realizing you had already
described it).

-Peff

On Wed, Jun 07, 2017 at 04:17:29AM -0400, Jeff King wrote:

quoted

Duplicates strbuf_expand to a certain extent, but not too badly, I
think.  Leaves the door open for letting strftime handle the local
case.

I guess you'd plan to do that like this in the caller:

  if (date->local)
	tz_name = NULL;
  else
	tz_name = "";

and then your strftime() doesn't do any %z expansion when tz_name is
NULL.

And here's a patch that handles the local case.

-- >8 --
Subject: [PATCH] date: use localtime() for "-local" time formats

When we convert seconds-since-epochs timestamps into a
broken-down "struct tm", we do so by adjusting the timestamp
according to the correct timezone and then using gmtime() to
break down the result. This means that the resulting struct
"knows" that it's in GMT, even though the time it represents
is adjusted for a different zone. The fields where it stores
this data are not portably accessible, so we have no way to
override them to tell them the real zone info.

For the most part, this works. Our date-formatting routines
don't pay attention to these inaccessible fields, and use
the the same tz info we provided for adjustment. The one
exception is when we call strftime(), whose %z and %Z
formats reveal this hidden timezone data.

We can't make this work in the general case, as there's no
portable function for setting an arbitrary timezone. But for
the special case of the "-local" formats, we can just skip
the adjustment and use localtime() instead of gmtime(). This
makes --date=format-local:%Z work correctly, showing the
local timezone instead of an empty string.

This patch adds three tests:

  1. We check that format:%Z returns an empty string. This
     isn't what we'd want ideally, but it's important to
     confirm that it doesn't produce nonsense (like GMT when
     we are formatting another zone entirely).

  2. We check that format-local:%Z produces "UTC", which is
     the value of $TZ set by test-lib.sh.

  3. We check that format-local actually produces the
     correct time for zones other than UTC. If we made a
     mistake in the adjustment logic (say, applying the tz
     adjustment even though we are about to call
     localtime()), it wouldn't show up in the second test,
     because the offset for UTC is 0.

     We use the EST5 zone, which is already used elsewhere
     in the script, so is assumed to be available
     everywhere.

     However, this test _doesn't_ check %Z. That expansion
     produces an abbreviation which may not be portable
     across systems (on my system it expands as just "EST").
     Technically "UTC" could suffer from the same problem,
     but presumably it's universal enough to be relied upon.

Signed-off-by: Jeff King <redacted>
---
 date.c          | 14 ++++++++++++--
 t/t0006-date.sh | 20 ++++++++++++++++++--
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/date.c b/date.c
index 558057733..1fd6d6637 100644
--- a/date.c
+++ b/date.c

@@ -70,6 +70,12 @@ static struct tm *time_to_tm(timestamp_t time, int tz)
 	return gmtime(&t);
 }
 
+static struct tm *time_to_tm_local(timestamp_t time)
+{
+	time_t t = time;
+	return localtime(&t);
+}
+
 /*
  * What value of "tz" was in effect back then at "time" in the
  * local timezone?

@@ -214,7 +220,10 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
-	tm = time_to_tm(time, tz);
+	if (mode->local)
+		tm = time_to_tm_local(time);
+	else
+		tm = time_to_tm(time, tz);
 	if (!tm) {
 		tm = time_to_tm(0, 0);
 		tz = 0;

@@ -246,7 +255,8 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			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, tz, "");
+		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
+				mode->local ? NULL : "");
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 42d4ea61e..3be6692bb 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh

@@ -31,9 +31,18 @@ check_show () {
 	format=$1
 	time=$2
 	expect=$3
-	test_expect_success $4 "show date ($format:$time)" '
+	prereqs=$4
+	zone=$5
+	test_expect_success $prereqs "show date ($format:$time)" '
 		echo "$time -> $expect" >expect &&
-		test-date show:$format "$time" >actual &&
+		(
+			if test -n "$zone"
+			then
+				TZ=$zone
+				export $TZ
+			fi &&
+			test-date show:"$format" "$time" >actual
+		) &&
 		test_cmp expect actual
 	'
 }

@@ -51,6 +60,13 @@ check_show iso-local "$TIME" '2016-06-15 14:13:20 +0000'
 check_show raw-local "$TIME" '1466000000 +0000'
 check_show unix-local "$TIME" '1466000000'
 
+check_show "format:%Y-%m-%d %H:%M:%S %z (%Z)" "$TIME" \
+	   '2016-06-15 16:13:20 +0200 ()'
+check_show format-local:"%Y-%m-%d %H:%M:%S %z (%Z)" "$TIME" \
+	   '2016-06-15 14:13:20 +0000 (UTC)'
+check_show format-local:"%Y-%m-%d %H:%M:%S %z" "$TIME" \
+	   '2016-06-15 09:13:20 -0500' '' EST5
+
 # arbitrary time absurdly far in the future
 FUTURE="5758122296 -0400"
 check_show iso       "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT

-- 
2.13.1.664.g1b5a21ec3

Am 07.06.2017 um 10:17 schrieb Jeff King:

On Sat, Jun 03, 2017 at 12:40:34PM +0200, René Scharfe wrote:

quoted

Duplicates strbuf_expand to a certain extent, but not too badly, I
think.  Leaves the door open for letting strftime handle the local
case.

I guess you'd plan to do that like this in the caller:

   if (date->local)
	tz_name = NULL;
   else
	tz_name = "";

and then your strftime() doesn't do any %z expansion when tz_name is
NULL.

Yes, or you could look up a time zone name somewhere else -- except we
don't have a way to do that, at least for now.

I was thinking that we would need to have it take the actual time_t, and
then it would be able to do the tzset/localtime dance itself. But since
I don't think we're planning to do that (if anything we'd just handle
the normal localtime() case), the complication it would add to the
interface isn't worth it.

A caller that really needs to do that can, and pass the result as a
string.  Not pretty, but at least it's a possibility.

René

There is no portable way to pass timezone information to strftime.  Add
parameters for timezone offset and name to strbuf_addftime and let it
handle the timezone-related format specifiers %z and %Z internally.

Callers can opt out for %Z by passing NULL as timezone name.  %z is
always handled internally -- this helps on Windows, where strftime would
expand it to a timezone name (same as %Z), in violation of POSIX.
Modifiers are not handled, e.g. %Ez is still passed to strftime.

Use an empty string as timezone name in show_date (the only current
caller) for now because we only have the timezone offset in non-local
mode.  POSIX allows %Z to resolve to an empty string in case of missing
information.

Helped-by: Ulrich Mueller [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Rene Scharfe <redacted>
---
Changes from v1:
- Always handle %z internally.
- Move tests from t6006 to t0006, as that's a more appropriate place.
- Changed tests to only use %%, %z and %Z to avoid incompatibilities.
- Tested on mingw (applies there with patch and some fuzz).

 date.c          |  2 +-
 strbuf.c        | 41 +++++++++++++++++++++++++++++++++++++----
 strbuf.h        | 11 ++++++++---
 t/t0006-date.sh |  6 ++++++
 4 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/date.c b/date.c
index 63fa99685e..5580577334 100644
--- a/date.c
+++ b/date.c

@@ -246,7 +246,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			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);
+		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz, "");
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],

diff --git a/strbuf.c b/strbuf.c
index 00457940cf..be3b9e37b1 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -785,14 +785,48 @@ char *xstrfmt(const char *fmt, ...)
 	return ret;
 }
 
-void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
+void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
+		     int tz_offset, const char *tz_name)
 {
+	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
 	size_t len;
 
 	if (!*fmt)
 		return;
 
+	/*
+	 * There is no portable way to pass timezone information to
+	 * strftime, so we handle %z and %Z here.
+	 */
+	for (;;) {
+		const char *percent = strchrnul(fmt, '%');
+		strbuf_add(&munged_fmt, fmt, percent - fmt);
+		if (!*percent)
+			break;
+		fmt = percent + 1;
+		switch (*fmt) {
+		case '%':
+			strbuf_addstr(&munged_fmt, "%%");
+			fmt++;
+			break;
+		case 'z':
+			strbuf_addf(&munged_fmt, "%+05d", tz_offset);
+			fmt++;
+			break;
+		case 'Z':
+			if (tz_name) {
+				strbuf_addstr(&munged_fmt, tz_name);
+				fmt++;
+				break;
+			}
+			/* FALLTHROUGH */
+		default:
+			strbuf_addch(&munged_fmt, '%');
+		}
+	}
+	fmt = munged_fmt.buf;
+
 	strbuf_grow(sb, hint);
 	len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
 

@@ -804,17 +838,16 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 		 * output contains at least one character, and then drop the extra
 		 * character before returning.
 		 */
-		struct strbuf munged_fmt = STRBUF_INIT;
-		strbuf_addf(&munged_fmt, "%s ", fmt);
+		strbuf_addch(&munged_fmt, ' ');
 		while (!len) {
 			hint *= 2;
 			strbuf_grow(sb, hint);
 			len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
 				       munged_fmt.buf, tm);
 		}
-		strbuf_release(&munged_fmt);
 		len--; /* drop munged space */
 	}
+	strbuf_release(&munged_fmt);
 	strbuf_setlen(sb, sb->len + len);
 }
 

diff --git a/strbuf.h b/strbuf.h
index 80047b1bb7..39d5836abd 100644
--- a/strbuf.h
+++ b/strbuf.h

@@ -339,9 +339,14 @@ __attribute__((format (printf,2,0)))
 extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
- * Add the time specified by `tm`, as formatted by `strftime`.
- */
-extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm);
+ * Add the time specified by `tm`, as formatted by `strftime`.  `tz_offset`
+ * and `tz_name` are used to expand %z and %Z internally, unless `tz_name`
+ * is NULL.  `tz_offset` is in decimal hhmm format, e.g. -600 means six
+ * hours west of Greenwich.
+ */
+extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
+			    const struct tm *tm, int tz_offset,
+			    const char *tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 42d4ea61ef..71082008f0 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh

@@ -51,6 +51,12 @@ check_show iso-local "$TIME" '2016-06-15 14:13:20 +0000'
 check_show raw-local "$TIME" '1466000000 +0000'
 check_show unix-local "$TIME" '1466000000'
 
+check_show 'format:%z' "$TIME" '+0200'
+check_show 'format-local:%z' "$TIME" '+0000'
+check_show 'format:%Z' "$TIME" ''
+check_show 'format:%%z' "$TIME" '%z'
+check_show 'format-local:%%z' "$TIME" '%z'
+
 # arbitrary time absurdly far in the future
 FUTURE="5758122296 -0400"
 check_show iso       "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT

-- 
2.13.0

quoted

quoted

quoted

quoted

On Thu, 15 Jun 2017, René Scharfe wrote:

Callers can opt out for %Z by passing NULL as timezone name.  %z is
always handled internally -- this helps on Windows, where strftime would
expand it to a timezone name (same as %Z), in violation of POSIX.
Modifiers are not handled, e.g. %Ez is still passed to strftime.

POSIX would also allow other things, like a field width:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strftime.html

$ date '+%8z'
+0000200

(But I believe that's not very useful, and supporting it might require
duplicating much of strftime's code.)

Changes from v1:
- Always handle %z internally.

Minor nitpick: Shouldn't the comment in strbuf.h be updated to reflect
that change?

+ * Add the time specified by `tm`, as formatted by `strftime`.  `tz_offset`
+ * and `tz_name` are used to expand %z and %Z internally, unless `tz_name`
+ * is NULL.  `tz_offset` is in decimal hhmm format, e.g. -600 means six
+ * hours west of Greenwich.

Ulrich

Am 15.06.2017 um 13:27 schrieb Ulrich Mueller:

quoted

quoted

quoted

quoted

quoted

On Thu, 15 Jun 2017, René Scharfe wrote:

quoted

Callers can opt out for %Z by passing NULL as timezone name.  %z is
always handled internally -- this helps on Windows, where strftime would
expand it to a timezone name (same as %Z), in violation of POSIX.
Modifiers are not handled, e.g. %Ez is still passed to strftime.

POSIX would also allow other things, like a field width:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strftime.html

$ date '+%8z'
+0000200

(But I believe that's not very useful, and supporting it might require
duplicating much of strftime's code.)

Windows doesn't support that (unsurprisingly), but it accepts %#z,
which does the same as %z.  Let's wait for someone to request support
for modifiers and just document the behavior for now.

quoted

Changes from v1:
- Always handle %z internally.

Minor nitpick: Shouldn't the comment in strbuf.h be updated to reflect
that change?

quoted

+ * Add the time specified by `tm`, as formatted by `strftime`.  `tz_offset`
+ * and `tz_name` are used to expand %z and %Z internally, unless `tz_name`
+ * is NULL.  `tz_offset` is in decimal hhmm format, e.g. -600 means six
+ * hours west of Greenwich.

Yes, it should.  Thanks for paying attention! :)

René

There is no portable way to pass timezone information to strftime.  Add
parameters for timezone offset and name to strbuf_addftime and let it
handle the timezone-related format specifiers %z and %Z internally.

Callers can opt out for %Z by passing NULL as timezone name.  %z is
always handled internally -- this helps on Windows, where strftime would
expand it to a timezone name (same as %Z), in violation of POSIX.
Modifiers are not handled, e.g. %Ez is still passed to strftime.

Use an empty string as timezone name in show_date (the only current
caller) for now because we only have the timezone offset in non-local
mode.  POSIX allows %Z to resolve to an empty string in case of missing
information.

Helped-by: Ulrich Mueller [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Rene Scharfe <redacted>
---
Changes from v3:
- Updated developer documentation in strbuf.h.
- Added short note to user documentation.

 Documentation/rev-list-options.txt |  3 ++-
 date.c                             |  2 +-
 strbuf.c                           | 41 ++++++++++++++++++++++++++++++++++----
 strbuf.h                           | 10 ++++++++--
 t/t0006-date.sh                    |  6 ++++++
 5 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a46f70c2b1..34ae2553f1 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -768,7 +768,8 @@ timezone value.
 1970).  As with `--raw`, this is always in UTC and therefore `-local`
 has no effect.
 +
-`--date=format:...` feeds the format `...` to your system `strftime`.
+`--date=format:...` feeds the format `...` to your system `strftime`,
+except for %z and %Z, which are handled internally.
 Use `--date=format:%c` to show the date in your system locale's
 preferred format.  See the `strftime` manual for a complete list of
 format placeholders. When using `-local`, the correct syntax is

diff --git a/date.c b/date.c
index 63fa99685e..5580577334 100644
--- a/date.c
+++ b/date.c

@@ -246,7 +246,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			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);
+		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz, "");
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],

diff --git a/strbuf.c b/strbuf.c
index 00457940cf..be3b9e37b1 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -785,14 +785,48 @@ char *xstrfmt(const char *fmt, ...)
 	return ret;
 }
 
-void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
+void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
+		     int tz_offset, const char *tz_name)
 {
+	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
 	size_t len;
 
 	if (!*fmt)
 		return;
 
+	/*
+	 * There is no portable way to pass timezone information to
+	 * strftime, so we handle %z and %Z here.
+	 */
+	for (;;) {
+		const char *percent = strchrnul(fmt, '%');
+		strbuf_add(&munged_fmt, fmt, percent - fmt);
+		if (!*percent)
+			break;
+		fmt = percent + 1;
+		switch (*fmt) {
+		case '%':
+			strbuf_addstr(&munged_fmt, "%%");
+			fmt++;
+			break;
+		case 'z':
+			strbuf_addf(&munged_fmt, "%+05d", tz_offset);
+			fmt++;
+			break;
+		case 'Z':
+			if (tz_name) {
+				strbuf_addstr(&munged_fmt, tz_name);
+				fmt++;
+				break;
+			}
+			/* FALLTHROUGH */
+		default:
+			strbuf_addch(&munged_fmt, '%');
+		}
+	}
+	fmt = munged_fmt.buf;
+
 	strbuf_grow(sb, hint);
 	len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
 

@@ -804,17 +838,16 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 		 * output contains at least one character, and then drop the extra
 		 * character before returning.
 		 */
-		struct strbuf munged_fmt = STRBUF_INIT;
-		strbuf_addf(&munged_fmt, "%s ", fmt);
+		strbuf_addch(&munged_fmt, ' ');
 		while (!len) {
 			hint *= 2;
 			strbuf_grow(sb, hint);
 			len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
 				       munged_fmt.buf, tm);
 		}
-		strbuf_release(&munged_fmt);
 		len--; /* drop munged space */
 	}
+	strbuf_release(&munged_fmt);
 	strbuf_setlen(sb, sb->len + len);
 }
 

diff --git a/strbuf.h b/strbuf.h
index 80047b1bb7..4559035c47 100644
--- a/strbuf.h
+++ b/strbuf.h

@@ -340,8 +340,14 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * Add the time specified by `tm`, as formatted by `strftime`.
- */
-extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm);
+ * `tz_name` is used to expand %Z internally unless it's NULL.
+ * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
+ * of Greenwich, and it's used to expand %z internally.  However, tokens
+ * with modifiers (e.g. %Ez) are passed to `strftime`.
+ */
+extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
+			    const struct tm *tm, int tz_offset,
+			    const char *tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 42d4ea61ef..71082008f0 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh

@@ -51,6 +51,12 @@ check_show iso-local "$TIME" '2016-06-15 14:13:20 +0000'
 check_show raw-local "$TIME" '1466000000 +0000'
 check_show unix-local "$TIME" '1466000000'
 
+check_show 'format:%z' "$TIME" '+0200'
+check_show 'format-local:%z' "$TIME" '+0000'
+check_show 'format:%Z' "$TIME" ''
+check_show 'format:%%z' "$TIME" '%z'
+check_show 'format-local:%%z' "$TIME" '%z'
+
 # arbitrary time absurdly far in the future
 FUTURE="5758122296 -0400"
 check_show iso       "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT

-- 
2.13.0

On Thu, Jun 15, 2017 at 02:29:53PM +0200, René Scharfe wrote:

There is no portable way to pass timezone information to strftime.  Add
parameters for timezone offset and name to strbuf_addftime and let it
handle the timezone-related format specifiers %z and %Z internally.

Callers can opt out for %Z by passing NULL as timezone name.  %z is
always handled internally -- this helps on Windows, where strftime would
expand it to a timezone name (same as %Z), in violation of POSIX.
Modifiers are not handled, e.g. %Ez is still passed to strftime.

Use an empty string as timezone name in show_date (the only current
caller) for now because we only have the timezone offset in non-local
mode.  POSIX allows %Z to resolve to an empty string in case of missing
information.

Helped-by: Ulrich Mueller [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Rene Scharfe <redacted>
---
Changes from v3:
- Updated developer documentation in strbuf.h.
- Added short note to user documentation.

This looks good to me overall.

These check that the zone output is correct, but I don't think we ever
check that the value we feed to strftime is actually in the correct zone
in the first place (i.e., that %H shows the correct time).

I think that should go in a separate test from the %z/%Z handling, as
there are some subtleties.

So here are two patches on top of yours: more tests, and then the
format-local handling of %Z.

  [1/2]: t0006: check --date=format zone offsets
  [2/2]: date: use localtime() for "-local" time formats

 date.c          | 14 ++++++++++++--
 t/t0006-date.sh | 10 ++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

-Peff

We already test that "%z" and "%Z" show the right thing, but
we don't actually check that the time we display is the
correct one. Let's add two new tests:

  1. Test that "format:" shows the time in the author's
     timezone, just like the other time formats.

  2. Test that "format-local:" shows time in the local
     timezone. We don't want to use our normal UTC for this,
     because its offset is zero (so the result would be
     "correct" even if the code forgot to apply the offset
     or applied it in the wrong direction).

     We'll use the EST5 zone, which is already used
     elsewhere in the script (and so is assumed to be
     available everywhere).

Signed-off-by: Jeff King <redacted>
---
 t/t0006-date.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 71082008f..9f81bec7a 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh

@@ -31,9 +31,11 @@ check_show () {
 	format=$1
 	time=$2
 	expect=$3
-	test_expect_success $4 "show date ($format:$time)" '
+	prereqs=$4
+	zone=$5
+	test_expect_success $prereqs "show date ($format:$time)" '
 		echo "$time -> $expect" >expect &&
-		test-date show:$format "$time" >actual &&
+		TZ=${zone:-$TZ} test-date show:"$format" "$time" >actual &&
 		test_cmp expect actual
 	'
 }

@@ -57,6 +59,9 @@ check_show 'format:%Z' "$TIME" ''
 check_show 'format:%%z' "$TIME" '%z'
 check_show 'format-local:%%z' "$TIME" '%z'
 
+check_show 'format:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 16:13:20'
+check_show 'format-local:%Y-%m-%d %H:%M:%S' "$TIME" '2016-06-15 09:13:20' '' EST5
+
 # arbitrary time absurdly far in the future
 FUTURE="5758122296 -0400"
 check_show iso       "$FUTURE" "2152-06-19 18:24:56 -0400" TIME_IS_64BIT,TIME_T_IS_64BIT

-- 
2.13.1.766.g6bea926c5

When we convert seconds-since-epochs timestamps into a
broken-down "struct tm", we do so by adjusting the timestamp
according to the known offset and then using gmtime() to
break down the result. This means that the resulting struct
"knows" that it's in GMT, even though the time it represents
is adjusted for a different zone. The fields where it stores
this data are not portably accessible, so we have no way to
override them to tell them the real zone info.

For the most part, this works. Our date-formatting routines
don't pay attention to these inaccessible fields, and use
the same tz info we provided for adjustment. The one
exception is when we call strftime(), whose %Z format
reveals this hidden timezone data.

We solved that by always showing the empty string for %Z.
This is allowed by POSIX, but not very helpful to the user.
We can't make this work in the general case, as there's no
portable function for setting an arbitrary timezone (and
anyway, we don't have the zone name for the author zones,
only their offsets).

But for the special case of the "-local" formats, we can
just skip the adjustment and use localtime() instead of
gmtime(). This makes --date=format-local:%Z work correctly,
showing the local timezone instead of an empty string.

The new test checks the result for "UTC", our default
test-lib value for $TZ. Using something like EST5 might be
more interesting, but the actual zone string is
system-dependent (for instance, on my system it expands to
just EST). Hopefully "UTC" is vanilla enough that every
system treats it the same.

Signed-off-by: Jeff King <redacted>
---
I don't have a Windows system to test this on, but from the output Dscho
provided earlier, I believe this should pass.

 date.c          | 14 ++++++++++++--
 t/t0006-date.sh |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/date.c b/date.c
index 558057733..1fd6d6637 100644
--- a/date.c
+++ b/date.c

@@ -70,6 +70,12 @@ static struct tm *time_to_tm(timestamp_t time, int tz)
 	return gmtime(&t);
 }
 
+static struct tm *time_to_tm_local(timestamp_t time)
+{
+	time_t t = time;
+	return localtime(&t);
+}
+
 /*
  * What value of "tz" was in effect back then at "time" in the
  * local timezone?

@@ -214,7 +220,10 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 		return timebuf.buf;
 	}
 
-	tm = time_to_tm(time, tz);
+	if (mode->local)
+		tm = time_to_tm_local(time);
+	else
+		tm = time_to_tm(time, tz);
 	if (!tm) {
 		tm = time_to_tm(0, 0);
 		tz = 0;

@@ -246,7 +255,8 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			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, tz, "");
+		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
+				mode->local ? NULL : "");
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 9f81bec7a..7ac9466d5 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh

@@ -56,6 +56,7 @@ check_show unix-local "$TIME" '1466000000'
 check_show 'format:%z' "$TIME" '+0200'
 check_show 'format-local:%z' "$TIME" '+0000'
 check_show 'format:%Z' "$TIME" ''
+check_show 'format-local:%Z' "$TIME" 'UTC'
 check_show 'format:%%z' "$TIME" '%z'
 check_show 'format-local:%%z' "$TIME" '%z'
 

-- 
2.13.1.766.g6bea926c5

Am 15.06.2017 um 15:52 schrieb Jeff King:

But for the special case of the "-local" formats, we can
just skip the adjustment and use localtime() instead of
gmtime(). This makes --date=format-local:%Z work correctly,
showing the local timezone instead of an empty string.

Documentation/rev-list-options.txt should be updated to mention that %Z
is passed to strftime in the local case, no?

The new test checks the result for "UTC", our default
test-lib value for $TZ. Using something like EST5 might be
more interesting, but the actual zone string is
system-dependent (for instance, on my system it expands to
just EST). Hopefully "UTC" is vanilla enough that every
system treats it the same.

Signed-off-by: Jeff King <redacted>
---
I don't have a Windows system to test this on, but from the output Dscho
provided earlier, I believe this should pass.

The first patch applies with some fuzz on master of Git for Windows, the
second one applies cleanly.  A "typedef unsigned long timestamp_t;" is
required to compile it; such a fixup won't be needed for long, I guess.
t0006 succeeds.

René

On Thu, Jun 15, 2017 at 06:12:31PM +0200, René Scharfe wrote:

Am 15.06.2017 um 15:52 schrieb Jeff King:

quoted

But for the special case of the "-local" formats, we can
just skip the adjustment and use localtime() instead of
gmtime(). This makes --date=format-local:%Z work correctly,
showing the local timezone instead of an empty string.

Documentation/rev-list-options.txt should be updated to mention that %Z
is passed to strftime in the local case, no?

I wasn't sure if we wanted to get into that. Your documentation update
(with an empty string) says only that they are "handled internally".
While it is true that we aren't handling %Z internally anymore, the
point is that we try to do something sane which may or may not match
what your system strftime() does. And that continues to be the case
after my patch.

So unless we are going to give the full breakdown of when %Z is empty
and when it is not, I prefer to leave it unspecified.

quoted

I don't have a Windows system to test this on, but from the output Dscho
provided earlier, I believe this should pass.

The first patch applies with some fuzz on master of Git for Windows, the
second one applies cleanly.  A "typedef unsigned long timestamp_t;" is
required to compile it; such a fixup won't be needed for long, I guess.
t0006 succeeds.

Thanks for checking.

-Peff