Re: [PATCH v9 1/9] repository: introduce repo_config_values_clear()

[Tian Yuchen <cat@malon.dev>]

Hey Pablo,

On 7/12/26 17:28, Pablo Sabater wrote:
> On Sat Jul 11, 2026 at 8:35 PM CEST, Tian Yuchen wrote:
>> Hii Pablo,
>>
>> On 7/12/26 01:21, Pablo Sabater wrote:
>>> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>>>> As part of the ongoing libification effort, dynamically allocated
>>>> global configuration variables are being moved into
>>>> 'struct repo_config_values'. To prevent memory leaks, we need a
>>>> destructor to free these heap-allocated variables when a repository
>>>> instance is torn down.
>>>>
>>>> Introduce 'repo_config_values_clear()' in environment.c and invoke it
>>>> from 'repo_clear()' in repository.c. As a starting point, update this
>>>> new function to handle the cleanup of 'attributes_file'.
>>>
>>> Makes sense.
>>>
>>>>
>>>> Note:
>>>>
>>>> Submodules are currently not supported by repo_config_values(), which
>>>> explicitly BUG()s out if 'repo != the_repository'. Since repo_clear()
>>>> cleans up all repository instances, we must bypass them to prevent
>>>> crashing.
>>>>
>>>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>>>> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
>>>> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
>>>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>>>> ---
>>>>    environment.c | 19 +++++++++++++++++++
>>>>    environment.h |  9 +++++++++
>>>>    repository.c  |  1 +
>>>>    3 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/environment.c b/environment.c
>>>> index ba2c60103f..13677484de 100644
>>>> --- a/environment.c
>>>> +++ b/environment.c
>>>> @@ -726,3 +726,22 @@ void repo_config_values_init(struct repo_config_values *cfg)
>>>>    	cfg->sparse_expect_files_outside_of_patterns = 0;
>>>>    	cfg->warn_on_object_refname_ambiguity = 1;
>>>>    }
>>>> +
>>>> +void repo_config_values_clear(struct repository *repo)
>>>> +{
>>>> +	struct repo_config_values *cfg;
>>>> +
>>>> +	/*
>>>> +	 * NEEDSWORK: Submodules are currently not supported by
>>>> +	 * repo_config_values(), which explicitly BUG()s out if
>>>> +	 * repo != the_repository. Since repo_clear() cleans up all
>>>> +	 * repository instances, we must bypass them here to prevent
>>>> +	 * crashing.
>>>> +	 */
>>>> +	if (repo != the_repository)
>>>> +		return;
>>>> +
>>>> +	cfg = repo_config_values(repo);
>>>> +
>>>> +	FREE_AND_NULL(cfg->attributes_file);
>>>> +}
>>>> diff --git a/environment.h b/environment.h
>>>> index 6f18286955..c4a6a45704 100644
>>>> --- a/environment.h
>>>> +++ b/environment.h
>>>> @@ -135,6 +135,15 @@ int git_default_core_config(const char *var, const char *value,
>>>>
>>>>    void repo_config_values_init(struct repo_config_values *cfg);
>>>>
>>>> +/*
>>>> + * Frees memory allocated for dynamically loaded configuration values
>>>> + * inside `repo_config_values`.
>>>> + *
>>>> + * As dynamically allocated variables are migrated into this struct,
>>>> + * their FREE_AND_NULL() calls should be appended here.
>>>> + */
>>>> +void repo_config_values_clear(struct repository *repo);
>>>
>>> I think that I'm not comfortable having the _init() and the _clear()
>>> functions with different signatures.
>>>
>>> _clear() takes struct repository to dodge a BUG().
>>>
>>> I would like to have both signatures equal, why can't we just do directly:
>>>
>>>     void repo_config_values_clear(struct repo_config_values *cfg)
>>>     {
>>>     	FREE_AND_NULL(cfg->attributes_file);
>>>     }
>>>
>>> and call from repo_clear():
>>>
>>>     repo_config_values_clear(&repo->config_values_private_)
>>>
>>
>> I particularly agree with your point that the signatures of these two
>> functions should be consistent. I missed it tbh...I will change it in
>> the next reroll.
>>
>> However, I think it makes more sense to refactor to pass in 'struct
>> repository', which is consistent with repo-settings.
> 
> Whatever the signature is as long as it's symmetrical, is fine by me. However
> having 'struct repository' on _init() just passes the problem that
> _clear() had.
> 
> Let's look at initialize_repository():
> 
> 	void initialize_repository(struct repository *repo)
> 	{
> 		if (repo->initialized)
> 			BUG("repository initialized already");
> 		repo->initialized = true;
> 	[snip]
> 		repo_config_values_init(&repo->config_values_private_);
> 	[snip]
> 
> if we change the _init() signature to receive 'struct repository', how do
> we access our _private_ field? We cannot call repo_config_values() on
> repo_config_values_init() because initialize_repository() is also called for
> submodules (repo_init() -> initialize_repository()) so we would BUG() out.
> We would have to access the _private_ field directly at _init().
> 
> With _clear() accessing directly the _private_ field we can forget about
> the NEEDSWORK, it works for every repository.
> 
>>
>>> I get that the workaround might be to not access directly to
>>> &repo->config_values_private_ which repo_config_values() returns but for
>>> example initialize_repository() access this _private_ field directly as
>>> well.
>>
>> Now that we have used the _private_ suffix, if we can just define a
>> _clear() to bypass the assertion of repo_config_values(), wouldn't this
>> be self-deception? I'm not saying that the original lines are
>> necessarily correct... but I do think that semantically speaking, it is
>> inappropriate to pass in config_values_private_ to _clear().
> 
> I don't think it would be "self-deception": _init() and _clear() are the
> start and end of this _private_ field's lifecycle so having them receive
> 'struct repo_config_values' directly sounds reasonable to me.
> 
>>
>>> Also at repo_init(), initialize_repository() sets repo->initialized
>>> before anything can fail and call repo_clear() but
>>> repo_config_values_clear() should be able to free attributes_file even
>>> just after a memset() (which happens before initialize_repository()).
>>> But calling repo_config_values has a BUG() in case of
>>> !repo->initialized are we comfortable with this assert?
>>
>> This goes back to the previous topic: Who is responsible for the call to
>> _clear()? Who is responsible for filtering all those invalid usage of
>> repo instances? Faced with a repo instance that was not initialized but
>> was handed over to _clear() in some way, we have two concepts:
>>
>> - It doesn't matter. Since we always handle config_values_private_, it's
>> NULL at this point, so we don't BUG() and continue.
>>
>> - The very existence of such a repo is a mistake. It shouldn't have
>> appeared and shouldn't have been passed on to me. However, since this
>> situation is relatively common at this point, we choose to return
>> instead of BUG()ing it directly to temporarily avoid it. We will
>> gradually tighten the conditions. When the invalid calls are eliminated
>> in the end, such checks will no longer exist.
>>
>> Our consensus should at least be that this 'use of uninitialized repos'
>> is bad, so they are just two different ways to solve unexpected
>> situations. However, in my opinion, the difference between these two
>> concepts lies in whether we are consciously moving the assertion
>> downward. I think the latter line of thinking does this better.
> 
> Sounds reasonable, but I think I would go with the first thought of "It
> doesn't matter...".
> 
> Having _clear() access directly removes the need for tightening the
> conditions, because there would be none. The _clear() works for any repo
> as long as it has gone through a memset().
> 
> I think that the ones that should ensure that we don't have a "bad
> repository" should be the rest of the code and leave _clear() to just
> clear.
> 
>>
>>
>>   >
>>   > Even with the NEEDSWORK it is a silent return, what will happen when
>>   > submodules are supported? If no one remembers to change it we will leak
>>   > the submodules silently.
>>   >
>>
>> I will remember to change it ;)
>>
>> Regards, yuchen
>>
>>>> +
>>>>    /*
>>>>     * TODO: All the below state either explicitly or implicitly relies on
>>>>     * `the_repository`. We should eventually get rid of these and make the
>>>> diff --git a/repository.c b/repository.c
>>>> index 187dd471c4..b31f1b7852 100644
>>>> --- a/repository.c
>>>> +++ b/repository.c
>>>> @@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
>>>>    	FREE_AND_NULL(repo->parsed_objects);
>>>>
>>>>    	repo_settings_clear(repo);
>>>> +	repo_config_values_clear(repo);
>>>>
>>>>    	if (repo->config) {
>>>>    		git_configset_clear(repo->config);
>>>
>>> Regards,
>>> Pablo
> 
> Regards,
> Pablo

I thought about it again, and okay, I have to admit your approach is 
indeed more reasonable. Maybe it was just my stubbornness stemming from 
staying up late thinking last night. :D

Thanks! yuchen