Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames

[Junio C Hamano <gitster@pobox.com>]

Martin Ågren <martin.agren@gmail.com> writes:

> files_transaction_prepare() and the functions it calls add strings to a
> string list without duplicating them, i.e., we keep the original raw
> pointers we were given. That is "ok", since we keep them only for a
> short-enough time, but we end up leaking some of them.

Sorry, but I do not understand this statement.  If affected_refnames
string list borrows strings from other structures who own them, and
none of these strings are freed by clearing affected_refnames list,
that is not "leaking"---we didn't acquire the ownership, so it is
not our job to free them in the first place.  Among the original
owners of strings we borrow from, some may not properly free, in
which case that is a leak.

What problem are you solving?

>
> Switch to duplicating the strings, so that affected_refnames does not
> leak memory. The original strings might still leak, but at least that
> can now be addressed without worrying about these pointers.
>
> No-one takes any pointers to the strings in the list (it is basically
> only used to check for set membership), so it is ok for
> string_list_clear to free them.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  refs/files-backend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 5cca55510..22daca2ba 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2407,7 +2407,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>  			       "ref_transaction_prepare");
>  	size_t i;
>  	int ret = 0;
> -	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> +	struct string_list affected_refnames = STRING_LIST_INIT_DUP;
>  	char *head_ref = NULL;
>  	int head_type;
>  	struct object_id head_oid;