On Wed, Jul 20 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> Add release functions for "struct module_list", "struct >> submodule_update_clone" and "struct update_data". > > OK, this does a lot of things, but in short, nobody bothered to free > module_list (i.e. list of cache entries borrowed from the index to > represent the paths we are interested in that are gitlinks), > update_data (which has module_list among other things that do not > need to be freed), and submodule_update_clone (which has > update_data, update_clone_data and failed_clones list) in the > original (in other words, these release helpers had to be invented > by this patch, not factored out from some codepaths that free them). > > I think the earlier part of the patch that deals with module_list is > correct. I also think the last one that clears update_data in > module_update() is correct. ... > I am not sure about the helper that releases suc, especially how > suc->update_data is left alone by update_submodules(). Presumably > the caller is responsible for releasing the resources held by > update_data member alone, but the interaction makes me feel dirty. > Luckily there is only one caller of suc_release(), so we can design > however dirty interface for convenience, but still it feels wrong to > design a release() helper that pretends to be usable by general > public, yet they have to be aware of the fact that some members in > the struct are still their responsibility to release. I dunno. But isn't this fine because that "update_data" member has a "const" on it: /* configuration parameters which are passed on to the children */ const struct update_data *update_data; I fix other similar ownership issues you pointed out for a re-roll I'm preparing, but I don't see why this one is a problem as long as that "const" is there, it's a slot we use for passing things in externally & to ferry it along.
