From: Junio C Hamano Date: Thu, 29 Jan 2026 18:01:56 GMT Subject: Re: [Outreachy PATCH v5 1/3] environment: stop storing `core.attributesFile` globally Message-ID: In-Reply-To: Olamide Caleb Bello writes: > 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. > + if (!cfg->attributes_file) > + cfg->attributes_file = xdg_config_home("attributes"); > > - return git_attributes_file; > + return cfg->attributes_file; > } > 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. > 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. > @@ -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. > 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. > 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.