Re: format-patch subject-prefix gets truncated when using the --numbered flag

From: Jeff King <peff@peff.net>
Date: Feb 28, 2017, 22:44 UTC
Message-ID: <20170228224424.p2po5giijeyvkhib@sigill.intra.peff.net>
In-Reply-To: <xmqqzih6jejg.fsf@gitster.mtv.corp.google.com>

On Tue, Feb 28, 2017 at 11:33:55AM -0800, Junio C Hamano wrote:

> And having said all that, if we really want to allow overlong
> subject prefix that would end up hiding the real title of the patch,
> a modern way to do so would be to use xstrfmt() like the attached
> toy-patch does.

If you are going to keep the ownership inside this function via statics, you should use a strbuf. That lets you avoid reallocating on each entry (and we call this once per commit in a traversal, though see below).

Show 10 quoted lines

> Note that this is merely a small first step---you'd
> notice that "subject" is kept around as a "static" and only freed
> upon entry to this function for the second time, to preserve the
> ownership model of the original code.  In a real "fix" (if this
> needs to be "fixed", that is), I think the ownership model of the
> storage used for *subject_p and *extra_headers_p needs to be updated
> so that it will become caller's responsibility to free them
> (similarly, the ownership model of opt->diffopt.stat_sep that is
> assigned the address of the static buffer[] in the same function
> needs to be revisited).

I agree the ownership model is ugly, and would be nicer if the caller passed in a strbuf to write into (or a pointer out-parameter, I guess, but I think strbufs make the ownership semantics much more obvious).

I would just worry about allocation overhead, but that is probably an overblown concern for format-patch. It tends to be called on a much smaller set of commits, and it is generally used with "-p", which creates a lot of overhead already.

-Peff