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

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

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 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.

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.

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?

> +
>  /*
>   * 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