From: Phillip Wood Date: Mon, 05 Jan 2026 14:23:26 GMT Subject: Re: [Outreachy PATCH v2] environment: move "core.attributesFile" into repo-setting Message-ID: In-Reply-To: Hi Olamide On 02/01/2026 16:32, Olamide Caleb Bello wrote: > When handling multiple repositories within the same process, relying on > global state for accessing the "core.attributesFile" configuration can > lead to incorrect values being used. It also makes it harder to isolate > repositories and hinders the libification of git. > The functions `bootstrap_attr_stack()` and `git_attr_val_system()` > retrieve "core.attributesFile" via `git_attr_global_file()` > which reads from global state `git_attributes_file`. > > Move the "core.attributesFile" configuration into the > `struct repo_settings` instead of relying on the global state. This changes when the config setting gets parsed which unfortunately regresses the user experience when the setting is invalid. If I run 'git -c core.attributesFile=~does-not-exist rebase -i' with git built from master it fails immediately with "fatal: failed to expand user dir in: '~does-not-exist'". With this patch applied it prompts me to edit the todo list and then fails when it tries to checkout the commit we're rebasing onto. Because "git rebase" expects reset_head() to return an error rather die if the checkout fails it is left in a strange state where only practical course of action for the user is to run "git rebase --abort". It is quite common that moving from parsing config settings eagerly by calling repo_config() at startup to parsing them lazily via 'stuct repo_settings' causes regressions like this. We really should find a way to address that before moving more settings into 'struct repo_settings' Thanks Phillip > A new function `repo_settings_get_attributesfile_path()` is added > and used to retrieve this setting in a repository-scoped manner. > The functions to retrieve "core.attributesFile" are replaced with > the new accessor function `repo_settings_get_attributesfile_path()` > This improves multi-repository behaviour and aligns with the goal of > libifying of Git. > > Note that in `bootstrap_attr_stack()`, the `index_state` is used only > if it exists, else we default to `the_repository`. > > Based-on-patch-by: Ayush Chandekar > Mentored-by: Christian Couder > Mentored-by: Usman Akinyemi > Signed-off-by: Olamide Caleb Bello > --- > The link to the GitHub CI is provided below > https://github.com/git/git/actions/runs/20661817110 > > Changes in v2: > -------------- > - Renamed the variable in the repo-settings struct to `attributes_file_path` > - Modified the comment section of the accessor function declaration to > indicate the core.attributesFile is read via repo config. > > Range diff vs v1: > ----------------- > 1: 4975f77cde ! 1: fc1dbec892 environment: move "core.attributesfile" into repo-setting > @@ Commit message > Note that in `bootstrap_attr_stack()`, the `index_state` is used only > if it exists, else we default to `the_repository`. > > - Reported-by: Ayush Chandekar > + Based-on-patch-by: Ayush Chandekar > Mentored-by: Christian Couder > Mentored-by: Usman Akinyemi > Signed-off-by: Olamide Caleb Bello > @@ attr.c: static void bootstrap_attr_stack(struct index_state *istate, > if (*stack) > return; > @@ attr.c: static void bootstrap_attr_stack(struct index_state *istate, > - push_stack(stack, e, NULL, 0); > } > > -- /* home directory */ > + /* home directory */ > - if (git_attr_global_file()) { > - e = read_attr_from_file(git_attr_global_file(), flags); > + if (istate && istate->repo) > @@ repo-settings.c: void repo_settings_clear(struct repository *r) > struct repo_settings empty = REPO_SETTINGS_INIT; > FREE_AND_NULL(r->settings.fsmonitor); > FREE_AND_NULL(r->settings.hooks_path); > -+ FREE_AND_NULL(r->settings.git_attributes_file); > ++ FREE_AND_NULL(r->settings.attributes_file_path); > r->settings = empty; > } > > @@ repo-settings.c: void repo_settings_reset_shared_repository(struct repository *r > } > +const char *repo_settings_get_attributesfile_path(struct repository *repo) > +{ > -+ if (!repo->settings.git_attributes_file) { > -+ if (repo_config_get_pathname(repo, "core.attributesfile", &repo->settings.git_attributes_file)) > -+ repo->settings.git_attributes_file = xdg_config_home("attributes"); > ++ if (!repo->settings.attributes_file_path) { > ++ if (repo_config_get_pathname(repo, "core.attributesfile", &repo->settings.attributes_file_path)) > ++ repo->settings.attributes_file_path = xdg_config_home("attributes"); > + } > -+ return repo->settings.git_attributes_file; > ++ return repo->settings.attributes_file_path; > +} > > ## repo-settings.h ## > @@ repo-settings.h: struct repo_settings { > unsigned long big_file_threshold; > > char *hooks_path; > -+ char *git_attributes_file; > ++ char *attributes_file_path; > }; > #define REPO_SETTINGS_INIT { \ > .shared_repository = -1, \ > @@ repo-settings.h: int repo_settings_get_shared_repository(struct repository *repo > +/* > + * Read the value for "core.attributesfile". > + * Defaults to xdg_config_home("attributes") if the core.attributesfile > -+ * isn't available. > ++ * which is set via repo config isn't available. > + */ > +const char *repo_settings_get_attributesfile_path(struct repository *repo); > + > > attr.c | 19 +++++++++---------- > attr.h | 3 --- > builtin/var.c | 2 +- > environment.c | 6 ------ > environment.h | 1 - > repo-settings.c | 10 ++++++++++ > repo-settings.h | 8 ++++++++ > 7 files changed, 28 insertions(+), 21 deletions(-) > > diff --git a/attr.c b/attr.c > index 4999b7e09d..b081400c18 100644 > --- a/attr.c > +++ b/attr.c > @@ -879,14 +879,6 @@ const char *git_attr_system_file(void) > return system_wide; > } > > -const char *git_attr_global_file(void) > -{ > - if (!git_attributes_file) > - git_attributes_file = xdg_config_home("attributes"); > - > - return git_attributes_file; > -} > - > int git_attr_system_is_enabled(void) > { > return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); > @@ -912,6 +904,8 @@ static void bootstrap_attr_stack(struct index_state *istate, > { > struct attr_stack *e; > unsigned flags = READ_ATTR_MACRO_OK; > + const char *attributes_file_path; > + struct repository *repo; > > if (*stack) > return; > @@ -927,8 +921,13 @@ static void bootstrap_attr_stack(struct index_state *istate, > } > > /* home directory */ > - if (git_attr_global_file()) { > - e = read_attr_from_file(git_attr_global_file(), flags); > + if (istate && istate->repo) > + repo = istate->repo; > + else > + repo = the_repository; > + attributes_file_path = repo_settings_get_attributesfile_path(repo); > + if (attributes_file_path) { > + e = read_attr_from_file(attributes_file_path, flags); > push_stack(stack, e, NULL, 0); > } > > diff --git a/attr.h b/attr.h > index a04a521092..956ce6ba62 100644 > --- a/attr.h > +++ b/attr.h > @@ -232,9 +232,6 @@ void attr_start(void); > /* Return the system gitattributes file. */ > const char *git_attr_system_file(void); > > -/* Return the global gitattributes file, if any. */ > -const char *git_attr_global_file(void); > - > /* Return whether the system gitattributes file is enabled and should be used. */ > int git_attr_system_is_enabled(void); > > diff --git a/builtin/var.c b/builtin/var.c > index cc3a43cde2..fd577f2930 100644 > --- a/builtin/var.c > +++ b/builtin/var.c > @@ -72,7 +72,7 @@ static char *git_attr_val_system(int ident_flag UNUSED) > > static char *git_attr_val_global(int ident_flag UNUSED) > { > - char *file = xstrdup_or_null(git_attr_global_file()); > + char *file = xstrdup_or_null(repo_settings_get_attributesfile_path(the_repository)); > if (file) { > normalize_path_copy(file, file); > return file; > diff --git a/environment.c b/environment.c > index a770b5921d..ed7d8f42d9 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; > @@ -363,11 +362,6 @@ static int git_default_core_config(const char *var, const char *value, > return 0; > } > > - if (!strcmp(var, "core.attributesfile")) { > - FREE_AND_NULL(git_attributes_file); > - return git_config_pathname(&git_attributes_file, var, value); > - } > - > if (!strcmp(var, "core.bare")) { > is_bare_repository_cfg = git_config_bool(var, value); > return 0; > 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/repo-settings.c b/repo-settings.c > index 195c24e9c0..cc53a3cd3b 100644 > --- a/repo-settings.c > +++ b/repo-settings.c > @@ -5,6 +5,7 @@ > #include "midx.h" > #include "pack-objects.h" > #include "setup.h" > +#include "path.h" > > static void repo_cfg_bool(struct repository *r, const char *key, int *dest, > int def) > @@ -158,6 +159,7 @@ void repo_settings_clear(struct repository *r) > struct repo_settings empty = REPO_SETTINGS_INIT; > FREE_AND_NULL(r->settings.fsmonitor); > FREE_AND_NULL(r->settings.hooks_path); > + FREE_AND_NULL(r->settings.attributes_file_path); > r->settings = empty; > } > > @@ -230,3 +232,11 @@ void repo_settings_reset_shared_repository(struct repository *repo) > { > repo->settings.shared_repository_initialized = 0; > } > +const char *repo_settings_get_attributesfile_path(struct repository *repo) > +{ > + if (!repo->settings.attributes_file_path) { > + if (repo_config_get_pathname(repo, "core.attributesfile", &repo->settings.attributes_file_path)) > + repo->settings.attributes_file_path = xdg_config_home("attributes"); > + } > + return repo->settings.attributes_file_path; > +} > diff --git a/repo-settings.h b/repo-settings.h > index d477885561..1209e1db83 100644 > --- a/repo-settings.h > +++ b/repo-settings.h > @@ -68,6 +68,7 @@ struct repo_settings { > unsigned long big_file_threshold; > > char *hooks_path; > + char *attributes_file_path; > }; > #define REPO_SETTINGS_INIT { \ > .shared_repository = -1, \ > @@ -99,4 +100,11 @@ int repo_settings_get_shared_repository(struct repository *repo); > void repo_settings_set_shared_repository(struct repository *repo, int value); > void repo_settings_reset_shared_repository(struct repository *repo); > > +/* > + * Read the value for "core.attributesfile". > + * Defaults to xdg_config_home("attributes") if the core.attributesfile > + * which is set via repo config isn't available. > + */ > +const char *repo_settings_get_attributesfile_path(struct repository *repo); > + > #endif /* REPO_SETTINGS_H */