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

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

> 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 Fri, Jun 02, 2017 at 11:23:30AM +0900, Junio C Hamano wrote:

> René Scharfe  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

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

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

[Junio C Hamano]

René Scharfe  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(, mode->strftime_fmt, tm);
> + strbuf_addftime(, mode->strftime_fmt, tm, tz);
>   else
>   strbuf_addf(, "%.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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>>
>> Here are some links to past explorations:
>>
>>   http://public-inbox.org/git/20160208152858.ga17...@sigill.intra.peff.net/
>>
>>   http://public-inbox.org/git/87vb2d37ea@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

[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(, mode->strftime_fmt, tm);
+   strbuf_addftime(, mode->strftime_fmt, tm, tz);
else
strbuf_addf(, "%.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;
-   }
-
  

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  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.ga17...@sigill.intra.peff.net/
>
>   http://public-inbox.org/git/87vb2d37ea@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

[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.ga17...@sigill.intra.peff.net/

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

-Peff

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  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 +
> $ git --no-pager log -1 --pretty="%ad" --date="format-local:%F %T %z" 
> b06d364310
> 2017-05-09 16:26:02 +
>
> 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();
 }

+static struct tm *time_to_local_tm(timestamp_t time, int tz)
+{
+   time_t t = gm_time_t(time, tz);
+   return localtime();
+}
+
 /*
  * 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();

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.

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 +
$ git --no-pager log -1 --pretty="%ad" --date="format-local:%F %T %z" b06d364310
2017-05-09 16:26:02 +

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