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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2017, 21:47 UTC
Message-ID: <xmqq4lp2cisd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <b2238da3-9eba-1521-f4ca-3b805f103555@web.de>

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

Show 22 quoted lines

> Use string_list_append_nodup() instead of string_list_append() to hand
> over ownership of a detached strbuf and thus avoid leaking its memory.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/fmt-merge-msg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index 22034f87e7..8e8a15ea4a 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -377,7 +377,8 @@ static void shortlog(const char *name,
>  			string_list_append(&subjects,
>  					   oid_to_hex(&commit->object.oid));
>  		else
> -			string_list_append(&subjects, strbuf_detach(&sb, NULL));
> +			string_list_append_nodup(&subjects,
> +						 strbuf_detach(&sb, NULL));
>  	}
>  
>  	if (opts->credit_people)

What is leaked comes from strbuf, so the title is not a lie, but I tend to think that this leak is caused by a somewhat strange string_list API. The subjects string-list is initialized as a "dup" kind, but a caller that wants to avoid leaking can (and should) use _nodup() call to add a string without duping. It all feels a bit too convoluted.

The patch looks good.