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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/strbuf.c b/strbuf.c
index a7ba0281306e5dc271a446363946b6635a2dff15..e5e73700924e7a832424747edbb47988af7c2390 100644 (file)
--- 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 c7f368c77c057619a1bce93d0586c0c499699e4f..7c9bec76302259beebeea7521b9c74acfe6de264 100755 (executable)
--- 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