Re: [PATCH v1 2/2] environment: move excludes_file into repo_config_values

[Junio C Hamano <gitster@pobox.com>]

Tian Yuchen <cat@malon.dev> writes:

> Continue the libification effor by moving the 'excludes_file' global

"effort"?

> variable into 'struct repo_config_values'.
>
> Since 'excludes_file' is a dynamically allocated string (char *), it
> requires proper memory management. Introduce repo_config_values_clear()
> to safely free the heap memory when repository instance is destroyed.
>
> Note: 'if (repo != the_repository)' fallback logic is temporarily added
> in both the getter and the clear function. This prevents calling
> repo_config_values() on uninitialized submodules, which triggers BUG().
>
> 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 | 28 ++++++++++++++++++++++------
>  environment.h | 15 +++++++++++----
>  repository.c  |  1 +
>  3 files changed, 34 insertions(+), 10 deletions(-)
> ...
> @@ -733,3 +736,16 @@ 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;

Shouldn't cfg->excludes_file be explicitly initialized to NULL here
for completeness?  There are other explicit but redundant 0 assignment
to the members of this struct in the same function.

>  }
> +
> +void repo_config_values_clear(struct repository *repo)
> +{
> +	struct repo_config_values *cfg;
> +
> +	if (repo != the_repository)
> +		return;
> +
> +	cfg = repo_config_values(repo);
> +	if (!cfg)
> +		return;
> +	FREE_AND_NULL(cfg->excludes_file);
> +}
> diff --git a/environment.h b/environment.h
> index 52d531e4ea..2839913551 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -98,6 +98,7 @@ struct repo_config_values {
>  	int precomposed_unicode;
>  	int core_sparse_checkout_cone;
>  	int warn_on_object_refname_ambiguity;
> +	char *excludes_file;
>  
>  	/* section "sparse" config values */
>  	int sparse_expect_files_outside_of_patterns;
> @@ -133,13 +134,20 @@ int git_default_config(const char *, const char *,
>  int git_default_core_config(const char *var, const char *value,
>  			    const struct config_context *ctx, void *cb);
>  
> -/*
> - * TODO: This still relies on the global state.
> - */
>  const char *repo_excludes_file(struct repository *repo);

Good.

> +/*
> + * Frees memory allocated for dynamically loaded configuration values
> + * inside `repo_config_values`.
> + *
> + * Note: `excludes_file` is currently the only heap-allocated field in
> + * this struct. As other dynamically allocated variables are migrated,
> + * their FREE_AND_NULL() calls should be appended here.

Isn't attributes_file also heap-allocated member in this struct as well?