On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.ag...@gmail.com> wrote: > On 13 May 2018 at 10:59, Eric Sunshine <sunsh...@sunshineco.com> wrote: >> On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.ag...@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.
