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

From: Jeff King <peff@peff.net>
Date: Jun 7, 2017, 08:17 UTC
Message-ID: <20170607081729.6pz5yo2hmp4fwuas@sigill.intra.peff.net>
In-Reply-To: <a8b789e6-d0cd-6d96-1bfb-ccc5bc174013@web.de>

On Sat, Jun 03, 2017 at 12:40:34PM +0200, René Scharfe wrote:

Show 8 quoted lines

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

Show 5 quoted lines

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

Show 12 quoted lines

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