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

From: Jeff King <peff@peff.net>
Date: Jun 2, 2017, 03:08 UTC
Message-ID: <20170602030825.hdpbaisn54d4fi4n@sigill.intra.peff.net>
In-Reply-To: <xmqq1sr3161p.fsf@gitster.mtv.corp.google.com>

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

Show 22 quoted lines

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

Show 11 quoted lines

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