From: Phillip Wood <phillip.wood123@gmail•com>
To: Olamide Caleb Bello <belkid98@gmail•com>, git@vger•kernel.org
Cc: gitster@pobox•com, Christian Couder <christian.couder@gmail•com>,
Usman Akinyemi <usmanakinyemi202@gmail•com>,
Kaartic Sivaraam <kaartic.sivaraam@gmail•com>,
Taylor Blau <me@ttaylorr•com>,
Karthik Nayak <karthik.188@gmail•com>
Subject: Re: [Outreachy PATCH v2] environment: move "core.attributesFile" into repo-setting
Date: Mon, 5 Jan 2026 14:23:26 +0000 [thread overview]
Message-ID: <a881499d-e236-4f8e-a217-b6bce69e3e3c@gmail.com> (raw)
In-Reply-To: <aVfzMsN2ouY3UBFG@ubuntu>
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 <ayu.chandekar@gmail•com>
> Mentored-by: Christian Couder <christian.couder@gmail•com>
> Mentored-by: Usman Akinyemi <usmanakinyemi202@gmail•com>
> Signed-off-by: Olamide Caleb Bello <belkid98@gmail•com>
> ---
> 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 <ayu.chandekar@gmail•com>
> + Based-on-patch-by: Ayush Chandekar <ayu.chandekar@gmail•com>
> Mentored-by: Christian Couder <christian.couder@gmail•com>
> Mentored-by: Usman Akinyemi <usmanakinyemi202@gmail•com>
> Signed-off-by: Olamide Caleb Bello <belkid98@gmail•com>
> @@ 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 */
next prev parent reply other threads:[~2026-01-05 14:23 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-18 8:30 [Outreachy PATCH] environment: move "core.attributesFile" into repo-setting Olamide Caleb Bello
2025-12-18 17:31 ` Bello Olamide
2026-01-02 8:01 ` Bello Olamide
2026-01-02 8:49 ` Karthik Nayak
2026-01-02 8:48 ` Karthik Nayak
2026-01-02 11:26 ` Bello Olamide
2026-01-02 16:32 ` [Outreachy PATCH v2] " Olamide Caleb Bello
2026-01-05 11:09 ` Karthik Nayak
2026-01-05 11:39 ` Bello Olamide
2026-01-05 14:23 ` Phillip Wood [this message]
2026-01-05 15:00 ` Phillip Wood
2026-01-05 22:28 ` Junio C Hamano
2026-01-06 9:33 ` Bello Olamide
2026-01-06 13:44 ` Bello Olamide
2026-01-07 10:26 ` Phillip Wood
2026-01-07 14:18 ` Phillip Wood
2026-01-07 15:33 ` Bello Olamide
2026-01-06 8:09 ` Bello Olamide
2026-01-05 22:24 ` Junio C Hamano
2026-01-07 10:17 ` Phillip Wood
2026-01-06 8:08 ` Bello Olamide
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a881499d-e236-4f8e-a217-b6bce69e3e3c@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=belkid98@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=kaartic.sivaraam@gmail$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=usmanakinyemi202@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox