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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2017, 18:37 UTC
Message-ID: <xmqqd13p83sb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1654a696-73d5-c9ef-0fc2-bd82aaf2cabb@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 6 quoted lines

>> I'm not sure it's string-list's fault. Many callers (including this one)
>> ...
> The two modes (dup/nodup) make string_list code tricky.  Not sure
> how far we'd get with something simpler (e.g. an array of char pointers),
> but having the caller do all string allocations would make the code
> easier to analyze.

Yes.

It probably would have been more sensible if the API did not have two modes (instead, have the caller pass whatever string to be stored, *and* make the caller responsible for freeing them *if* it passed an allocated string).

For the push_refs_with_push() patch you sent, another possible fix would be to make cas_options a nodup kind so that the result of strbuf_detach() does not get an extra strdup to be lost when placed in cas_options. With the current string-list API that would not quite work, because freeing done in _release() is tied to the "dup/nodup" ness of the string list. I think there even is a codepath that initializes a string_list as nodup kind, stuffs string in it giving the ownership, and then flips it into dup kind just before calling _release() only to have it free the strings, or something silly/ugly like that.

In any case, the patch looks sensible. Thanks for plugging the leaks.