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

["Pablo Sabater" <pabloosabaterr@gmail.com>]

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