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