From: Phillip Wood Date: Mon, 12 Jan 2026 14:29:40 GMT Subject: Re: [Outreachy PATCH RFC 1/3] environment: stop storing `core.attributesFile` globally Message-ID: In-Reply-To: Hi Olamide On 12/01/2026 12:59, Olamide Caleb Bello wrote: > The config value parsed in git_default_core_config() is loaded eagerly > and stored in the global variable `git_attributes_file`. > Storing this value in a global variable can lead to unexpected > behaviours when more than one Git repository run in the same Git process. > > Move this value into a `struct config_values` which holds all the values > parsed by `git_default_config()` and can be accessed per > repository via `git_default_config()`. This will prevent us from > moving any code from git_default_core_config(), ensuring the current > behaviour remains the same while also enabling the libification of Git. The important thing is that we're not changing when the config is parsed, not that we're not removing code from git_default_core_config(). Looking at the changes below, I think it would be simpler to embed `struct config_values` in `struct repository` as we do for `struct repo_settings`. That would simplify things as we wouldn't have to mess about allocating an instance on the heap and freeing it in repo_clear(). I'd be tempted to call the new struct `repo_config` rather than `config_values` which is rather non-specific. I'm also not sure config.[ch] is the best home for it, maybe it should live in environment.[ch] for now - we might want to move it to it's own file at some point. Thanks Phillip > It is important to note that `git_default_config()` is a wrapper to other > `git_default_*_config()` such as `git_default_core_config()`. > Therefore to access and modify this global variable, > the change has to be made in the function which parses and > stores the value in the global variable. > > Suggested-by: Phillip Wood > Mentored-by: Christian Couder > Mentored-by: Usman Akinyemi > Signed-off-by: Olamide Caleb Bello > --- > attr.c | 8 +++++--- > config.c | 5 +++++ > config.h | 8 ++++++++ > environment.c | 7 ++++--- > environment.h | 1 - > repository.c | 6 ++++++ > repository.h | 4 ++++ > 7 files changed, 32 insertions(+), 7 deletions(-) > > diff --git a/attr.c b/attr.c > index 4999b7e09d..eb7b82707d 100644 > --- a/attr.c > +++ b/attr.c > @@ -881,10 +881,12 @@ const char *git_attr_system_file(void) > > const char *git_attr_global_file(void) > { > - if (!git_attributes_file) > - git_attributes_file = xdg_config_home("attributes"); > + struct config_values *cfg = the_repository->cfg_values; > > - return git_attributes_file; > + if (!cfg->attributes_file_path) > + cfg->attributes_file_path = xdg_config_home("attributes"); > + > + return cfg->attributes_file_path; > } > > int git_attr_system_is_enabled(void) > diff --git a/config.c b/config.c > index 7f6d53b473..8b882f64ae 100644 > --- a/config.c > +++ b/config.c > @@ -1761,6 +1761,11 @@ static int config_set_element_cmp(const void *cmp_data UNUSED, > return strcmp(e1->key, e2->key); > } > > +void config_values_clear(struct config_values *cfg) > +{ > + free(cfg->attributes_file_path); > +} > + > void git_configset_init(struct config_set *set) > { > hashmap_init(&set->config_hash, config_set_element_cmp, NULL, 0); > diff --git a/config.h b/config.h > index ba426a960a..1652d315e2 100644 > --- a/config.h > +++ b/config.h > @@ -135,6 +135,13 @@ struct config_context { > /* Config source metadata for key and value. */ > const struct key_value_info *kvi; > }; > + > +/* Holds values parsed by git_default_config() */ > +struct config_values { > + /* core config values */ > + char *attributes_file_path; > + > +}; > #define CONFIG_CONTEXT_INIT { 0 } > > /** > @@ -187,6 +194,7 @@ int git_config_from_blob_oid(config_fn_t fn, const char *name, > void git_config_push_parameter(const char *text); > void git_config_push_env(const char *spec); > int git_config_from_parameters(config_fn_t fn, void *data); > +void config_values_clear(struct config_values *cfg); > > /* > * Read config when the Git directory has not yet been set up. In case > diff --git a/environment.c b/environment.c > index a770b5921d..d633b0405b 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; > int zlib_compression_level = Z_BEST_SPEED; > int pack_compression_level = Z_DEFAULT_COMPRESSION; > int fsync_object_files = -1; > @@ -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 config_values *cfg = the_repository->cfg_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_path); > + return git_config_pathname(&cfg->attributes_file_path, var, value); > } > > if (!strcmp(var, "core.bare")) { > diff --git a/environment.h b/environment.h > index 51898c99cd..3512a7072e 100644 > --- a/environment.h > +++ b/environment.h > @@ -152,7 +152,6 @@ extern int assume_unchanged; > extern int warn_on_object_refname_ambiguity; > extern char *apply_default_whitespace; > extern char *apply_default_ignorewhitespace; > -extern char *git_attributes_file; > extern int zlib_compression_level; > extern int pack_compression_level; > extern unsigned long pack_size_limit_cfg; > diff --git a/repository.c b/repository.c > index c7e75215ac..3ad944e71c 100644 > --- a/repository.c > +++ b/repository.c > @@ -55,6 +55,7 @@ void initialize_repository(struct repository *repo) > repo->remote_state = remote_state_new(); > repo->parsed_objects = parsed_object_pool_new(repo); > ALLOC_ARRAY(repo->index, 1); > + CALLOC_ARRAY(repo->cfg_values, 1); > index_state_init(repo->index, repo); > repo->check_deprecated_config = true; > > @@ -403,6 +404,11 @@ void repo_clear(struct repository *repo) > FREE_AND_NULL(repo->remote_state); > } > > + if (repo->cfg_values) { > + config_values_clear(repo->cfg_values); > + FREE_AND_NULL(repo->cfg_values); > + } > + > strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e) > ref_store_release(e->value); > strmap_clear(&repo->submodule_ref_stores, 1); > diff --git a/repository.h b/repository.h > index 6063c4b846..5fb825f799 100644 > --- a/repository.h > +++ b/repository.h > @@ -13,6 +13,7 @@ struct object_database; > struct submodule_cache; > struct promisor_remote_config; > struct remote_state; > +struct config_values; > > enum ref_storage_format { > REF_STORAGE_FORMAT_UNKNOWN, > @@ -171,6 +172,9 @@ struct repository { > > /* Should repo_config() check for deprecated settings */ > bool check_deprecated_config; > + > + /* Repository's config values parsed by git_default_config() */ > + struct config_values *cfg_values; > }; > > #ifdef USE_THE_REPOSITORY_VARIABLE