Re: [PATCH v9 5/8] convert: add 'working-tree-encoding' attribute

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 6, 2018, 22:22 UTC
Message-ID: <CAPig+cQE0pKs-AMvh4GndyCXBMnx=70jPpDM6K4jJTe-74FecQ@mail.gmail.com>
In-Reply-To: <570D707A-DD9E-4397-8155-E8B3C3D09760@gmail.com>

On Tue, Mar 6, 2018 at 5:13 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 22 quoted lines

>> On 06 Mar 2018, at 21:42, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Mar 4, 2018 at 3:14 PM,  <lars.schneider@autodesk.com> 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).