From: Eric Sunshine Date: Tue, 06 Mar 2018 22:22:10 GMT Subject: Re: [PATCH v9 5/8] convert: add 'working-tree-encoding' attribute Message-ID: In-Reply-To: <570D707A-DD9E-4397-8155-E8B3C3D09760@gmail.com> On Tue, Mar 6, 2018 at 5:13 PM, Lars Schneider wrote: >> On 06 Mar 2018, at 21:42, Eric Sunshine wrote: >> On Sun, Mar 4, 2018 at 3:14 PM, 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).