Re: format-patch subject-prefix gets truncated when using the --numbered flag
- From
Jeff King <peff@peff.net>
- Date
- Feb 28, 2017, 18:17 UTC
- Message-ID
- <20170228181719.t67abnnnsdcx2xfd@sigill.intra.peff.net>
- In-Reply-To
- <1488297556.2955.11.camel@enea.com>
On Tue, Feb 28, 2017 at 03:59:16PM +0000, Adrian Dudau wrote:
Show 26 quoted lines
> This is happening on the latest master branch, so I dug through the
> code and tracked the issue to this piece of code in log-tree.c:
>
> if (opt->total > 0) {
> static char buffer[64];
> snprintf(buffer, sizeof(buffer),
> "Subject: [%s%s%0*d/%d] ",
> opt->subject_prefix,
> *opt->subject_prefix ? " " : "",
> digits_in_number(opt->total),
> opt->nr, opt->total);
> subject = buffer;
> } else if (opt->total == 0 && opt->subject_prefix && *opt-
> >subject_prefix) {
> static char buffer[256];
> snprintf(buffer, sizeof(buffer),
> "Subject: [%s] ",
> opt->subject_prefix);
> subject = buffer;
> } else {
> subject = "Subject: ";
> }
>
> Apparently the size of the "buffer" var is different in the two
> situations. Anybody knows if this is by design or just an old
> oversight?I think it's just an old oversight. There are some other fixed-size buffers later, too, which could similarly truncate.
I think these should all be "static strbuf", and entering the function they should get strbuf_reset(), followed by a strbuf_addf(). The static strbufs will be the owners of the allocated heap memory, and it will get reused from call to call.
That stops the immediate problem. As a function interface, it's pretty ugly. It would probably make more sense for the caller to pass in a strbuf rather than have us pass out pointers to static storage. For the call in make_cover_letter(), that would be fine. For show_log(), it's less clear. That's called for every commit in "git log", which might be a little sensitive to allocations.
The only persistent storage it has is via the rev_info. Perhaps it could hold some scratch buffers for the traversal (though I don't think we currently do any resource-freeing when done with a rev_info, so it effectively becomes a leak).
-Peff