Subject: Re: [PATCH v2 08/24] submodule--helper: add and use *_release() functions

From: Junio C Hamano <[email-masked]>
Date: Wed, 20 Jul 2022 10:06:21 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> 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.