On Sun, Mar 09, 2025 at 09:03:21PM +0530, Ayush Chandekar wrote:
> Stop relying on global state to access the "core.attributesfile"
> configuration. Instead, store the value in `struct repo_settings` and
> retrieve it via `repo_settings_get_attributes_file_path()`.
>
> This prevents incorrect values from being used when a user or tool is
> handling multiple repositories in the same process, each with different
> attribute configurations. It also improves repository isolation and helps
> progress towards libification by avoiding unnecessary global state.
We typically switch the order around a bit in our commit messages: we
first explain what the actual problem is, and then we say how we fix it.
> diff --git a/attr.c b/attr.c
> index 0bd2750528..aec4b42245 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -879,12 +879,9 @@ const char *git_attr_system_file(void)
> return system_wide;
> }
>
> -const char *git_attr_global_file(void)
> +const char *git_attr_global_file(struct repository *repo)
> {
> - if (!git_attributes_file)
> - git_attributes_file = xdg_config_home("attributes");
> -
> - return git_attributes_file;
> + return repo_settings_get_attributesfile_path(repo);
> }
>
> int git_attr_system_is_enabled(void)
Hm. I wonder what the actual merit of this function is after the
refactoring. Right now there isn't really any as it is a direct wrapper
of `repo_settings_get_attributesfile_path()`.
> diff --git a/attr.h b/attr.h
> index a04a521092..c4f26b8f58 100644
> --- a/attr.h
> +++ b/attr.h
> @@ -213,11 +213,13 @@ void git_check_attr(struct index_state *istate,
> const char *path,
> struct attr_check *check);
>
> +struct repository;
> +
> /*
> * Retrieve all attributes that apply to the specified path.
> * check holds the attributes and their values.
> */
> -void git_all_attrs(struct index_state *istate,
> +void git_all_attrs(struct repository *repo, struct index_state *istate,
> const char *path, struct attr_check *check);
>
> enum git_attr_direction {
> @@ -233,7 +235,7 @@ void attr_start(void);
> const char *git_attr_system_file(void);
>
> /* Return the global gitattributes file, if any. */
> -const char *git_attr_global_file(void);
> +const char *git_attr_global_file(struct repository *repo);
>
> /* Return whether the system gitattributes file is enabled and should be used. */
> int git_attr_system_is_enabled(void);
I think it would make sense to split out this change into a separate
commit. The first commit would move the config into "repo-settings.c",
the second commit would adapt functions and their callers as necessary.
> @@ -283,4 +285,5 @@ struct match_attr {
> struct match_attr *parse_attr_line(const char *line, const char *src,
> int lineno, unsigned flags);
>
> +
> #endif /* ATTR_H */
Extraneous newline.
> diff --git a/builtin/check-attr.c b/builtin/check-attr.c
> index 7cf275b893..1b8a89dfb2 100644
> --- a/builtin/check-attr.c
> +++ b/builtin/check-attr.c
> @@ -70,7 +70,7 @@ static void check_attr(const char *prefix, struct attr_check *check,
> prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
>
> if (collect_all) {
> - git_all_attrs(the_repository->index, full_path, check);
> + git_all_attrs(the_repository, the_repository->index, full_path, check);
> } else {
> git_check_attr(the_repository->index, full_path, check);
> }
> diff --git a/builtin/var.c b/builtin/var.c
> index ada642a9fe..3d635c235e 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -69,9 +69,9 @@ static char *git_attr_val_system(int ident_flag UNUSED)
> return NULL;
> }
>
> -static char *git_attr_val_global(int ident_flag UNUSED)
> +static char *repo_git_attr_val_global(struct repository *repo, int ident_flag UNUSED)
> {
> - char *file = xstrdup_or_null(git_attr_global_file());
> + char *file = xstrdup_or_null(git_attr_global_file(repo));
> if (file) {
> normalize_path_copy(file, file);
> return file;
> @@ -79,6 +79,11 @@ static char *git_attr_val_global(int ident_flag UNUSED)
> return NULL;
> }
>
> +static char *git_attr_val_global(int ident_flag)
> +{
> + return repo_git_attr_val_global(the_repository, ident_flag);
> +}
> +
> static char *git_config_val_system(int ident_flag UNUSED)
> {
> if (git_config_system()) {
I think we should just retain `git_attr_val_global()` and plug in
`the_repository`. The extra change here doesn't add anything, and
"builtin/var.c" being a builtin means is not reused anywhere else,
either.
> diff --git a/repo-settings.c b/repo-settings.c
> index 9d16d5399e..420ca72f5f 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -167,3 +168,13 @@ int repo_settings_get_warn_ambiguous_refs(struct repository *repo)
> &repo->settings.warn_ambiguous_refs, 1);
> return repo->settings.warn_ambiguous_refs;
> }
> +
> +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");
> + }
> + }
We don't use curly braces around one-line statements.
> + return repo->settings.git_attributes_file;
> +}
One thing I'm missing is the code to `free()` the allocated memory in
`repo_settings_clear()`.
> \ No newline at end of file
Nit: missing newline at the end of the file.
Thanks!
Patrick
