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). > 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
