On Fri, Jun 02, 2017 at 11:23:30AM +0900, Junio C Hamano wrote:
> René Scharfe <l.s.r@xxxxxx> 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