Re: [PATCH v9 5/8] convert: add 'working-tree-encoding' attribute
- Date: Tue, 6 Mar 2018 17:22:10 -0500
- From: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
- Subject: Re: [PATCH v9 5/8] convert: add 'working-tree-encoding' attribute
On Tue, Mar 6, 2018 at 5:13 PM, Lars Schneider <larsxschneider@xxxxxxxxx> wrote:
>> On 06 Mar 2018, at 21:42, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Sun, Mar 4, 2018 at 3:14 PM, <lars.schneider@xxxxxxxxxxxx> wrote:
>>> + return xstrdup_toupper(value);
>> xstrdup_toupper() allocates memory...
>>> + const char *working_tree_encoding; /* Supported encoding or default encoding if NULL */
>> ...which is assigned to 'const char *'...
>>> + ca->working_tree_encoding = git_path_check_encoding(ccheck + 5);
>> ...by this code, and eventually leaked.
>> It's too bad it isn't cleaned up (freed), but looking at the callers,
>> fixing this leak would be mildly noisy (though not particularly
>> invasive). How much do we care about this leak?
> Hmm. You are right. That was previously handled by the encoding struct
> linked list that I removed in this iteration. I forgot about that aspect :/
> I don't like it leaking. I think I would like to reintroduce the linked
> list. This way every encoding is only once in memory. What do you think?
It's subjective, but I find the use of a linked-list just for the
purpose of not leaking these strings unnecessarily confusing.
If I was doing it, I'd probably add a conv_attrs_free() function and
call it from each function allocates a 'struct conv_attrs' (including
calling it before early returns -- which prompted my earlier comment
about it being a "mildly noisy" fix).