On Tue, Feb 28, 2017 at 03:59:16PM +0000, Adrian Dudau wrote:
> 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
