From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 04 Feb 2026 16:39:09 GMT
Subject: Re: [Outreachy PATCH v6 1/3] environment: stop storing `core.attributesFile` globally
Message-ID: <c95a7730-7b14-4be0-a4e4-861b2f5430ea@gmail.com>
In-Reply-To: <7e3082125df08d3e5fb2195d73698c4c28c6645e.1770127568.git.belkid98@gmail.com>

On 03/02/2026 15:42, Olamide Caleb Bello wrote:
> The `core.attributeFile` config value is parsed in
> git_default_core_config(), loaded eagerly and stored in the global
> variable `git_attributes_file`. Storing this value in a global variable
> can lead to it being overwritten by another repository when more than one
> Git repository run in the same Git process.
> 
> Create a new struct `repo_config_values` to hold this value and
> other repository dependent values parsed by `git_default_config()`.
> This will ensure the current behaviour remains the same while also
> enabling the libification of Git.
> 
> An accessor function 'repo_config_values()' is created and used to access
> the new struct member of the repository struct.
> This is to ensure that we detect if the struct repository has been
> initialized and also prevent double initialization of the repository.

Sounds sensible. This paragraph could be reflowed.

> It is important to note that `git_default_config()` is a wrapper to other
> `git_default_*_config()` functions such as `git_default_core_config()`.
> Therefore to access and modify this global variable,
> the change has to be made `git_default_core_config()`.

I'm not sure what this paragraph is saying with regard to the changes in 
this patch.

> --- a/environment.c
> +++ b/environment.c
> @@ -756,3 +757,8 @@ int git_default_config(const char *var, const char *value,
>   	/* Add other config variables here and to Documentation/config.adoc. */
>   	return 0;
>   }
> +
> +void repo_config_values_init(struct repo_config_values *cfg)
> +{
> +	cfg->attributes_file = NULL;
> +}

Should we be free()ing cfg->attributes_file when the repository instance 
is free()d? At the moment we're using "the_repository" which points to a 
static instance so it does not make any practical difference but once we 
start storing the config per-repository instance we will need to free 
the config when the repository instance is free()d.

> diff --git a/repository.c b/repository.c
> index c7e75215ac..a9b727540f 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -50,13 +50,25 @@ static void set_default_hash_algo(struct repository *repo)
>   	repo_set_hash_algo(repo, algo);
>   }
>   
> +struct repo_config_values *repo_config_values(struct repository *repo)
> +{
> +	if(!repo->initialized)
> +		BUG("config values from uninitialized repository");

This check and the one in initialize_repository() below assume that the 
repository instance is zeroed out when it is created, that's a 
reasonable requirement but we should probably document it as our other 
data structures tend not to require that they're zeroed out before they 
are initialized. For example

	struct strbuf buf;
	strbuf_init(&buf, 0);

is perfectly fine as strbuf_init() does not assume the instance passed 
to it has been zeroed out.

As we only support retrieving values from "the_repository" at the moment 
we should perhaps add

	if (repo != the_repository)
		BUG("trying to read config from wrong repository instance");

Everything else looks fine to me

Thanks

Phillip

> +	return &repo->config_values_private_;
> +}
> +
>   void initialize_repository(struct repository *repo)
>   {
> +	if (repo->initialized)
> +		BUG("repository initialized already");
> +	repo->initialized = true;
> +
>   	repo->remote_state = remote_state_new();
>   	repo->parsed_objects = parsed_object_pool_new(repo);
>   	ALLOC_ARRAY(repo->index, 1);
>   	index_state_init(repo->index, repo);
>   	repo->check_deprecated_config = true;
> +	repo_config_values_init(repo_config_values(repo));
>   
>   	/*
>   	 * When a command runs inside a repository, it learns what
> diff --git a/repository.h b/repository.h
> index 6063c4b846..9717e45000 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -3,6 +3,7 @@
>   
>   #include "strmap.h"
>   #include "repo-settings.h"
> +#include "environment.h"
>   
>   struct config_set;
>   struct git_hash_algo;
> @@ -148,6 +149,9 @@ struct repository {
>   	/* Repository's compatibility hash algorithm. */
>   	const struct git_hash_algo *compat_hash_algo;
>   
> +	/* Repository's config values parsed by git_default_config() */
> +	struct repo_config_values config_values_private_;
> +
>   	/* Repository's reference storage format, as serialized on disk. */
>   	enum ref_storage_format ref_storage_format;
>   
> @@ -171,6 +175,9 @@ struct repository {
>   
>   	/* Should repo_config() check for deprecated settings */
>   	bool check_deprecated_config;
> +
> +	/* Has this repository instance been initialized? */
> +	bool initialized;
>   };
>   
>   #ifdef USE_THE_REPOSITORY_VARIABLE