Re: [Outreachy PATCH v5 1/3] environment: stop storing `core.attributesFile` globally

[Junio C Hamano <gitster@pobox.com>]

Olamide Caleb Bello <belkid98@gmail.com> writes:

>  const char *git_attr_global_file(void)
>  {
> -	if (!git_attributes_file)
> -		git_attributes_file = xdg_config_home("attributes");
> +	struct repo_config_values *cfg = &the_repository->config_values;

Here, the_repository, being defined in repository.c as the address
of a singleton "the_repo" instance, cannot be NULL even outside a
repository, so taking the address of its config_values member is
always safe.  OK.

> +	if (!cfg->attributes_file)
> +		cfg->attributes_file = xdg_config_home("attributes");
>  
> -	return git_attributes_file;
> +	return cfg->attributes_file;
>  }

> diff --git a/repository.h b/repository.h
> index 6063c4b846..638a142577 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -3,6 +3,7 @@
>  
>  #include "strmap.h"
>  #include "repo-settings.h"
> +#include "environment.h"
>  
>  struct config_set;
>  struct git_hash_algo;
> @@ -148,6 +149,9 @@ struct repository {
>  	/* Repository's compatibility hash algorithm. */
>  	const struct git_hash_algo *compat_hash_algo;
>  
> +	/* Repository's config values parsed by git_default_config() */
> +	struct repo_config_values config_values;
> +
>  	/* Repository's reference storage format, as serialized on disk. */
>  	enum ref_storage_format ref_storage_format;

And because this new config_values member is directly embedded in
the repository structure, and "the_repo" instance is a global in
BSS, its members are initialized exactly the same way as the
global variables like git_attributes_file were initialized.  Good.


> diff --git a/environment.c b/environment.c
> index a770b5921d..72735d9e4b 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;

And we lose this global, that used to be zero-initialized for being
in BSS.

> @@ -327,6 +326,8 @@ static enum fsync_component parse_fsync_components(const char *var, const char *
>  static int git_default_core_config(const char *var, const char *value,
>  				   const struct config_context *ctx, void *cb)
>  {
> +	struct repo_config_values *cfg = &the_repository->config_values;
> +
>  	/* This needs a better name */
>  	if (!strcmp(var, "core.filemode")) {
>  		trust_executable_bit = git_config_bool(var, value);
> @@ -364,8 +365,8 @@ static int git_default_core_config(const char *var, const char *value,
>  	}
>  
>  	if (!strcmp(var, "core.attributesfile")) {
> -		FREE_AND_NULL(git_attributes_file);
> -		return git_config_pathname(&git_attributes_file, var, value);
> +		FREE_AND_NULL(cfg->attributes_file);
> +		return git_config_pathname(&cfg->attributes_file, var, value);
>  	}
>  
>  	if (!strcmp(var, "core.bare")) {
> @@ -756,3 +757,8 @@ int git_default_config(const char *var, const char *value,
>  	/* Add other config variables here and to Documentation/config.adoc. */
>  	return 0;
>  }

And instead of assigning to the global git_attributes_file, we
assign to the config_values.attributes_file member via the global
"the_repository".  No functional changes.  OK.

> +void repo_config_values_init(struct repo_config_values *cfg)
> +{
> +	cfg->attributes_file = NULL;
> +}

This is not strictly needed, as git_attributes_file is left to be
zero-initialized for being in BSS; its replacement, i.e.,
the_repo.config_values.attributes_file, will be zero-initialized the
same way.

But other members we may want add later to the struct may need a
place to initialize them.  Or we can do a static initialization for
the_repo in repository.c then we do not have to have this function
and we do not have to call it.  Either would work fine, as long as
everybody calls initialize_repository() function, which is the only
caller of this helper.

> diff --git a/environment.h b/environment.h
> index 51898c99cd..0c0dcc6847 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -84,6 +84,11 @@ extern const char * const local_repo_env[];
>  
>  struct strvec;
>  
> +struct repo_config_values {
> +	/* section "core" config values */
> +	char *attributes_file;
> +};

OK.

> diff --git a/repository.c b/repository.c
> index c7e75215ac..d308cd78bf 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -57,6 +57,7 @@ void initialize_repository(struct repository *repo)
>  	ALLOC_ARRAY(repo->index, 1);
>  	index_state_init(repo->index, repo);
>  	repo->check_deprecated_config = true;
> +	repo_config_values_init(&repo->config_values);
>  
>  	/*
>  	 * When a command runs inside a repository, it learns what

Continuing the discussion on repo_config_values_init(), currently,
initialize_repository() is called by init_git(), which is called
from "main()", so it should be fairly safe to assume that all in Git
codebase will call repo_config_values_init().

But those who replace "main()" for their own libified use of Git
code may not call init_git() hence initialize_repository() hence
your repo_config_values_init().  In that sense, this is less safe
than the other alternative.