Re: [PATCH 13/22] builtin/fast-export: plug leaking tag names

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

On Wed, Aug 07, 2024 at 06:31:33PM +1000, James Liu wrote:
> On Tue Aug 6, 2024 at 7:00 PM AEST, Patrick Steinhardt wrote:
> > Refactor the code to make the lists we put those names into duplicate
> > the memory. This allows us to properly free the string as required and
> > thus plugs the memory leak.
> >
> > While this requires us to allocate more data overall, it shouldn't be
> > all that bad given that the number of allocations corresponds with the
> > number of command line parameters, which typically aren't all that many.
> 
> Ahh so using the `STRING_LIST_INIT_DUP` initialiser means that every
> time we call `string_list_append()` on the list, we retain ownership of
> the string and the list gets its own copy.
> 
> That means we're able to free our own copy later on.

Yes, exactly. I think that we really should change the naming though.
I've repeatedly seen the pattern that people think initializing the list
wtih `_NODUP` would transfer ownership of inserted strings. It does not
though, it simply assumes that the strings will be kept alive by the
caller.

This is made worse by the fact that we have `strvec_insert_nodup()`,
which _does_ transfer ownership. So we're using two different meanings
for "nodup", so I totally get why people are confused by this interface.

I'll leave that for a separate series though.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]