From: Eric Sunshine <sunshine@sunshineco.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
Git List <git@vger.kernel.org>,
"H.Merijn Brand" <h.m.brand@xs4all.nl>
Subject: Re: [PATCH 3/3] introduce "format" date-mode
Date: Mon, 20 Jul 2015 20:41:08 -0400 [thread overview]
Message-ID: <CAPig+cT_3pFH6XKBO1F2rbNyiGz5NgLMMiA_T+NWBroW5Sj7cg@mail.gmail.com> (raw)
In-Reply-To: <20150630132653.GA25742@peff.net>
On Tue, Jun 30, 2015 at 9:26 AM, Jeff King <peff@peff.net> wrote:
> 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).
While cleaning up old local branches, I noticed that, although the
jk/date-mode-format topic[1] made it into 'next' (and will be merged
to 'master' according to "What's cooking"[2]), the below follow-on
patch[3] which improves strbuf_addftime() never got picked up. Was
this omission intentional? Based upon the discussion[4], I was under
the impression that the patch was considered reasonably acceptable
(and did not worsen problems with bogus format strings -- which are
bogus anyway).
[1]: http://thread.gmane.org/gmane.comp.version-control.git/272658/focus=272695
[2]: http://news.gmane.org/gmane.comp.version-control.git
[3]: http://article.gmane.org/gmane.comp.version-control.git/273061
[4]: http://thread.gmane.org/gmane.comp.version-control.git/272658/focus=273026
> -- >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
>
next prev parent reply other threads:[~2015-07-21 0:41 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
2015-06-30 17:05 ` Eric Sunshine
2015-07-21 0:41 ` Eric Sunshine [this message]
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=CAPig+cT_3pFH6XKBO1F2rbNyiGz5NgLMMiA_T+NWBroW5Sj7cg@mail.gmail.com \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=h.m.brand@xs4all.nl \
--cc=peff@peff.net \
/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).