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

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 10, 2025, 07:05 UTC
Message-ID: <Z86PUkJ1sbSH2VTU@pks.im>
In-Reply-To: <20250309153321.254844-1-ayu.chandekar@gmail.com>

On Sun, Mar 09, 2025 at 09:03:21PM +0530, Ayush Chandekar wrote:

Show 8 quoted lines

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

Show 19 quoted lines

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

Show 28 quoted lines

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

Show 6 quoted lines

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

Show 41 quoted lines

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

Show 16 quoted lines

> 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