[PATCH] environment: move access to "core.attributesfile" into repo settings

[PATCH] environment: move access to "core.attributesfile" into repo settings

[Ayush Chandekar]

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.

Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
 attr.c               | 27 ++++++++++++---------------
 attr.h               |  7 +++++--
 builtin/check-attr.c |  2 +-
 builtin/var.c        |  9 +++++++--
 config.c             |  5 -----
 environment.c        |  1 -
 environment.h        |  1 -
 repo-settings.c      | 11 +++++++++++
 repo-settings.h      |  3 +++
 9 files changed, 39 insertions(+), 27 deletions(-)

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)
@@ -906,7 +903,7 @@ static void push_stack(struct attr_stack **attr_stack_p,
 	}
 }
 
-static void bootstrap_attr_stack(struct index_state *istate,
+static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate,
 				 const struct object_id *tree_oid,
 				 struct attr_stack **stack)
 {
@@ -927,8 +924,8 @@ 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 (git_attr_global_file(repo)) {
+		e = read_attr_from_file(git_attr_global_file(repo), flags);
 		push_stack(stack, e, NULL, 0);
 	}
 
@@ -946,7 +943,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
 	push_stack(stack, e, NULL, 0);
 }
 
-static void prepare_attr_stack(struct index_state *istate,
+static void prepare_attr_stack(struct repository *repo, struct index_state *istate,
 			       const struct object_id *tree_oid,
 			       const char *path, int dirlen,
 			       struct attr_stack **stack)
@@ -969,7 +966,7 @@ static void prepare_attr_stack(struct index_state *istate,
 	 * .gitattributes in deeper directories to shallower ones,
 	 * and finally use the built-in set as the default.
 	 */
-	bootstrap_attr_stack(istate, tree_oid, stack);
+	bootstrap_attr_stack(repo, istate, tree_oid, stack);
 
 	/*
 	 * Pop the "info" one that is always at the top of the stack.
@@ -1143,7 +1140,7 @@ static void determine_macros(struct all_attrs_item *all_attrs,
  * If check->check_nr is non-zero, only attributes in check[] are collected.
  * Otherwise all attributes are collected.
  */
-static void collect_some_attrs(struct index_state *istate,
+static void collect_some_attrs(struct repository *repo, struct index_state *istate,
 			       const struct object_id *tree_oid,
 			       const char *path, struct attr_check *check)
 {
@@ -1164,7 +1161,7 @@ static void collect_some_attrs(struct index_state *istate,
 		dirlen = 0;
 	}
 
-	prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack);
+	prepare_attr_stack(repo, istate, tree_oid, path, dirlen, &check->stack);
 	all_attrs_init(&g_attr_hashmap, check);
 	determine_macros(check->all_attrs, check->stack);
 
@@ -1310,7 +1307,7 @@ void git_check_attr(struct index_state *istate,
 	int i;
 	const struct object_id *tree_oid = default_attr_source();
 
-	collect_some_attrs(istate, tree_oid, path, check);
+	collect_some_attrs(the_repository, istate, tree_oid, path, check);
 
 	for (i = 0; i < check->nr; i++) {
 		unsigned int n = check->items[i].attr->attr_nr;
@@ -1321,14 +1318,14 @@ void git_check_attr(struct index_state *istate,
 	}
 }
 
-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)
 {
 	int i;
 	const struct object_id *tree_oid = default_attr_source();
 
 	attr_check_reset(check);
-	collect_some_attrs(istate, tree_oid, path, check);
+	collect_some_attrs(repo, istate, tree_oid, path, check);
 
 	for (i = 0; i < check->all_attrs_nr; i++) {
 		const char *name = check->all_attrs[i].attr->name;
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);
@@ -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 */
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()) {
diff --git a/config.c b/config.c
index 36f76fafe5..d483f1418c 100644
--- a/config.c
+++ b/config.c
@@ -1432,11 +1432,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.hookspath")) {
 		FREE_AND_NULL(git_hooks_path);
 		return git_config_pathname(&git_hooks_path, var, value);
diff --git a/environment.c b/environment.c
index e5b361bb5d..e1da5a69b7 100644
--- a/environment.c
+++ b/environment.c
@@ -42,7 +42,6 @@ char *git_commit_encoding;
 char *git_log_output_encoding;
 char *apply_default_whitespace;
 char *apply_default_ignorewhitespace;
-char *git_attributes_file;
 char *git_hooks_path;
 int zlib_compression_level = Z_BEST_SPEED;
 int pack_compression_level = Z_DEFAULT_COMPRESSION;
diff --git a/environment.h b/environment.h
index 2f43340f0b..9dc6bc0f3f 100644
--- a/environment.h
+++ b/environment.h
@@ -159,7 +159,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 char *git_hooks_path;
 extern int zlib_compression_level;
 extern int pack_compression_level;
diff --git a/repo-settings.c b/repo-settings.c
index 9d16d5399e..420ca72f5f 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -4,6 +4,7 @@
 #include "repository.h"
 #include "midx.h"
 #include "pack-objects.h"
+#include "path.h"
 
 static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
 			  int def)
@@ -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");
+		}
+	}
+	return repo->settings.git_attributes_file;
+}
\ No newline at end of file
diff --git a/repo-settings.h b/repo-settings.h
index 93ea0c3274..2a5ca5f07d 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -61,6 +61,7 @@ struct repo_settings {
 	size_t delta_base_cache_limit;
 	size_t packed_git_window_size;
 	size_t packed_git_limit;
+	char *git_attributes_file;
 };
 #define REPO_SETTINGS_INIT { \
 	.index_version = -1, \
@@ -78,5 +79,7 @@ void prepare_repo_settings(struct repository *r);
 enum log_refs_config repo_settings_get_log_all_ref_updates(struct repository *repo);
 /* Read the value for "core.warnAmbiguousRefs". */
 int repo_settings_get_warn_ambiguous_refs(struct repository *repo);
+/* Read the value for "core.attributesfile". */
+const char *repo_settings_get_attributesfile_path(struct repository *repo);
 
 #endif /* REPO_SETTINGS_H */
-- 
2.48.GIT

Re: [PATCH] environment: move access to "core.attributesfile" into repo settings

[Patrick Steinhardt]

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

Re: [PATCH] environment: move access to "core.attributesfile" into repo settings

[Ayush Chandekar]

Hey,
thanks for reviewing the patch!

> 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.

Got it.


> 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()`.

I can remove the function and replace all the instances with
`repo_settings_get_attributesfile_path()`. What do you think?

> 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.

Alright.

> Extraneous newline.
Apologies. Will fix it.

> 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.

Makes sense. I will drop `repo_git_attr_val_globa()` and keep
`git_attr_val_global()` with `the_repository`.


> We don't use curly braces around one-line statements.
Will fix it.

> One thing I'm missing is the code to `free()` the allocated memory in
> `repo_settings_clear()`.
Oh right.

> > \ No newline at end of file

Got it.

Thanks,
Ayush:)

Re: [PATCH] environment: move access to "core.attributesfile" into repo settings

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> 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.

A good suggestion.

>>  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()`.

Another thing that felt awkward about this change is actually
larger.  The current attributes globals are built around the notion
that the functions involved work on a single set of attributes at a
time.  Even in a single repository, when you are checking new contents
into the object database and when you are checking objects out of
the object database, you'd need to switch the direction manually,
which means you always have two sets of attributes active that you
can switch between (one is from the working tree and the other one
is from the index, if I am recalling correctly).

But step back and think.  What does it mean to make them belong to a
repository instance?  Whose index and working tree does the attribute
set that belongs to a repository that is not the_repository come from?

So it seems to me that removing the reliance on globals is a good
thing, and introducing some abstraction is a good step forward, but
it is dubious if the abstracted "set of attributes" should be part
of a repository instance.  This is a bit similar to the_index
situation, where we can have more than one in-core index instance
for a given repository we are working with, an index_state knows its
repository, but a repository instance only has a pointer to its
primary index_state instance and does not know other index_state
objects.  The right way to deal with in-core index is to pass
index_state, not repository, through the call chain for this reason,
and I suspect that we are better off to make sure that we do the
same for the attributes, i.e. pass pointer to an attribute set
instance through the callchain, not a repository.

Re: [PATCH] environment: move access to "core.attributesfile" into repo settings

[Ayush Chandekar]

> Another thing that felt awkward about this change is actually
> larger.  The current attributes globals are built around the notion
> that the functions involved work on a single set of attributes at a
> time.  Even in a single repository, when you are checking new contents
> into the object database and when you are checking objects out of
> the object database, you'd need to switch the direction manually,
> which means you always have two sets of attributes active that you
> can switch between (one is from the working tree and the other one
> is from the index, if I am recalling correctly).
>
> But step back and think.  What does it mean to make them belong to a
> repository instance?  Whose index and working tree does the attribute
> set that belongs to a repository that is not the_repository come from?

I'm trying my best to wrap my head around this. I definitely don't fully
understand your review yet since I'm still quite new to the codebase.
I get what you mean when you said which repo does it belong to.
But in the long term, isn’t our goal to get rid of the_repository anyway?
So at some point, wouldn't we need to either attach attributes to a
repository or have the attribute set know about its repository?

Thanks,
Ayush:)

Re: [PATCH] environment: move access to "core.attributesfile" into repo settings

[Junio C Hamano]

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

> But in the long term, isn’t our goal to get rid of the_repository anyway?
> So at some point, wouldn't we need to either attach attributes to a
> repository or have the attribute set know about its repository?

My point is that it may not help further the cause of removing the
assumption that certain operations only work on the_repository and
not on an arbitrary "struct repository" instance, to muck with the
attribute subsystem.  If it turns out that attribute data should not
belong to a repository instance, then it would not help to have the
globals moved to members of "struct repository" and pass a
repository instance down the code paths.  Rather, it may turn out
that we are better off passing a separate structure that is *NOT* a
"struct repository" that represents the set(s) of attributes down
the same code paths.

[GSOC PATCH v2 0/2] Stop depending on `the_repository` for core.attributesfile

[Ayush Chandekar]

This series moves access to the "core.attributesfile" configuration into
`repo_settings`, eliminating the dependency on the global `the_repository`
instance. It also updates the relevant attribute-related code paths to use
a repository-scoped accessor. This is a part of the ongoing effort towards
libification of git.

Ayush Chandekar (2):
  environment: move access to "core.attributesfile" into repo settings
  attr: use `repo_settings_get_attributesfile_path()` and update callers

 attr.c               | 28 ++++++++++------------------
 attr.h               |  7 +++----
 builtin/check-attr.c |  2 +-
 builtin/var.c        |  2 +-
 config.c             |  5 -----
 environment.c        |  1 -
 environment.h        |  1 -
 repo-settings.c      | 11 +++++++++++
 repo-settings.h      |  3 +++
 9 files changed, 29 insertions(+), 31 deletions(-)

-- 
2.48.GIT

[GSOC PATCH v2 1/2] environment: move access to "core.attributesfile" into repo settings

[Ayush Chandekar]

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.

Store the "core.attributesfile" configuration in the `repo_settings`
instead of relying on the global state. Add a new function
`repo_settings_get_attributesfile_path()` to retrieve this setting in a
repository-scoped manner.

Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
 config.c        |  5 -----
 environment.c   |  1 -
 environment.h   |  1 -
 repo-settings.c | 11 +++++++++++
 repo-settings.h |  3 +++
 5 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index 658569af08..b52ad3e3ad 100644
--- a/config.c
+++ b/config.c
@@ -1432,11 +1432,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.c b/environment.c
index 9e4c7781be..d7bf911ec5 100644
--- a/environment.c
+++ b/environment.c
@@ -42,7 +42,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;
diff --git a/environment.h b/environment.h
index 45e690f203..b7860eed3a 100644
--- a/environment.h
+++ b/environment.h
@@ -149,7 +149,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 size_t packed_git_window_size;
diff --git a/repo-settings.c b/repo-settings.c
index 67e9cfd2e6..17e60aa0d6 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)
@@ -148,6 +149,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.git_attributes_file);
 	r->settings = empty;
 }
 
@@ -207,3 +209,12 @@ 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.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");
+	}
+	return repo->settings.git_attributes_file;
+}
diff --git a/repo-settings.h b/repo-settings.h
index ddc11967e0..58dadd9dae 100644
--- a/repo-settings.h
+++ b/repo-settings.h
@@ -66,6 +66,7 @@ struct repo_settings {
 	size_t packed_git_limit;
 
 	char *hooks_path;
+	char *git_attributes_file;
 };
 #define REPO_SETTINGS_INIT { \
 	.shared_repository = -1, \
@@ -92,5 +93,7 @@ const char *repo_settings_get_hooks_path(struct repository *repo);
 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". */
+const char *repo_settings_get_attributesfile_path(struct repository *repo);
 
 #endif /* REPO_SETTINGS_H */
-- 
2.48.GIT

Re: [GSOC PATCH v2 1/2] environment: move access to "core.attributesfile" into repo settings

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

[snip]

> diff --git a/repo-settings.h b/repo-settings.h
> index ddc11967e0..58dadd9dae 100644
> --- a/repo-settings.h
> +++ b/repo-settings.h
> @@ -66,6 +66,7 @@ struct repo_settings {
>  	size_t packed_git_limit;
>
>  	char *hooks_path;
> +	char *git_attributes_file;
>  };
>  #define REPO_SETTINGS_INIT { \
>  	.shared_repository = -1, \
> @@ -92,5 +93,7 @@ const char *repo_settings_get_hooks_path(struct repository *repo);
>  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". */

Nit: Shouldn't we also mention that we default to
`xdg_config_home("attributes")` if the 'core.attributesfile' value isn't
available?

> +const char *repo_settings_get_attributesfile_path(struct repository *repo);
>
>  #endif /* REPO_SETTINGS_H */
> --
> 2.48.GIT

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[GSOC PATCH v2 2/2] attr: use `repo_settings_get_attributesfile_path()` and update callers

[Ayush Chandekar]

Update attribute-related functions to retrieve the "core.attributesfile"
configuration via the new repository-scoped accessor
`repo_settings_get_attributesfile_path()`. This improves behaviour in
multi-repository contexts and aligns with the goal of minimizing
reliance on global state.

Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
 attr.c               | 28 ++++++++++------------------
 attr.h               |  7 +++----
 builtin/check-attr.c |  2 +-
 builtin/var.c        |  2 +-
 4 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/attr.c b/attr.c
index 0bd2750528..8f28463e8c 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);
@@ -906,7 +898,7 @@ static void push_stack(struct attr_stack **attr_stack_p,
 	}
 }
 
-static void bootstrap_attr_stack(struct index_state *istate,
+static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate,
 				 const struct object_id *tree_oid,
 				 struct attr_stack **stack)
 {
@@ -927,8 +919,8 @@ 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 (repo_settings_get_attributesfile_path(repo)) {
+		e = read_attr_from_file(repo_settings_get_attributesfile_path(repo), flags);
 		push_stack(stack, e, NULL, 0);
 	}
 
@@ -946,7 +938,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
 	push_stack(stack, e, NULL, 0);
 }
 
-static void prepare_attr_stack(struct index_state *istate,
+static void prepare_attr_stack(struct repository *repo, struct index_state *istate,
 			       const struct object_id *tree_oid,
 			       const char *path, int dirlen,
 			       struct attr_stack **stack)
@@ -969,7 +961,7 @@ static void prepare_attr_stack(struct index_state *istate,
 	 * .gitattributes in deeper directories to shallower ones,
 	 * and finally use the built-in set as the default.
 	 */
-	bootstrap_attr_stack(istate, tree_oid, stack);
+	bootstrap_attr_stack(repo, istate, tree_oid, stack);
 
 	/*
 	 * Pop the "info" one that is always at the top of the stack.
@@ -1143,7 +1135,7 @@ static void determine_macros(struct all_attrs_item *all_attrs,
  * If check->check_nr is non-zero, only attributes in check[] are collected.
  * Otherwise all attributes are collected.
  */
-static void collect_some_attrs(struct index_state *istate,
+static void collect_some_attrs(struct repository *repo, struct index_state *istate,
 			       const struct object_id *tree_oid,
 			       const char *path, struct attr_check *check)
 {
@@ -1164,7 +1156,7 @@ static void collect_some_attrs(struct index_state *istate,
 		dirlen = 0;
 	}
 
-	prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack);
+	prepare_attr_stack(repo, istate, tree_oid, path, dirlen, &check->stack);
 	all_attrs_init(&g_attr_hashmap, check);
 	determine_macros(check->all_attrs, check->stack);
 
@@ -1310,7 +1302,7 @@ void git_check_attr(struct index_state *istate,
 	int i;
 	const struct object_id *tree_oid = default_attr_source();
 
-	collect_some_attrs(istate, tree_oid, path, check);
+	collect_some_attrs(the_repository, istate, tree_oid, path, check);
 
 	for (i = 0; i < check->nr; i++) {
 		unsigned int n = check->items[i].attr->attr_nr;
@@ -1321,14 +1313,14 @@ void git_check_attr(struct index_state *istate,
 	}
 }
 
-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)
 {
 	int i;
 	const struct object_id *tree_oid = default_attr_source();
 
 	attr_check_reset(check);
-	collect_some_attrs(istate, tree_oid, path, check);
+	collect_some_attrs(repo, istate, tree_oid, path, check);
 
 	for (i = 0; i < check->all_attrs_nr; i++) {
 		const char *name = check->all_attrs[i].attr->name;
diff --git a/attr.h b/attr.h
index a04a521092..1ff058bef7 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 {
@@ -232,9 +234,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/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..8fbf5430a4 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -71,7 +71,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;
-- 
2.48.GIT

Re: [GSOC PATCH v2 2/2] attr: use `repo_settings_get_attributesfile_path()` and update callers

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 6969 bytes --]

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

> Update attribute-related functions to retrieve the "core.attributesfile"
> configuration via the new repository-scoped accessor
> `repo_settings_get_attributesfile_path()`. This improves behaviour in
> multi-repository contexts and aligns with the goal of minimizing
> reliance on global state.
>

We should also talk about the modifications made to pass around the
repository struct.

> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
>  attr.c               | 28 ++++++++++------------------
>  attr.h               |  7 +++----
>  builtin/check-attr.c |  2 +-
>  builtin/var.c        |  2 +-
>  4 files changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index 0bd2750528..8f28463e8c 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);
> @@ -906,7 +898,7 @@ static void push_stack(struct attr_stack **attr_stack_p,
>  	}
>  }
>
> -static void bootstrap_attr_stack(struct index_state *istate,
> +static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate,

Nit: here and other places, can this be a 'const'?

>  				 const struct object_id *tree_oid,
>  				 struct attr_stack **stack)
>  {
> @@ -927,8 +919,8 @@ 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 (repo_settings_get_attributesfile_path(repo)) {
> +		e = read_attr_from_file(repo_settings_get_attributesfile_path(repo), flags);
>  		push_stack(stack, e, NULL, 0);
>  	}
>
> @@ -946,7 +938,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
>  	push_stack(stack, e, NULL, 0);
>  }
>
> -static void prepare_attr_stack(struct index_state *istate,
> +static void prepare_attr_stack(struct repository *repo, struct index_state *istate,
>  			       const struct object_id *tree_oid,
>  			       const char *path, int dirlen,
>  			       struct attr_stack **stack)
> @@ -969,7 +961,7 @@ static void prepare_attr_stack(struct index_state *istate,
>  	 * .gitattributes in deeper directories to shallower ones,
>  	 * and finally use the built-in set as the default.
>  	 */
> -	bootstrap_attr_stack(istate, tree_oid, stack);
> +	bootstrap_attr_stack(repo, istate, tree_oid, stack);
>
>  	/*
>  	 * Pop the "info" one that is always at the top of the stack.
> @@ -1143,7 +1135,7 @@ static void determine_macros(struct all_attrs_item *all_attrs,
>   * If check->check_nr is non-zero, only attributes in check[] are collected.
>   * Otherwise all attributes are collected.
>   */
> -static void collect_some_attrs(struct index_state *istate,
> +static void collect_some_attrs(struct repository *repo, struct index_state *istate,
>  			       const struct object_id *tree_oid,
>  			       const char *path, struct attr_check *check)
>  {
> @@ -1164,7 +1156,7 @@ static void collect_some_attrs(struct index_state *istate,
>  		dirlen = 0;
>  	}
>
> -	prepare_attr_stack(istate, tree_oid, path, dirlen, &check->stack);
> +	prepare_attr_stack(repo, istate, tree_oid, path, dirlen, &check->stack);
>  	all_attrs_init(&g_attr_hashmap, check);
>  	determine_macros(check->all_attrs, check->stack);
>
> @@ -1310,7 +1302,7 @@ void git_check_attr(struct index_state *istate,
>  	int i;
>  	const struct object_id *tree_oid = default_attr_source();
>
> -	collect_some_attrs(istate, tree_oid, path, check);
> +	collect_some_attrs(the_repository, istate, tree_oid, path, check);
>

The other places in the same file we pass around the 'repository'
struct, but here we use 'the_repository'. Even below, we modify an
external function's signature to avail the 'repository' struct.

Can't we modify 'git_check_attr()' to also receive a 'repository'? If
not, perhaps it would be much simpler to simply pass 'the_repository'
everywhere and cleanup this file in another follow up series?

>  	for (i = 0; i < check->nr; i++) {
>  		unsigned int n = check->items[i].attr->attr_nr;
> @@ -1321,14 +1313,14 @@ void git_check_attr(struct index_state *istate,
>  	}
>  }
>
> -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)
>  {
>  	int i;
>  	const struct object_id *tree_oid = default_attr_source();
>
>  	attr_check_reset(check);
> -	collect_some_attrs(istate, tree_oid, path, check);
> +	collect_some_attrs(repo, istate, tree_oid, path, check);
>
>  	for (i = 0; i < check->all_attrs_nr; i++) {
>  		const char *name = check->all_attrs[i].attr->name;
> diff --git a/attr.h b/attr.h
> index a04a521092..1ff058bef7 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 {
> @@ -232,9 +234,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/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..8fbf5430a4 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -71,7 +71,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;
> --
> 2.48.GIT

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [GSOC PATCH v2 2/2] attr: use `repo_settings_get_attributesfile_path()` and update callers

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>
>> Update attribute-related functions to retrieve the "core.attributesfile"
>> configuration via the new repository-scoped accessor
>> `repo_settings_get_attributesfile_path()`. This improves behaviour in
>> multi-repository contexts and aligns with the goal of minimizing
>> reliance on global state.
>>
>
> We should also talk about the modifications made to pass around the
> repository struct.

Yes.  We first should justify if it makes sense to cram attribute
set into the repository object and pass it around in the first
place.  Many index-state related functions do not pass repository
around because they work on index-state, so index-state is passed
around instead.  Perhaps the attribute subsystem should be the same
way, in that their globals should belong to its own abstraction that
is smaller scale than a repository object (it is permissible to have
such an attribute-set object know about which repository instance it
is related to, though).

Re: [GSOC PATCH v2 2/2] attr: use `repo_settings_get_attributesfile_path()` and update callers

[Ayush Chandekar]

> Can't we modify 'git_check_attr()' to also receive a 'repository'? If
> not, perhaps it would be much simpler to simply pass 'the_repository'
> everywhere and cleanup this file in another follow up series?
>

Right, that was one of the things I considered too. But since `git_check_attr()`
is used in a lot of widely used code paths that don't currently pass a
struct repository, it felt like threading repo through all of them would
create a much larger change that I intended for this patch series.

That's why I decided to stick with `the_repository` for now, and perhaps revisit
the cleanup in a follow-up series once the proposed changes are
accepted by the community.

Re: [GSOC PATCH v2 2/2] attr: use `repo_settings_get_attributesfile_path()` and update callers

[shejialuo]

On Mon, Mar 10, 2025 at 08:40:48PM +0530, Ayush Chandekar wrote:
> Update attribute-related functions to retrieve the "core.attributesfile"
> configuration via the new repository-scoped accessor
> `repo_settings_get_attributesfile_path()`. This improves behaviour in
> multi-repository contexts and aligns with the goal of minimizing
> reliance on global state.
> 
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
>  attr.c               | 28 ++++++++++------------------
>  attr.h               |  7 +++----
>  builtin/check-attr.c |  2 +-
>  builtin/var.c        |  2 +-
>  4 files changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/attr.c b/attr.c
> index 0bd2750528..8f28463e8c 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);
> @@ -906,7 +898,7 @@ static void push_stack(struct attr_stack **attr_stack_p,
>  	}
>  }
>  
> -static void bootstrap_attr_stack(struct index_state *istate,
> +static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate,

I have scanned the definition of the "struct index_state", there is a
"struct repository *repo" member in this data structure. This makes me
think why do we need to pass the "struct repository *repo" in the first
place. A design question, should we just use `istate->repo` directly?

>  				 const struct object_id *tree_oid,
>  				 struct attr_stack **stack)
>  {
> @@ -927,8 +919,8 @@ 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 (repo_settings_get_attributesfile_path(repo)) {
> +		e = read_attr_from_file(repo_settings_get_attributesfile_path(repo), flags);
>  		push_stack(stack, e, NULL, 0);
>  	}
>  
> @@ -946,7 +938,7 @@ static void bootstrap_attr_stack(struct index_state *istate,
>  	push_stack(stack, e, NULL, 0);
>  }
>  
> -static void prepare_attr_stack(struct index_state *istate,
> +static void prepare_attr_stack(struct repository *repo, struct index_state *istate,
>  			       const struct object_id *tree_oid,
>  			       const char *path, int dirlen,
>  			       struct attr_stack **stack)

If we use "istate->repo", we don't even need to change this function.

> @@ -969,7 +961,7 @@ static void prepare_attr_stack(struct index_state *istate,
>  	 * .gitattributes in deeper directories to shallower ones,
>  	 * and finally use the built-in set as the default.
>  	 */
> -	bootstrap_attr_stack(istate, tree_oid, stack);
> +	bootstrap_attr_stack(repo, istate, tree_oid, stack);
>  
>  	/*
>  	 * Pop the "info" one that is always at the top of the stack.

[snip]

Thanks,
Jialuo

Re: [GSOC PATCH v2 2/2] attr: use `repo_settings_get_attributesfile_path()` and update callers

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

>> -static void bootstrap_attr_stack(struct index_state *istate,
>> +static void bootstrap_attr_stack(struct repository *repo, struct index_state *istate,
>
> I have scanned the definition of the "struct index_state", there is a
> "struct repository *repo" member in this data structure. This makes me
> think why do we need to pass the "struct repository *repo" in the first
> place. A design question, should we just use `istate->repo` directly?

Good thing to notice.

As the attribute system is all about giving extra information on the
paths that appear in the index and in the working tree, it may make
sense for the API to go from the index state which is about the
index and the working tree to access the attributes, rather than
from the repository structure, which controls a lot wider concept
and moving anything and everything there will easily and quickly
make it a messy kitchen sink.

Re: [GSOC PATCH v2 2/2] attr: use `repo_settings_get_attributesfile_path()` and update callers

[Ayush Chandekar]

> If we use "istate->repo", we don't even need to change this function.

Oh you're absolutely right about that. I actually just got to learn more
about `index_state` from another thread where Junio brought it up.

But now with his suggestion that attributes may not belong in the
repository struct at all, I'm a bit unsure how to move the patch forward.

One thing I did take away from this is that we shouldn't be cramming
environment variables into the repository struct. This discussion has
definitely helped me think more clearly about the design, and I think
it'll guide me take better decisions going forward.

I’d really appreciate your thoughts on how you think we should
approach this from here.

Thanks,
Ayush:)