git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	"H.Merijn Brand" <h.m.brand@xs4all.nl>,
	git@vger.kernel.org
Subject: Re: [PATCH 3/3] introduce "format" date-mode
Date: Tue, 30 Jun 2015 09:26:53 -0400	[thread overview]
Message-ID: <20150630132653.GA25742@peff.net> (raw)
In-Reply-To: <20150629222247.GA31607@flurp.local>

On Mon, Jun 29, 2015 at 06:22:47PM -0400, Eric Sunshine wrote:

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

Here's a patch, on top of jk/date-mode-format (I think it would also be
fine to just squash into the tip commit; the explanation in the commit
message is sufficiently mirrored in the code comment).

-- >8 --
Subject: [PATCH] strbuf: make strbuf_addftime more robust

The return value of strftime is poorly designed; when it
returns 0, the caller cannot tell if the buffer was not
large enough, or if the output was actually 0 bytes. In the
original implementation of strbuf_addftime, we simply punted
and guessed that our 128-byte hint would be large enough.

We can do better, though, if we're willing to treat strftime
like less of a black box. We can munge the incoming format
to make sure that it never produces 0-length output, and
then "fix" the resulting output.  That lets us reliably grow
the buffer based on strftime's return value.

Clever-idea-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 strbuf.c                | 38 +++++++++++++++++++++-----------------
 t/t6300-for-each-ref.sh | 10 ++++++++++
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index a7ba028..e5e7370 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -712,29 +712,33 @@ char *xstrfmt(const char *fmt, ...)
 
 void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm)
 {
+	size_t hint = 128;
 	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);
+	if (!*fmt)
+		return;
+
+	strbuf_grow(sb, hint);
 	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.
+		 * 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 our strategy is to munge the format so that the
+		 * output contains at least one character, and then drop the extra
+		 * character before returning.
 		 */
-		sb->buf[sb->len] = '\0';
-	} else {
-		sb->len += len;
+		struct strbuf munged_fmt = STRBUF_INIT;
+		strbuf_addf(&munged_fmt, "%s ", fmt);
+		while (!len) {
+			hint *= 2;
+			strbuf_grow(sb, hint);
+			len = strftime(sb->buf + sb->len, sb->alloc - sb->len,
+				       munged_fmt.buf, tm);
+		}
+		strbuf_release(&munged_fmt);
+		len--; /* drop munged space */
 	}
+	strbuf_setlen(sb, sb->len + len);
 }
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index c7f368c..7c9bec7 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -235,6 +235,16 @@ test_expect_success 'Check format of strftime date fields' '
 	test_cmp expected actual
 '
 
+test_expect_success 'exercise strftime with odd fields' '
+	echo >expected &&
+	git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
+	test_cmp expected actual &&
+	long="long format -- $_z40$_z40$_z40$_z40$_z40$_z40$_z40" &&
+	echo $long >expected &&
+	git for-each-ref --format="%(authordate:format:$long)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<\EOF
 refs/heads/master
 refs/remotes/origin/master
-- 
2.5.0.rc0.336.g8460790

  parent reply	other threads:[~2015-06-30 13:27 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-25 11:19 several date related issues H.Merijn Brand
2015-06-25 12:44 ` Jeff King
2015-06-25 12:56   ` H.Merijn Brand
2015-06-25 16:53     ` [PATCH 0/3] localized date format Jeff King
2015-06-25 16:54       ` [PATCH 1/3] show-branch: use DATE_RELATIVE instead of magic number Jeff King
2015-06-25 16:55       ` [PATCH 2/3] convert "enum date_mode" into a struct Jeff King
2015-06-25 17:03         ` John Keeping
2015-06-25 17:22           ` Jeff King
2015-07-07 20:37         ` Junio C Hamano
2015-07-07 20:48           ` Jeff King
2015-07-07 21:05             ` Junio C Hamano
2015-07-07 21:13               ` Jeff King
2015-07-07 21:19                 ` Junio C Hamano
2015-06-25 16:55       ` [PATCH 3/3] introduce "format" date-mode Jeff King
2015-06-29 22:22         ` Eric Sunshine
2015-06-30 10:20           ` Jeff King
2015-06-30 16:22             ` Junio C Hamano
2015-06-30 17:50               ` Jeff King
2015-06-30 19:23                 ` Junio C Hamano
2015-06-30 19:33                   ` Jeff King
2015-06-30 16:58             ` Eric Sunshine
2015-06-30 17:58               ` Jeff King
2015-06-30 18:13                 ` Eric Sunshine
2015-06-30 19:22                   ` Jeff King
2015-06-30 17:05             ` Junio C Hamano
2015-06-30 17:48               ` Eric Sunshine
2015-06-30 19:17               ` Jeff King
2015-06-30 13:26           ` Jeff King [this message]
2015-06-30 17:05             ` Eric Sunshine
2015-07-21  0:41             ` Eric Sunshine
2015-07-21  1:19               ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150630132653.GA25742@peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=h.m.brand@xs4all.nl \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).