Re: [Outreachy PATCH v5 1/3] environment: stop storing `core.attributesFile` globally
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 29, 2026, 18:01 UTC
- Message-ID
- <xmqqv7gkb8mj.fsf@gitster.g>
- In-Reply-To
- <d28850bcdb5677ad0c81cf4bfa51ae1c056aabd3.1769256839.git.belkid98@gmail.com>
Olamide Caleb Bello <belkid98@gmail.com> writes:
Show 5 quoted lines
> const char *git_attr_global_file(void)
> {
> - if (!git_attributes_file)
> - git_attributes_file = xdg_config_home("attributes");
> + struct repo_config_values *cfg = &the_repository->config_values;Here, the_repository, being defined in repository.c as the address of a singleton "the_repo" instance, cannot be NULL even outside a repository, so taking the address of its config_values member is always safe. OK.
Show 6 quoted lines
> + if (!cfg->attributes_file)
> + cfg->attributes_file = xdg_config_home("attributes");
>
> - return git_attributes_file;
> + return cfg->attributes_file;
> }Show 21 quoted lines
> diff --git a/repository.h b/repository.h
> index 6063c4b846..638a142577 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;
> +
> /* Repository's reference storage format, as serialized on disk. */
> enum ref_storage_format ref_storage_format;And because this new config_values member is directly embedded in the repository structure, and "the_repo" instance is a global in BSS, its members are initialized exactly the same way as the global variables like git_attributes_file were initialized. Good.
Show 9 quoted lines
> diff --git a/environment.c b/environment.c > index a770b5921d..72735d9e4b 100644 > --- a/environment.c > +++ b/environment.c > @@ -53,7 +53,6 @@ char *git_commit_encoding; > char *git_log_output_encoding; > char *apply_default_whitespace; > char *apply_default_ignorewhitespace; > -char *git_attributes_file;
And we lose this global, that used to be zero-initialized for being in BSS.
Show 24 quoted lines
> @@ -327,6 +326,8 @@ static enum fsync_component parse_fsync_components(const char *var, const char *
> static int git_default_core_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb)
> {
> + struct repo_config_values *cfg = &the_repository->config_values;
> +
> /* This needs a better name */
> if (!strcmp(var, "core.filemode")) {
> trust_executable_bit = git_config_bool(var, value);
> @@ -364,8 +365,8 @@ static int git_default_core_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "core.attributesfile")) {
> - FREE_AND_NULL(git_attributes_file);
> - return git_config_pathname(&git_attributes_file, var, value);
> + FREE_AND_NULL(cfg->attributes_file);
> + return git_config_pathname(&cfg->attributes_file, var, value);
> }
>
> if (!strcmp(var, "core.bare")) {
> @@ -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;
> }And instead of assigning to the global git_attributes_file, we assign to the config_values.attributes_file member via the global "the_repository". No functional changes. OK.
> +void repo_config_values_init(struct repo_config_values *cfg)
> +{
> + cfg->attributes_file = NULL;
> +}This is not strictly needed, as git_attributes_file is left to be zero-initialized for being in BSS; its replacement, i.e., the_repo.config_values.attributes_file, will be zero-initialized the same way.
But other members we may want add later to the struct may need a place to initialize them. Or we can do a static initialization for the_repo in repository.c then we do not have to have this function and we do not have to call it. Either would work fine, as long as everybody calls initialize_repository() function, which is the only caller of this helper.
Show 12 quoted lines
> diff --git a/environment.h b/environment.h
> index 51898c99cd..0c0dcc6847 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -84,6 +84,11 @@ extern const char * const local_repo_env[];
>
> struct strvec;
>
> +struct repo_config_values {
> + /* section "core" config values */
> + char *attributes_file;
> +};OK.
Show 12 quoted lines
> diff --git a/repository.c b/repository.c > index c7e75215ac..d308cd78bf 100644 > --- a/repository.c > +++ b/repository.c > @@ -57,6 +57,7 @@ void initialize_repository(struct repository *repo) > ALLOC_ARRAY(repo->index, 1); > index_state_init(repo->index, repo); > repo->check_deprecated_config = true; > + repo_config_values_init(&repo->config_values); > > /* > * When a command runs inside a repository, it learns what
Continuing the discussion on repo_config_values_init(), currently, initialize_repository() is called by init_git(), which is called from "main()", so it should be fairly safe to assume that all in Git codebase will call repo_config_values_init().
But those who replace "main()" for their own libified use of Git code may not call init_git() hence initialize_repository() hence your repo_config_values_init(). In that sense, this is less safe than the other alternative.