Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()

On Fri, Dec 08, 2017 at 06:29:34PM +0100, René Scharfe wrote:

quoted

By the way, I think there's another quite subtle leak in this function.
We do this:

   format_commit_message(commit, "%s", &sb, &ctx);
   strbuf_ltrim(&sb);

and then only use "sb" if sb.len is non-zero. But we may have actually
allocated to create our zero-length string (e.g., if we had a strbuf
full of spaces and trimmed them all off). Since we reuse "sb" over and
over as we loop, this will actually only leak once for the whole loop,
not once per iteration. So it's probably not a big deal, but writing it
with the explicit reset/release pattern fixes that (and is more
idiomatic for our code base, I think).

It's subtle, but I think it's not leaking, at least not in your example
case (and I can't think of another way).  IIUC format_subject(), which
handles the "%s" part, doesn't touch sb if the subject is made up only
of whitespace.

Yeah, I suspected that may be the case. But IMHO it is a poor use of
strbufs if you have to dig that far to see whether the code leaks or
not. The whole point of strbufs is to make string handling and memory
ownership more obviously correct.

Just skimming the history, I think it's mostly an artifact of the
function was slowly converted over the years.

-Peff