Re: [PATCH 3/3] introduce "format" date-mode
flat view
From: Jeff King <hidden>
Date: 2016-06-15 23:05:35
Subsystem:
the rest · Maintainer:
Linus Torvalds
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 [off-list ref] Signed-off-by: Jeff King <redacted> --- 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