Re: [PATCH 3/3] introduce "format" date-mode

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 29, 2015, 22:22 UTC
Message-ID: <20150629222247.GA31607@flurp.local>
In-Reply-To: <20150625165545.GC23503@peff.net>

On Thu, Jun 25, 2015 at 12:55:45PM -0400, Jeff King wrote:

Show 40 quoted lines

> This feeds the format directly to strftime. Besides being a
> little more flexible, the main advantage is that your system
> strftime may know more about your locale's preferred format
> (e.g., how to spell the days of the week).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/strbuf.c b/strbuf.c
> index 0d4f4e5..a7ba028 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -709,3 +709,32 @@ char *xstrfmt(const char *fmt, ...)
> +void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
> +{
> +	size_t len;
> +
> +	/*
> +	 * strftime reports "0" if it could not fit the result in the buffer.
> +	 * Unfortunately, it also reports "0" if the requested time string
> +	 * takes 0 bytes. So if we were to probe and grow, we have to choose
> +	 * some arbitrary cap beyond which we guess that the format probably
> +	 * just results in a 0-length output. Since we have to choose some
> +	 * reasonable cap anyway, and since it is not that big, we may
> +	 * as well just grow to their in the first place.
> +	 */
> +	strbuf_grow(sb, 128);
> +	len = strftime(sb->buf + sb->len, sb->alloc - sb->len, fmt, tm);
> +
> +	if (!len) {
> +		/*
> +		 * Either we failed, or the format actually produces a 0-length
> +		 * output. There's not much we can do, so we leave it blank.
> +		 * However, the output array is left in an undefined state, so
> +		 * we must re-assert our NUL terminator.
> +		 */
> +		sb->buf[sb->len] = '\0';
> +	} else {
> +		sb->len += len;
> +	}
> +}

Clients of strbuf rightly expect the buffer to grow as needed in order to complete the requested operation. It is, therefore, both weird and expectation-breaking for strbuf_addftime() to lack this behavior. Worse, it doesn't even signal when the format has failed due to insufficient buffer space.

How about taking this approach (or something similar), instead, which grows the strbuf as needed?

--- 8< ---
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
{
	size_t len;
	struct strbuf f = STRBUF_INIT;

	/*
	 * This is a bit tricky since strftime returns 0 if the result did not
	 * fit in the supplied buffer, as well as when the formatted time has
	 * zero length. In the former case, we need to grow the buffer and try
	 * again. To distinguish between the two cases, we supply strftime with
	 * a format string one character longer than what the client supplied,
	 * which ensures that a successful format will have non-zero length,
	 * and then drop the extra character from the formatted time before
	 * returning.
	 */
	strbuf_addf(&f, "%s ", fmt);

	do {
		strbuf_grow(sb, 128);
		len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
			       f.buf, tm);
	} while (!len);
	strbuf_setlen(sb, sb->len + len - 1);

	strbuf_release(&f);
}
--- 8< ---

If this is performance critical code, then the augmented format string can be constructed with less expensive functions than strbuf_addf().