Re: [PATCH] config: free resources of `struct config_store_data`

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.agren@gmail.com> wrote:
> On 13 May 2018 at 10:59, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.agren@gmail.com> wrote:
>>> Introduce and use a small helper function `config_store_data_clear()` to
>>> plug these leaks. This should be safe. The memory tracked here is config
>>> parser events. Once the users (`git_config_set_multivar_in_file_gently()`
>>> and `git_config_copy_or_rename_section_in_file()` at the moment) are
>>> done, no-one should be holding on to a pointer into this memory.
>>
>> A newcomer to this code (such as myself) might legitimately wonder why
>> store->key and store->value_regex are not also being cleaned up by this
>> function. An examination of the relevant code reveals that those structure
>> members are manually (and perhaps tediously) freed already by
>> git_config_set_multivar_in_file_gently(), however, if
>> config_store_data_clear() was responsible for freeing them, then
>> git_config_set_multivar_in_file_gently() could be made a bit less noisy.
>
> How about the following two patches as patches 2/3 and 3/3? I would also
> need to mention in the commit message of this patch (1/3) that the
> function will soon learn to clean up more members.
>
> I could of course squash the three patches into one, but there is some
> minor trickery involved, like we can't reuse a pointer in one place, but
> need to xstrdup it.
>
> Thank you for your comments. I'd be very interested in your thoughts on
> this.

Yep, making this a multi-part patch series and updating the commit
message of the patch which introduces config_store_data_clear(), as
you suggest, makes sense. The patch series could be organized
differently -- such as first moving freeing of 'value_regex' into new
config_store_data_clear(), then freeing additional fields in later
patches -- but I don't think it matters much in practice, so the
current organization is likely good enough.