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

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

[Ulrich Mueller]

The following commands work as expected (using commit b06d364310
in the git://git.kernel.org/pub/scm/git/git.git repo as test case):

$ export TZ=Europe/Berlin
$ git --no-pager log -1 --pretty="%ad" --date=iso b06d364310
2017-05-09 23:26:02 +0900
$ git --no-pager log -1 --pretty="%ad" --date=iso-local b06d364310
2017-05-09 16:26:02 +0200

However, if I use explicit format: then the output of the time zone is
wrong:

$ git --no-pager log -1 --pretty="%ad" --date="format:%F %T %z" b06d364310
2017-05-09 23:26:02 +0000
$ git --no-pager log -1 --pretty="%ad" --date="format-local:%F %T %z" b06d364310
2017-05-09 16:26:02 +0000

I would expect the output to be the same as in the first two examples.

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

[Ævar Arnfjörð Bjarmason]

On Fri, May 26, 2017 at 8:33 PM, Ulrich Mueller <ulm@gentoo.org> wrote:
> The following commands work as expected (using commit b06d364310
> in the git://git.kernel.org/pub/scm/git/git.git repo as test case):
>
> $ export TZ=Europe/Berlin
> $ git --no-pager log -1 --pretty="%ad" --date=iso b06d364310
> 2017-05-09 23:26:02 +0900
> $ git --no-pager log -1 --pretty="%ad" --date=iso-local b06d364310
> 2017-05-09 16:26:02 +0200
>
> However, if I use explicit format: then the output of the time zone is
> wrong:
>
> $ git --no-pager log -1 --pretty="%ad" --date="format:%F %T %z" b06d364310
> 2017-05-09 23:26:02 +0000
> $ git --no-pager log -1 --pretty="%ad" --date="format-local:%F %T %z" b06d364310
> 2017-05-09 16:26:02 +0000
>
> I would expect the output to be the same as in the first two examples.

This patch solves half of your problem, i.e. makes the latter
format-local case work:

diff --git a/date.c b/date.c
index 63fa99685e..469306ebf5 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_local_tm(timestamp_t time, int tz)
+{
+       time_t t = gm_time_t(time, tz);
+       return localtime(&t);
+}
+
 /*
  * What value of "tz" was in effect back then at "time" in the
  * local timezone?
@@ -214,10 +220,14 @@ const char *show_date(timestamp_t time, int tz,
const struct date_mode *mode)
                return timebuf.buf;
        }

-       tm = time_to_tm(time, tz);
-       if (!tm) {
-               tm = time_to_tm(0, 0);
-               tz = 0;
+       if (mode->type == DATE_STRFTIME) {
+               tm = time_to_local_tm(time, tz);
+       } else {
+               tm = time_to_tm(time, tz);
+               if (!tm) {
+                       tm = time_to_tm(0, 0);
+                       tz = 0;
+               }
         }

        strbuf_reset(&timebuf);

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.

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

[Jeff King]

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

> 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. At least on _some_ platforms, the zone information is
embedded in "struct tm" and stored by gmtime() and localtime(), but the
fields aren't publicly accessible. Which is why your patch worked for
format-local (it swaps out gmtime() for localtime() which sets those
fields behind the scenes). But:

  - I'm not sure that's guaranteed by the standard; strftime() might get
    its zone information elsewhere (if it needs to reliably distinguish
    between gmtime() and localtime() results it has to at least set a
    bit in the "struct tm", but that bit may not be the full zone info).

  - Even if it does work, you're stuck with only the local timezone. In
    theory you could temporarily tweak the process's timezone, call
    localtime(), and then tweak it back. I was never able to get that to
    work (links below).

On glibc, at least, you can access the zone fields in "struct tm" by
compiling with _DEFAULT_SOURCE.

So I think the best we could do is probably to have a feature macro like
TM_HAS_GMTOFF, and set tm->tm_gmtoff and tm->tm_zone on platforms that
support it. I'm not sure what we'd put in the latter, though; we don't
actually have the timezone name at all (we just have "+0200" or whatever
we parsed from the git object, but that would be better than nothing).

That leaves other platforms still broken, but like I said, I don't think
there's a portable solution.

Here are some links to past explorations:

  http://public-inbox.org/git/20160208152858.GA17226@sigill.intra.peff.net/

  http://public-inbox.org/git/87vb2d37ea.fsf@web.de/

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Sat, May 27, 2017 at 11:46 PM, Jeff King <peff@peff.net> wrote:
> On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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. At least on _some_ platforms, the zone information is
> embedded in "struct tm" and stored by gmtime() and localtime(), but the
> fields aren't publicly accessible. Which is why your patch worked for
> format-local (it swaps out gmtime() for localtime() which sets those
> fields behind the scenes). But:
>
>   - I'm not sure that's guaranteed by the standard; strftime() might get
>     its zone information elsewhere (if it needs to reliably distinguish
>     between gmtime() and localtime() results it has to at least set a
>     bit in the "struct tm", but that bit may not be the full zone info).
>
>   - Even if it does work, you're stuck with only the local timezone. In
>     theory you could temporarily tweak the process's timezone, call
>     localtime(), and then tweak it back. I was never able to get that to
>     work (links below).
>
> On glibc, at least, you can access the zone fields in "struct tm" by
> compiling with _DEFAULT_SOURCE.
>
> So I think the best we could do is probably to have a feature macro like
> TM_HAS_GMTOFF, and set tm->tm_gmtoff and tm->tm_zone on platforms that
> support it. I'm not sure what we'd put in the latter, though; we don't
> actually have the timezone name at all (we just have "+0200" or whatever
> we parsed from the git object, but that would be better than nothing).
>
> That leaves other platforms still broken, but like I said, I don't think
> there's a portable solution.
>
> Here are some links to past explorations:
>
>   http://public-inbox.org/git/20160208152858.GA17226@sigill.intra.peff.net/
>
>   http://public-inbox.org/git/87vb2d37ea.fsf@web.de/

There's a third and possibly least shitty option that isn't covered in
those threads; We could just make a pass over the strftime format
ourselves and replace %z and %Z with the timezone (as done for
DATE_ISO8601_STRICT in date.c), then hand the rest off to strftime().

I'm not going to pursue it though, Ulrich, are you maybe interested in
hacking on that?

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

[René Scharfe]

Am 27.05.2017 um 23:46 schrieb Jeff King:
> On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
>> 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:

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1890d7a639..9cf2e4291d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -252,6 +252,11 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 {
 	const char *end;
 
+	if (*start == '%') {
+		strbuf_addch(sb, '%');
+		return 1;
+	}
+
 	if (*start != '(')
 		return 0;
 	end = strchr(start + 1, ')');
diff --git a/convert.c b/convert.c
index 8d652bf27c..8336fff694 100644
--- a/convert.c
+++ b/convert.c
@@ -399,6 +399,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[] = {
 		{ "f", NULL, },
+		{ "%", "%" },
 		{ NULL, NULL, },
 	};
 
diff --git a/daemon.c b/daemon.c
index ac7181a483..191fac2716 100644
--- a/daemon.c
+++ b/daemon.c
@@ -148,6 +148,9 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
 	case 'D':
 		strbuf_addstr(sb, context->directory);
 		return 1;
+	case '%':
+		strbuf_addch(sb, '%');
+		return 1;
 	}
 	return 0;
 }
diff --git a/date.c b/date.c
index 63fa99685e..d16a88eea5 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/ll-merge.c b/ll-merge.c
index ac0d4a5d78..e6202c7397 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -172,7 +172,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 {
 	char temp[4][50];
 	struct strbuf cmd = STRBUF_INIT;
-	struct strbuf_expand_dict_entry dict[6];
+	struct strbuf_expand_dict_entry dict[7];
 	struct strbuf path_sq = STRBUF_INIT;
 	const char *args[] = { NULL, NULL };
 	int status, fd, i;
@@ -185,7 +185,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 	dict[2].placeholder = "B"; dict[2].value = temp[2];
 	dict[3].placeholder = "L"; dict[3].value = temp[3];
 	dict[4].placeholder = "P"; dict[4].value = path_sq.buf;
-	dict[5].placeholder = NULL; dict[5].value = NULL;
+	dict[5].placeholder = "%"; dict[5].value = "%";
+	dict[6].placeholder = NULL; dict[6].value = NULL;
 
 	if (fn->cmdline == NULL)
 		die("custom merge driver %s lacks command line.", fn->name);
diff --git a/pretty.c b/pretty.c
index 587d48371b..56872bfa25 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1428,6 +1428,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
 	} magic = NO_MAGIC;
 
 	switch (placeholder[0]) {
+	case '%':
+		strbuf_addch(sb, '%');
+		return 1;
 	case '-':
 		magic = DEL_LF_BEFORE_EMPTY;
 		break;
diff --git a/strbuf.c b/strbuf.c
index 00457940cf..206dff6037 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -309,12 +309,6 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 			break;
 		format = percent + 1;
 
-		if (*format == '%') {
-			strbuf_addch(sb, '%');
-			format++;
-			continue;
-		}
-
 		consumed = fn(sb, format, context);
 		if (consumed)
 			format += consumed;
@@ -785,7 +779,28 @@ char *xstrfmt(const char *fmt, ...)
 	return ret;
 }
 
-void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
+struct date_format_context {
+	int tz;
+};
+
+static size_t expand_date_format(struct strbuf *sb, const char *placeholder,
+				 void *ctx)
+{
+	struct date_format_context *c = ctx;
+
+	switch (placeholder[0]) {
+	case '%':
+		strbuf_addstr(sb, "%%");
+		return 1;
+	case 'z':
+		strbuf_addf(sb, "%+05d", c->tz);
+		return 1;
+	}
+	return 0;
+}
+
+void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
+		     int tz)
 {
 	size_t hint = 128;
 	size_t len;
@@ -794,7 +809,10 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 		return;
 
 	strbuf_grow(sb, hint);
-	len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
+	if (strstr(fmt, "%z"))
+		len = 0;
+	else
+		len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
 
 	if (!len) {
 		/*
@@ -805,7 +823,10 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 		 * character before returning.
 		 */
 		struct strbuf munged_fmt = STRBUF_INIT;
-		strbuf_addf(&munged_fmt, "%s ", fmt);
+		struct date_format_context ctx;
+		ctx.tz = tz;
+		strbuf_expand(&munged_fmt, fmt, expand_date_format, &ctx);
+		strbuf_addch(&munged_fmt, ' ');
 		while (!len) {
 			hint *= 2;
 			strbuf_grow(sb, hint);
diff --git a/strbuf.h b/strbuf.h
index 80047b1bb7..fc4c796a2b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -281,10 +281,6 @@ extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
  * the length of the placeholder recognized and `strbuf_expand()` skips
  * over it.
  *
- * The format `%%` is automatically expanded to a single `%` as a quoting
- * mechanism; callers do not need to handle the `%` placeholder themselves,
- * and the callback function will not be invoked for this placeholder.
- *
  * All other characters (non-percent and not skipped ones) are copied
  * verbatim to the strbuf.  If the callback returned zero, meaning that the
  * placeholder is unknown, then the percent sign is copied, too.
@@ -339,9 +335,12 @@ __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`.
+ * Add the time specified by `tm` and `tz`, as formatted by `strftime`.
+ * The time zone offset is specified like hhmm, so e.g. -600 means six
+ * hours west of Greenwich.
  */
-extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm);
+extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
+			    const struct tm *tm, int tz);
 
 /**
  * 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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>
>> Here are some links to past explorations:
>>
>>   http://public-inbox.org/git/20160208152858.GA17226@sigill.intra.peff.net/
>>
>>   http://public-inbox.org/git/87vb2d37ea.fsf@web.de/
>
> There's a third and possibly least shitty option that isn't covered in
> those threads; We could just make a pass over the strftime format
> ourselves and replace %z and %Z with the timezone (as done for
> DATE_ISO8601_STRICT in date.c), then hand the rest off to strftime().

I do not know about %Z but certainly for %z that sounds the least
bad.

In a nearby message René seems to have come up with the same idea,
too ;-)

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 27.05.2017 um 23:46 schrieb Jeff King:
>> On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 
>>> 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.


>
> --- >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(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 1890d7a639..9cf2e4291d 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -252,6 +252,11 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
>  {
>  	const char *end;
>  
> +	if (*start == '%') {
> +		strbuf_addch(sb, '%');
> +		return 1;
> +	}
> +
>  	if (*start != '(')
>  		return 0;
>  	end = strchr(start + 1, ')');
> diff --git a/convert.c b/convert.c
> index 8d652bf27c..8336fff694 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -399,6 +399,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
>  	struct strbuf path = STRBUF_INIT;
>  	struct strbuf_expand_dict_entry dict[] = {
>  		{ "f", NULL, },
> +		{ "%", "%" },
>  		{ NULL, NULL, },
>  	};
>  
> diff --git a/daemon.c b/daemon.c
> index ac7181a483..191fac2716 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -148,6 +148,9 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
>  	case 'D':
>  		strbuf_addstr(sb, context->directory);
>  		return 1;
> +	case '%':
> +		strbuf_addch(sb, '%');
> +		return 1;
>  	}
>  	return 0;
>  }
> diff --git a/date.c b/date.c
> index 63fa99685e..d16a88eea5 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/ll-merge.c b/ll-merge.c
> index ac0d4a5d78..e6202c7397 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> @@ -172,7 +172,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
>  {
>  	char temp[4][50];
>  	struct strbuf cmd = STRBUF_INIT;
> -	struct strbuf_expand_dict_entry dict[6];
> +	struct strbuf_expand_dict_entry dict[7];
>  	struct strbuf path_sq = STRBUF_INIT;
>  	const char *args[] = { NULL, NULL };
>  	int status, fd, i;
> @@ -185,7 +185,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
>  	dict[2].placeholder = "B"; dict[2].value = temp[2];
>  	dict[3].placeholder = "L"; dict[3].value = temp[3];
>  	dict[4].placeholder = "P"; dict[4].value = path_sq.buf;
> -	dict[5].placeholder = NULL; dict[5].value = NULL;
> +	dict[5].placeholder = "%"; dict[5].value = "%";
> +	dict[6].placeholder = NULL; dict[6].value = NULL;
>  
>  	if (fn->cmdline == NULL)
>  		die("custom merge driver %s lacks command line.", fn->name);
> diff --git a/pretty.c b/pretty.c
> index 587d48371b..56872bfa25 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1428,6 +1428,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
>  	} magic = NO_MAGIC;
>  
>  	switch (placeholder[0]) {
> +	case '%':
> +		strbuf_addch(sb, '%');
> +		return 1;
>  	case '-':
>  		magic = DEL_LF_BEFORE_EMPTY;
>  		break;
> diff --git a/strbuf.c b/strbuf.c
> index 00457940cf..206dff6037 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -309,12 +309,6 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
>  			break;
>  		format = percent + 1;
>  
> -		if (*format == '%') {
> -			strbuf_addch(sb, '%');
> -			format++;
> -			continue;
> -		}
> -
>  		consumed = fn(sb, format, context);
>  		if (consumed)
>  			format += consumed;
> @@ -785,7 +779,28 @@ char *xstrfmt(const char *fmt, ...)
>  	return ret;
>  }
>  
> -void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
> +struct date_format_context {
> +	int tz;
> +};
> +
> +static size_t expand_date_format(struct strbuf *sb, const char *placeholder,
> +				 void *ctx)
> +{
> +	struct date_format_context *c = ctx;
> +
> +	switch (placeholder[0]) {
> +	case '%':
> +		strbuf_addstr(sb, "%%");
> +		return 1;
> +	case 'z':
> +		strbuf_addf(sb, "%+05d", c->tz);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
> +		     int tz)
>  {
>  	size_t hint = 128;
>  	size_t len;
> @@ -794,7 +809,10 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
>  		return;
>  
>  	strbuf_grow(sb, hint);
> -	len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
> +	if (strstr(fmt, "%z"))
> +		len = 0;
> +	else
> +		len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
>  
>  	if (!len) {
>  		/*
> @@ -805,7 +823,10 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
>  		 * character before returning.
>  		 */
>  		struct strbuf munged_fmt = STRBUF_INIT;
> -		strbuf_addf(&munged_fmt, "%s ", fmt);
> +		struct date_format_context ctx;
> +		ctx.tz = tz;
> +		strbuf_expand(&munged_fmt, fmt, expand_date_format, &ctx);
> +		strbuf_addch(&munged_fmt, ' ');
>  		while (!len) {
>  			hint *= 2;
>  			strbuf_grow(sb, hint);
> diff --git a/strbuf.h b/strbuf.h
> index 80047b1bb7..fc4c796a2b 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -281,10 +281,6 @@ extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
>   * the length of the placeholder recognized and `strbuf_expand()` skips
>   * over it.
>   *
> - * The format `%%` is automatically expanded to a single `%` as a quoting
> - * mechanism; callers do not need to handle the `%` placeholder themselves,
> - * and the callback function will not be invoked for this placeholder.
> - *
>   * All other characters (non-percent and not skipped ones) are copied
>   * verbatim to the strbuf.  If the callback returned zero, meaning that the
>   * placeholder is unknown, then the percent sign is copied, too.
> @@ -339,9 +335,12 @@ __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`.
> + * Add the time specified by `tm` and `tz`, as formatted by `strftime`.
> + * The time zone offset is specified like hhmm, so e.g. -600 means six
> + * hours west of Greenwich.
>   */
> -extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm);
> +extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
> +			    const struct tm *tm, int tz);
>  
>  /**
>   * 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

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

[Jeff King]

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

> René Scharfe <l.s.r@web.de> writes:
> 
> > Am 27.05.2017 um 23:46 schrieb Jeff King:
> >> On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> 
> >>> 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.

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

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

[René Scharfe]

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é

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

[Jeff King]

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

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

[Ulrich Mueller]

>>>>> On Fri, 2 Jun 2017, Jeff King wrote:

> On Fri, Jun 02, 2017 at 07:25:43PM +0200, René Scharfe wrote:
>> 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).

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

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

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

[Jeff King]

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

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

[Ulrich Mueller]

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

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

[Jeff King]

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

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

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

[PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

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 <ulm@gentoo.org>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
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

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Ulrich Mueller]

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

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

Am 03.06.2017 um 15:13 schrieb Ulrich Mueller:
>>>>>> 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?

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é

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Jeff King]

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

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

[Jeff King]

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

> > 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 <peff@peff.net>
---
 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

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

Am 07.06.2017 um 10:17 schrieb Jeff King:
> On Sat, Jun 03, 2017 at 12:40:34PM +0200, René Scharfe wrote:
>> 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é

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 07.06.2017 um 10:17 schrieb Jeff King:
>> On Sat, Jun 03, 2017 at 12:40:34PM +0200, René Scharfe wrote:
>>> 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.

Is that only "for now"?  I have a feeling that it is fundamentally
impossible with the data we record.  When GMTOFF 9:00 is the only
thing we have for a timestamp, can we tell if we should label it as
JST (aka Asia/Tokyo) or KST (aka Asia/Seoul)?

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 12, 2017 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 07.06.2017 um 10:17 schrieb Jeff King:
>>> On Sat, Jun 03, 2017 at 12:40:34PM +0200, René Scharfe wrote:
>>>> 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.
>
> Is that only "for now"?  I have a feeling that it is fundamentally
> impossible with the data we record.  When GMTOFF 9:00 is the only
> thing we have for a timestamp, can we tell if we should label it as
> JST (aka Asia/Tokyo) or KST (aka Asia/Seoul)?

It's obviously not perfect for all the reasons mentioned in this
thread, but we already have a timezone->offset mapping in the
timezone_names variable in date.c, a good enough solution might be to
simply reverse that lookup when formatting %Z

Of course we can never know if you were in Tokyo or Seul from the info
in the commit object, but we don't need to, it's enough that we just
emit JST for +0900 and anyone reading the output has at least some
idea what +0900 maps to.

We could also simply replace "%Z" with the empty string, as the the
POSIX strftime() documentation allows for:
http://pubs.opengroup.org/onlinepubs/009695399/functions/strftime.html
("Replaced by the timezone name or abbreviation, or by no bytes if no
timezone information exists.").

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Ulrich Mueller]

>>>>> On Mon, 12 Jun 2017, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Jun 12, 2017 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>> 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.
>> 
>> Is that only "for now"?  I have a feeling that it is fundamentally
>> impossible with the data we record.  When GMTOFF 9:00 is the only
>> thing we have for a timestamp, can we tell if we should label it as
>> JST (aka Asia/Tokyo) or KST (aka Asia/Seoul)?

> It's obviously not perfect for all the reasons mentioned in this
> thread, but we already have a timezone->offset mapping in the
> timezone_names variable in date.c, a good enough solution might be to
> simply reverse that lookup when formatting %Z

That cannot work, because there is no unique mapping from offset to
timezone name. For a given offset there may be several timezone names,
or no name at all. (For example, if I decide to commit with my local
mean time which has an offset of +0033 then there won't be any
timezone name at all.)

> Of course we can never know if you were in Tokyo or Seul from the info
> in the commit object, but we don't need to, it's enough that we just
> emit JST for +0900 and anyone reading the output has at least some
> idea what +0900 maps to.

Please don't. Outputting invented information for something that
really isn't in the data is worse than outputting no information at
all.

> We could also simply replace "%Z" with the empty string, as the the
> POSIX strftime() documentation allows for:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/strftime.html
> ("Replaced by the timezone name or abbreviation, or by no bytes if no
> timezone information exists.").

IMHO the two viable possibilities are the empty string, or the same
information as for %z. But this has been said before in this thread.

tzdata2017b has the following data in its "etcetera" file, the last
column being the timezone name:

   Zone	    Etc/GMT 	     0	-    GMT

   Zone	    Etc/GMT-14	    14	-    +14
   Zone	    Etc/GMT-13	    13	-    +13
   [...]
   Zone	    Etc/GMT+12	   -12	-    -12

This would be at least some rationale for converting %Z to [+-]hhmm,
i.e., the same as for %z. (One could even think about [+-]hh if the
minutes part is zero, but this already looks like over-engineering
to me.)

Ulrich

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

Am 12.06.2017 um 18:16 schrieb Ævar Arnfjörð Bjarmason:
> On Mon, Jun 12, 2017 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Am 07.06.2017 um 10:17 schrieb Jeff King:
>>>> On Sat, Jun 03, 2017 at 12:40:34PM +0200, René Scharfe wrote:
>>>>> 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.
>>
>> Is that only "for now"?  I have a feeling that it is fundamentally
>> impossible with the data we record.  When GMTOFF 9:00 is the only
>> thing we have for a timestamp, can we tell if we should label it as
>> JST (aka Asia/Tokyo) or KST (aka Asia/Seoul)?

We could track the time zone on commit, e.g. in a new header.  I doubt
that the need for showing time zone names will be strong enough to go to
that length, though.

> It's obviously not perfect for all the reasons mentioned in this
> thread, but we already have a timezone->offset mapping in the
> timezone_names variable in date.c, a good enough solution might be to
> simply reverse that lookup when formatting %Z
> 
> Of course we can never know if you were in Tokyo or Seul from the info
> in the commit object, but we don't need to, it's enough that we just
> emit JST for +0900 and anyone reading the output has at least some
> idea what +0900 maps to.

I suspect that there will be different opinions, for cultural and
political reasons.

> We could also simply replace "%Z" with the empty string, as the the
> POSIX strftime() documentation allows for:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/strftime.html
> ("Replaced by the timezone name or abbreviation, or by no bytes if no
> timezone information exists.").

Yes, that's what the patch does.

René

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Jeff King]

On Mon, Jun 12, 2017 at 06:58:13PM +0200, René Scharfe wrote:

> > We could also simply replace "%Z" with the empty string, as the the
> > POSIX strftime() documentation allows for:
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/strftime.html
> > ("Replaced by the timezone name or abbreviation, or by no bytes if no
> > timezone information exists.").
> 
> Yes, that's what the patch does.

Yeah, after thinking about all that was said in this thread, the empty
string is the only thing that makes sense to me.

IMHO, your patch as-is is a strict improvement. Mine on top (to handle
the "-local" cases) is optional, but I think worth doing. It makes the
rule easy to understand and explain ("-local" makes %Z behave as
expected, and the non-local variants leave it blank because the
information isn't present).

-Peff

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 12, 2017 at 6:56 PM, Ulrich Mueller <ulm@gentoo.org> wrote:
>>>>>> On Mon, 12 Jun 2017, Ęvar Arnfjörš Bjarmason wrote:
>
>> On Mon, Jun 12, 2017 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> René Scharfe <l.s.r@web.de> writes:
>>>> 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.
>>>
>>> Is that only "for now"?  I have a feeling that it is fundamentally
>>> impossible with the data we record.  When GMTOFF 9:00 is the only
>>> thing we have for a timestamp, can we tell if we should label it as
>>> JST (aka Asia/Tokyo) or KST (aka Asia/Seoul)?
>
>> It's obviously not perfect for all the reasons mentioned in this
>> thread, but we already have a timezone->offset mapping in the
>> timezone_names variable in date.c, a good enough solution might be to
>> simply reverse that lookup when formatting %Z
>
> That cannot work, because there is no unique mapping from offset to
> timezone name. For a given offset there may be several timezone names,
> or no name at all. (For example, if I decide to commit with my local
> mean time which has an offset of +0033 then there won't be any
> timezone name at all.)

Yes, I understand that it's a 1:many not a 1:1 mapping. What I'm
saying is that %Z in the context of git-log means a human readable
timezone name, and we can just make a best-effort at guessing that by
picking 1/many out of the 1:many mapping.

This obviously doesn't give us the user's timezone, but when you're
browsing through git log you can get an approximate human-readable
idea of what timezone someone was located in, and that's the whole
point of %Z in the context of how it would be used via git-log, isn't
it?

>> Of course we can never know if you were in Tokyo or Seul from the info
>> in the commit object, but we don't need to, it's enough that we just
>> emit JST for +0900 and anyone reading the output has at least some
>> idea what +0900 maps to.
>
> Please don't. Outputting invented information for something that
> really isn't in the data is worse than outputting no information at
> all.

It's not invented information. %z being +0900 is the same thing as %Z
being JST since +0900 == JST, just because some other things are also
== +0900 that doesn't mean that JST is invalid or less useful than
+0900 or "" for the purposes of human-readable output.

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, Jun 12, 2017 at 6:56 PM, Ulrich Mueller <ulm@gentoo.org> wrote:
>
>> Please don't. Outputting invented information for something that
>> really isn't in the data is worse than outputting no information at
>> all.
>
> It's not invented information. %z being +0900 is the same thing as %Z
> being JST since +0900 == JST, just because some other things are also
> == +0900 that doesn't mean that JST is invalid or less useful than
> +0900 or "" for the purposes of human-readable output.

Ulrich is right.  The issue is not if JST is a possibly correct
answer.

GMT offset alone does not tell us anything about which one of the
zones that happen to share the same value is associated with the
committer (or author) time, and picking one at random to pretend
that we know more than we actually do is a disservice to the users.

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Jeff King]

On Mon, Jun 12, 2017 at 07:53:21PM +0200, Ævar Arnfjörð Bjarmason wrote:

> >> Of course we can never know if you were in Tokyo or Seul from the info
> >> in the commit object, but we don't need to, it's enough that we just
> >> emit JST for +0900 and anyone reading the output has at least some
> >> idea what +0900 maps to.
> >
> > Please don't. Outputting invented information for something that
> > really isn't in the data is worse than outputting no information at
> > all.
> 
> It's not invented information. %z being +0900 is the same thing as %Z
> being JST since +0900 == JST, just because some other things are also
> == +0900 that doesn't mean that JST is invalid or less useful than
> +0900 or "" for the purposes of human-readable output.

I think the main problem is that the mapping isn't just "JST->+0900".
It's a set of rules that depend on the specific time being converted. So
it's true that at some time t, +0900 may mean JST or KST or whatever.
But at a different time, those time zones will map to a different
offset.

So I think at best this is semantically confusing (the author was not in
JST, but just happened to be in a time zone that maps to the same thing
at that moment, and any attempt to extrapolate their zone at another
time is going to be wrong). And at worst the output of "git log" is
going to show the same user flip-flopping between zones as their commits
move around with respect to the (politically determined) zone changes.
For instance in my time zone I'd shift between EST and CST twice per
year due to DST. My real time zone is EST5EDT, or better still,
America/New_York (I don't live there, but the Olson database carves up
regions based on the most populous city in the zone).

-Peff

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 12, 2017 at 8:20 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Jun 12, 2017 at 07:53:21PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> Of course we can never know if you were in Tokyo or Seul from the info
>> >> in the commit object, but we don't need to, it's enough that we just
>> >> emit JST for +0900 and anyone reading the output has at least some
>> >> idea what +0900 maps to.
>> >
>> > Please don't. Outputting invented information for something that
>> > really isn't in the data is worse than outputting no information at
>> > all.
>>
>> It's not invented information. %z being +0900 is the same thing as %Z
>> being JST since +0900 == JST, just because some other things are also
>> == +0900 that doesn't mean that JST is invalid or less useful than
>> +0900 or "" for the purposes of human-readable output.
>
> I think the main problem is that the mapping isn't just "JST->+0900".
> It's a set of rules that depend on the specific time being converted. So
> it's true that at some time t, +0900 may mean JST or KST or whatever.
> But at a different time, those time zones will map to a different
> offset.

To the extent that it doesn't map correctly (e.g. due to summer time)
that's already a bug/understood limitation in the fuzzy TZ parser in
date.c, isn't it (dating back to Linus's "In my world, it's always
summer" comment in 89967023da).

> So I think at best this is semantically confusing (the author was not in
> JST, but just happened to be in a time zone that maps to the same thing
> at that moment, and any attempt to extrapolate their zone at another
> time is going to be wrong). And at worst the output of "git log" is
> going to show the same user flip-flopping between zones as their commits
> move around with respect to the (politically determined) zone changes.
> For instance in my time zone I'd shift between EST and CST twice per
> year due to DST. My real time zone is EST5EDT, or better still,
> America/New_York (I don't live there, but the Olson database carves up
> regions based on the most populous city in the zone).

I think everyone in this thread disagrees with what I think makes
sense for %Z, and I'm not going to argue the point, I just thought I'd
chime in because it seemed to me that the discussion had missed the
mapping we had in the timezone_names variable.

But FWIW and just to (I think fairly) summarize, here's where we differ.

You (and others) think that unless we can actually show the user's
time zone as it appeared on their machine %Z shouldn't show anything
at all, since it would be confusing. Fair enough.

I only ever use the time offset info to quickly make a mental note of
"oh +0200, this guy's in Europe", or "oh -0400 America East". Having
any info at all for %Z would allow me to easily replace that already
buggy mapping that exists in my head right now with something that's
at least a bit more accurate, e.g. I remember that +0900 is Japan, but
I can't now offhand recall what timezones India is at.

Now, obviously we can't know whether to translate e.g. -0400 to New
York or Santiago, but for the purposes of human readable output I
thought it would be more useful to guess than show nothing at all. So
for that purpose the most useful output of %Z *for me* would be just
showing a string with the 3 most notable cities/areas, weighted for
showing locations on different continents, e.g.:

  +0000 = Iceland, West Africa, Ittoqqortoormiit
  +0100 = London, Lisbon, Kinshasa
  ...
  +0900 = Tokyo, Seul, Manokwari
  ....
  -0900 = San Francisco, Vancouver, Tijuana
  ....
  -0600 = Denver, Managua, Calgary

Then I could run:

    git log --date=format-local:"%Y[...](%Z)"

And get output like:

    commit 826c06412e (HEAD -> master, origin/master, origin/HEAD)
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Fri Jun 2 15:07:36 2017 +0900 (San Francisco, Vancouver,
Tijuana etc.)

    Fifth batch for 2.14
    [...]
    commit 30d005c020
    Author: Jeff King <peff@peff.net>
    Date:   Fri May 19 08:59:34 2017 -0400 (New York, Havana, Santiago etc.)

        diff: use blob path for blob/file diffs

Which gives me a pretty good idea of where the people who are making
my colleges / collaborators who are making commits all over the world
are located, for the purposes of reinforcing the abstract numeric
mapping with a best-guess at what the location might be, or at least
something that's close to the actual location.

I'd definitely use a feature like that, and could hack it up on top of
the current patches if there wasn't vehement opposition to it. Maybe
the above examples change someone's mind, I can't think of a case
where someone's relying on %Z now for date-local, or at least cases
where something like the above wouldn't be more useful in practice
than just an empty string, but if nobody agrees so be it :)

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Jeff King]

On Mon, Jun 12, 2017 at 09:02:32PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > I think the main problem is that the mapping isn't just "JST->+0900".
> > It's a set of rules that depend on the specific time being converted. So
> > it's true that at some time t, +0900 may mean JST or KST or whatever.
> > But at a different time, those time zones will map to a different
> > offset.
> 
> To the extent that it doesn't map correctly (e.g. due to summer time)
> that's already a bug/understood limitation in the fuzzy TZ parser in
> date.c, isn't it (dating back to Linus's "In my world, it's always
> summer" comment in 89967023da).

To be honest, I find that whole timezone_names[] array pretty gross. It
gets basic stuff wrong like:

  $ t/helper/test-date parse '2017-02-01 00:00:00 PST8PDT'
  2017-02-01 00:00:00 PST8PDT -> 2017-02-01 00:00:00 -0700

That should be -0800 because it's winter (though the bigger problem is
that our parser is too stupid and just sees "PDT" at the end and guesses
it's summer).

I suspect nobody has complained because we generally encourage real
"-0800" names when specifying zones. Which is really just punting the
problem to whoever is generating the timestamp, but it works in
practice. And we use real localtime() for handling things in the current
zone, which is usually what's responsible for generating any such time.
So just using the local zone, both of these are right (notice the zone
difference in the summer):

  $ TZ=PST8PDT t/helper/test-date parse '2017-02-01 00:00:00'
  2017-02-01 00:00:00 -> 2017-02-01 00:00:00 -0800

  $ TZ=PST8PDT t/helper/test-date parse '2017-06-01 00:00:00'
  2017-06-01 00:00:00 -> 2017-06-01 00:00:00 -0700

It would be nice to fix that. In theory strptime is the solution, but
comments in date.c indicate that it doesn't work well (and I can well
believe that). Presumably there is some not-terrible date library out
there that really can parse/format in arbitrary time-zones (perl's
DateTime is pleasant to use, for example). But I don't know of a C
library offhand (and unless we can ship it with the source, I'd guess
that the extra dependency isn't worth it).

> I think everyone in this thread disagrees with what I think makes
> sense for %Z, and I'm not going to argue the point, I just thought I'd
> chime in because it seemed to me that the discussion had missed the
> mapping we had in the timezone_names variable.

I did actually forget we had it, though I don't think it changes my
opinion.

> But FWIW and just to (I think fairly) summarize, here's where we differ.
> 
> You (and others) think that unless we can actually show the user's
> time zone as it appeared on their machine %Z shouldn't show anything
> at all, since it would be confusing. Fair enough.
> 
> I only ever use the time offset info to quickly make a mental note of
> "oh +0200, this guy's in Europe", or "oh -0400 America East". Having
> any info at all for %Z would allow me to easily replace that already
> buggy mapping that exists in my head right now with something that's
> at least a bit more accurate, e.g. I remember that +0900 is Japan, but
> I can't now offhand recall what timezones India is at.

I can't either. But nor could I tell you which timezone abbreviations
they use. ;)

>     Author: Jeff King <peff@peff.net>
>     Date:   Fri May 19 08:59:34 2017 -0400 (New York, Havana, Santiago etc.)

But I don't live in any of those places. :)

I see your point, though. I wouldn't want that on, but I wouldn't have
an objection to turning that into a configurable feature. But I'm not
sure we would just want it plumbed through to %Z. It seems like you'd
potentially want it on for any dates that show the numeric zone.

The biggest headache would be managing the reverse-mapping to cities. It
would be nice if there were some way to access the system zone database,
but I doubt there's a portable way to do so. And besides, it would
probably come up with way too many options (e.g., even just in the US
-0500 encompasses not just what we normally think of as Eastern Time,
but lots of little localities who do or do not handle DST; we obviously
wouldn't want to mention them, but I don't know how we'd limit the tz
database entries to only "you know, the important ones").

-Peff

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

Am 12.06.2017 um 21:02 schrieb Ævar Arnfjörð Bjarmason:
> I only ever use the time offset info to quickly make a mental note of
> "oh +0200, this guy's in Europe", or "oh -0400 America East". Having
> any info at all for %Z would allow me to easily replace that already
> buggy mapping that exists in my head right now with something that's
> at least a bit more accurate, e.g. I remember that +0900 is Japan, but
> I can't now offhand recall what timezones India is at.
> 
> Now, obviously we can't know whether to translate e.g. -0400 to New
> York or Santiago, but for the purposes of human readable output I
> thought it would be more useful to guess than show nothing at all. So
> for that purpose the most useful output of %Z *for me* would be just
> showing a string with the 3 most notable cities/areas, weighted for
> showing locations on different continents, e.g.:
> 
>    +0000 = Iceland, West Africa, Ittoqqortoormiit
>    +0100 = London, Lisbon, Kinshasa
>    ...
>    +0900 = Tokyo, Seul, Manokwari
>    ....
>    -0900 = San Francisco, Vancouver, Tijuana
>    ....
>    -0600 = Denver, Managua, Calgary
> 
> Then I could run:
> 
>      git log --date=format-local:"%Y[...](%Z)"
> 
> And get output like:
> 
>      commit 826c06412e (HEAD -> master, origin/master, origin/HEAD)
>      Author: Junio C Hamano <gitster@pobox.com>
>      Date:   Fri Jun 2 15:07:36 2017 +0900 (San Francisco, Vancouver,
> Tijuana etc.)
> 
>      Fifth batch for 2.14
>      [...]
>      commit 30d005c020
>      Author: Jeff King <peff@peff.net>
>      Date:   Fri May 19 08:59:34 2017 -0400 (New York, Havana, Santiago etc.)
> 
>          diff: use blob path for blob/file diffs
> 
> Which gives me a pretty good idea of where the people who are making
> my colleges / collaborators who are making commits all over the world
> are located, for the purposes of reinforcing the abstract numeric
> mapping with a best-guess at what the location might be, or at least
> something that's close to the actual location.

Half the time this would be off by a zone in areas that use daylight
saving time, or you'd need to determine when DST starts and ends, but
since that depends on the exact time zone it will be tricky.

You could use military time zones, which are nice and easy to convert:
Alpha (UTC+1) to Mike (UTC+12) (Juliet is skipped), November (UTC-1) to 
Yankee (UTC-12), and Zulu time zone (UTC+0).  Downside: Most civilians
don't use them. :)  Also there are no names for zones that have an
offset of a fraction of an hour.

> I'd definitely use a feature like that, and could hack it up on top of
> the current patches if there wasn't vehement opposition to it. Maybe
> the above examples change someone's mind, I can't think of a case
> where someone's relying on %Z now for date-local, or at least cases
> where something like the above wouldn't be more useful in practice
> than just an empty string, but if nobody agrees so be it :)

Personally I don't have a use for time information; dates alone would
suffice for me -- so I certainly won't hold you back.  But I don't see
how it could be done.

René

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Linus Torvalds]

On Mon, Jun 12, 2017 at 2:10 PM, Jeff King <peff@peff.net> wrote:
>
> I suspect nobody has complained because we generally encourage real
> "-0800" names when specifying zones.

That's what any sane person uses, and it's what SMTP requiries.

The timezone names are a (bad) joke. If a human can't understand them
(and people don't), there's no point in thinking that a computer
should either.

So yes, timezones should strive very much to be numeric. The names are
an incomprehensible mess. Don't rely on them, and try to avoid using
them.

                  Linus

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 13, 2017 at 12:31 AM, René Scharfe <l.s.r@web.de> wrote:
> Am 12.06.2017 um 21:02 schrieb Ævar Arnfjörð Bjarmason:
>>
>> I only ever use the time offset info to quickly make a mental note of
>> "oh +0200, this guy's in Europe", or "oh -0400 America East". Having
>> any info at all for %Z would allow me to easily replace that already
>> buggy mapping that exists in my head right now with something that's
>> at least a bit more accurate, e.g. I remember that +0900 is Japan, but
>> I can't now offhand recall what timezones India is at.
>>
>> Now, obviously we can't know whether to translate e.g. -0400 to New
>> York or Santiago, but for the purposes of human readable output I
>> thought it would be more useful to guess than show nothing at all. So
>> for that purpose the most useful output of %Z *for me* would be just
>> showing a string with the 3 most notable cities/areas, weighted for
>> showing locations on different continents, e.g.:
>>
>>    +0000 = Iceland, West Africa, Ittoqqortoormiit
>>    +0100 = London, Lisbon, Kinshasa
>>    ...
>>    +0900 = Tokyo, Seul, Manokwari
>>    ....
>>    -0900 = San Francisco, Vancouver, Tijuana
>>    ....
>>    -0600 = Denver, Managua, Calgary
>>
>> Then I could run:
>>
>>      git log --date=format-local:"%Y[...](%Z)"
>>
>> And get output like:
>>
>>      commit 826c06412e (HEAD -> master, origin/master, origin/HEAD)
>>      Author: Junio C Hamano <gitster@pobox.com>
>>      Date:   Fri Jun 2 15:07:36 2017 +0900 (San Francisco, Vancouver,
>> Tijuana etc.)
>>
>>      Fifth batch for 2.14
>>      [...]
>>      commit 30d005c020
>>      Author: Jeff King <peff@peff.net>
>>      Date:   Fri May 19 08:59:34 2017 -0400 (New York, Havana, Santiago
>> etc.)
>>
>>          diff: use blob path for blob/file diffs
>>
>> Which gives me a pretty good idea of where the people who are making
>> my colleges / collaborators who are making commits all over the world
>> are located, for the purposes of reinforcing the abstract numeric
>> mapping with a best-guess at what the location might be, or at least
>> something that's close to the actual location.
>
> Half the time this would be off by a zone in areas that use daylight
> saving time, or you'd need to determine when DST starts and ends, but
> since that depends on the exact time zone it will be tricky.

Yes, not to beat this point to death, but to clarify since we still
seem to be talking about different things:

I'm not saying it isn't fuzzy and inaccurate, I'm saying there's a
use-case for human readable output where saying e.g. "somewhere New
York-ish" is more useful than nothing, even given summer time I'm
still going to know approximately what timezone it is, more so than
the offset number.

And that is, I would argue, the best thing to do when the user is via
%Z asking for a human-readable timezone-ish given a commit object,
which only has the offset.

> You could use military time zones, which are nice and easy to convert:
> Alpha (UTC+1) to Mike (UTC+12) (Juliet is skipped), November (UTC-1) to
> Yankee (UTC-12), and Zulu time zone (UTC+0).  Downside: Most civilians
> don't use them. :)  Also there are no names for zones that have an
> offset of a fraction of an hour.

Yes I think if we're going down the road of providing some
human-readable version of the offset this is near-useless since almost
nobody using Git is going to know these by heart.

>> I'd definitely use a feature like that, and could hack it up on top of
>> the current patches if there wasn't vehement opposition to it. Maybe
>> the above examples change someone's mind, I can't think of a case
>> where someone's relying on %Z now for date-local, or at least cases
>> where something like the above wouldn't be more useful in practice
>> than just an empty string, but if nobody agrees so be it :)
>
> Personally I don't have a use for time information; dates alone would
> suffice for me -- so I certainly won't hold you back.  But I don't see
> how it could be done.

It could be done by accepting that you're not going to get a perfect
solution but one that's good enough to aid the humans currently
reading the currently only numeric output.

My use-case for this is having colleges all over the world who create
commits using different time offsets. It's simply easier to at a
glance realize what office someone is in if "git log" is printing city
names in/near those timezones, even if none of those cities are where
those people are located.

Re: [PATCH] strbuf: let strbuf_addftime handle %z and %Z itself

[Ulrich Mueller]

>>>>> On Tue, 13 Jun 2017, René Scharfe wrote:

> Am 12.06.2017 um 21:02 schrieb Ævar Arnfjörð Bjarmason:
>> Which gives me a pretty good idea of where the people who are making
>> my colleges / collaborators who are making commits all over the world
>> are located, for the purposes of reinforcing the abstract numeric
>> mapping with a best-guess at what the location might be, or at least
>> something that's close to the actual location.

> Half the time this would be off by a zone in areas that use daylight
> saving time, or you'd need to determine when DST starts and ends, but
> since that depends on the exact time zone it will be tricky.

And sometimes it would be impossible since DST rules differ between
hemispheres. For example, there is Europe/Berlin vs Africa/Windhoek,
or America/Halifax vs America/Santiago.

> You could use military time zones, which are nice and easy to convert:
> Alpha (UTC+1) to Mike (UTC+12) (Juliet is skipped), November (UTC-1) to 
> Yankee (UTC-12), and Zulu time zone (UTC+0).  Downside: Most civilians
> don't use them. :)

Yeah, that would ensure complete confusion. :) You'd have "Quebec"
for -0400 whereas the city of Quebec is in zone -0500. Similar for
"Lima" denoting +1100.

> Also there are no names for zones that have an offset of a fraction
> of an hour.

There are also zones like Pacific/Tongatapu which have +1300 (and +1400
in summer).

All in all I think that Jeff's suggestion makes most sense: Expand %Z
to the timezone name for the -local case, when the name is readily
available. Otherwise, expand it to the empty string.

Ulrich

[PATCH v2] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

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 <ulm@gentoo.org>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
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

Re: [PATCH v2] strbuf: let strbuf_addftime handle %z and %Z itself

[Ulrich Mueller]

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

Re: [PATCH v2] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

Am 15.06.2017 um 13:27 schrieb Ulrich Mueller:
>>>>>> 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.)

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.

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

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

René

[PATCH v3] strbuf: let strbuf_addftime handle %z and %Z itself

[René Scharfe]

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 <ulm@gentoo.org>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
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

Re: [PATCH v3] strbuf: let strbuf_addftime handle %z and %Z itself

[Jeff King]

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 <ulm@gentoo.org>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Changes from v3:
> - Updated developer documentation in strbuf.h.
> - Added short note to user documentation.

This looks good to me overall.

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

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

[PATCH 1/2] t0006: check --date=format zone offsets

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH 2/2] date: use localtime() for "-local" time formats

[Jeff King]

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 <peff@peff.net>
---
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

Re: [PATCH 2/2] date: use localtime() for "-local" time formats

[René Scharfe]

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 <peff@peff.net>
> ---
> 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é

Re: [PATCH 2/2] date: use localtime() for "-local" time formats

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> 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 <peff@peff.net>
>> ---
>> 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.

This time, I'll do s/timestamp_t/unsigned long/ while queuing ;-)
Of course, it needs to be converted back to timestamp_t when the
topic is merged to a more recent integration branch that knows about
the type.

Thanks.

Re: [PATCH 2/2] date: use localtime() for "-local" time formats

[Jeff King]

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

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

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.

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