store git_default_config() parsed values in new config struct

The config value parsed in git_default_core_config() is loaded eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

Move this value into a struct config_values which holds all the values parsed by git_default_config() and can be accessed per repository via git_default_config(). This will prevent us from moving any code from git_default_core_config(), ensuring the current behaviour remains the same while also enabling the libification of Git.

It is important to note that git_default_config() is a wrapper to other git_default_*_config() such as git_default_core_config(). Therefore to access and modify this global variable,
the change has to be made in the function which parses and stores the value in the global variable.

Suggested-by Phillip Wood <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Usman Akinyemi <[email protected]>

Signed-off-by Olamide Caleb Bello <[email protected]>

---
  attr.c        | 8 +++++---
  config.c      | 5 +++++
  config.h      | 8 ++++++++
  environment.c | 7 ++++---
  environment.h | 1 -
  repository.c  | 6 ++++++
  repository.h  | 4 ++++
  7 files changed, 32 insertions(+), 7 deletions(-)

attr.c +5 -3

index 4999b7e09d..eb7b82707d 100644
@@ -881,10 +881,12 @@ const char *git_attr_system_file(void)
 
  const char *git_attr_global_file(void)
  {
-	if (!git_attributes_file)
-		git_attributes_file = xdg_config_home("attributes");
+	struct config_values *cfg = the_repository->cfg_values;
 
-	return git_attributes_file;
+	if (!cfg->attributes_file_path)
+		cfg->attributes_file_path = xdg_config_home("attributes");
+
+	return cfg->attributes_file_path;
  }
 
  int git_attr_system_is_enabled(void)

config.c +5 -0

index 7f6d53b473..8b882f64ae 100644
@@ -1761,6 +1761,11 @@ static int config_set_element_cmp(const void *cmp_data UNUSED,
  	return strcmp(e1->key, e2->key);
  }
 
+void config_values_clear(struct config_values *cfg)
+{
+	free(cfg->attributes_file_path);
+}
+
  void git_configset_init(struct config_set *set)
  {
  	hashmap_init(&set->config_hash, config_set_element_cmp, NULL, 0);

config.h +8 -0

index ba426a960a..1652d315e2 100644
@@ -135,6 +135,13 @@ struct config_context {
  	/* Config source metadata for key and value. */
  	const struct key_value_info *kvi;
  };
+
+/* Holds values parsed by git_default_config() */
+struct config_values {
+	/* core config values */
+	char *attributes_file_path;
+
+};
  #define CONFIG_CONTEXT_INIT { 0 }
 
  /**

@@ -187,6 +194,7 @@ int git_config_from_blob_oid(config_fn_t fn, const char *name,
  void git_config_push_parameter(const char *text);
  void git_config_push_env(const char *spec);
  int git_config_from_parameters(config_fn_t fn, void *data);
+void config_values_clear(struct config_values *cfg);
 
  /*
   * Read config when the Git directory has not yet been set up. In case

environment.c +4 -3

index a770b5921d..d633b0405b 100644
@@ -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;

@@ -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 config_values *cfg = the_repository->cfg_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_path);
+		return git_config_pathname(&cfg->attributes_file_path, var, value);
  	}
 
  	if (!strcmp(var, "core.bare")) {

environment.h +0 -1

index 51898c99cd..3512a7072e 100644
@@ -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;

repository.c +6 -0

index c7e75215ac..3ad944e71c 100644
@@ -55,6 +55,7 @@ void initialize_repository(struct repository *repo)
  	repo->remote_state = remote_state_new();
  	repo->parsed_objects = parsed_object_pool_new(repo);
  	ALLOC_ARRAY(repo->index, 1);
+	CALLOC_ARRAY(repo->cfg_values, 1);
  	index_state_init(repo->index, repo);
  	repo->check_deprecated_config = true;
 

@@ -403,6 +404,11 @@ void repo_clear(struct repository *repo)
  		FREE_AND_NULL(repo->remote_state);
  	}
 
+	if (repo->cfg_values) {
+		config_values_clear(repo->cfg_values);
+		FREE_AND_NULL(repo->cfg_values);
+	}
+
  	strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e)
  		ref_store_release(e->value);
  	strmap_clear(&repo->submodule_ref_stores, 1);

repository.h +4 -0

index 6063c4b846..5fb825f799 100644
@@ -13,6 +13,7 @@ struct object_database;
  struct submodule_cache;
  struct promisor_remote_config;
  struct remote_state;
+struct config_values;
 
  enum ref_storage_format {
  	REF_STORAGE_FORMAT_UNKNOWN,

@@ -171,6 +172,9 @@ struct repository {
 
  	/* Should repo_config() check for deprecated settings */
  	bool check_deprecated_config;
+
+	/* Repository's config values parsed by git_default_config() */
+	struct config_values *cfg_values;
  };
 
  #ifdef USE_THE_REPOSITORY_VARIABLE

Hi Olamide

Quoted 5 lines

The config value parsed in git_default_core_config() is loaded eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

Quoted 5 lines

Move this value into a struct config_values which holds all the values parsed by git_default_config() and can be accessed per repository via git_default_config(). This will prevent us from moving any code from git_default_core_config(), ensuring the current behaviour remains the same while also enabling the libification of Git.

The important thing is that we're not changing when the config is parsed, not that we're not removing code from git_default_core_config().

Looking at the changes below, I think it would be simpler to embed struct config_values in struct repository as we do for struct repo_settings. That would simplify things as we wouldn't have to mess about allocating an instance on the heap and freeing it in repo_clear().

I'd be tempted to call the new struct repo_config rather than config_values which is rather non-specific.

I'm also not sure
config.[ch] is the best home for it, maybe it should live in environment.[ch] for now - we might want to move it to it's own file at some point.

The config value parsed in git_default_core_config() is loaded eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

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;

Quoted 4 lines · Olamide Caleb Bello

The config value parsed in git_default_core_config() is loaded eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

There are quite a many global singleton variables that are accessed by git_default_core_config(), and this patch addresses only one of them. Are most of these variables per repository, or are there some variables that are inherently just one for a user or for a system?

builtin/backfill.c +1 -1

 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "builtin.h"

@@ -139,7 +138,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
 	repo_config(repo, git_default_config, NULL);
 
 	if (ctx.sparse < 0)
-		ctx.sparse = core_apply_sparse_checkout;
+		ctx.sparse = repo->config_values.sparse_checkout;
 
 	result = do_backfill(&ctx);
 	backfill_context_clear(&ctx);

builtin/clone.c +0 -0

index b19b302b06..b6b19e83d1 100644

builtin/backfill.c line 1

Quoted 6 lines · Olamide Caleb Bello

builtin/backfill.c +0 -1

index e80fc1b694..90d5312240 100644
@@ -1,4 +1,3 @@
-/* We need this macro to access core_apply_sparse_checkout */

Why this removal? You'll need to be able to access the_repository because ...

Quoted 10 lines

builtin/backfill.c +1 -1

 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "builtin.h"

@@ -139,7 +138,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
      repo_config(repo, git_default_config, NULL);
 
      if (ctx.sparse < 0)
-             ctx.sparse = core_apply_sparse_checkout;
+             ctx.sparse = repo->config_values.sparse_checkout;
 

builtin/clone.c static int git_sparse_checkout_init(const char *repo) · line 626

Quoted 12 lines

builtin/backfill.c +0 -0

      result = do_backfill(&ctx);
      backfill_context_clear(&ctx);

builtin/clone.c +1 -1

index b19b302b06..b6b19e83d1 100644
@@ -623,7 +623,7 @@ static int git_sparse_checkout_init(const char *repo)
       * We must apply the setting in the current process
       * for the later checkout to use the sparse-checkout file.
       */
-     core_apply_sparse_checkout = 1;
+     the_repository->config_values.sparse_checkout = 1;

... you'd need to access this, even if it is now called slightly differently, no?

The config value brach.autoSetupMerge is parsed in git_default_branch_config() and stored in the global variable git_branch_track. This global variable can cause unexpected behaviours when multiple Git repos run in the the same process.

Move this value into struct repo_config_values which holds all values parsed by git_default_config() and can be accessed per repo via git_default_config(). This would retain the same behaviours while achieving repository scoped access.

Suggested-by Phillip Wood <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Usman Akinyemi <[email protected]>

Signed-off-by Olamide Caleb Bello <[email protected]>

---

Quoted 10 lines · Olamide Caleb Bello

The config value brach.autoSetupMerge is parsed in git_default_branch_config() and stored in the global variable git_branch_track. This global variable can cause unexpected behaviours when multiple Git repos run in the the same process.

Move this value into struct repo_config_values which holds all values parsed by git_default_config() and can be accessed per repo via git_default_config(). This would retain the same behaviours while achieving repository scoped access.

Quoted 5 lines

Suggested-by Phillip Wood <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Usman Akinyemi <[email protected]>

Signed-off-by Olamide Caleb Bello <[email protected]>

---

Currently the code flow is for git_config(git_default_config) to be called fairly early in the program, updating the singleton globals that are independent from individual repository. This moves these global variables to be stored in the config_values structure that is tied to the_repository.

The claim in the cover letter was that this will make it possible for us to later work on more than one repositories at once and each repository can keep its own independent value. While the updated data structure may make it _possible_, I am not sure if this is a safe approach to get to the final state, without seeing how the config_values structure in the second "repo" structure is populated.

And before moving all these globals into config_values, it is not possible to safely populate the config_values structure in the second "repo" structure, if git_config(git_default_config) is what we plan to use. The settings that are still stored in globals will then get overwritten.

That is why my first question for this round of patches was "are all these settings inherently per repository?", because the scheme would not work if there are globals that cannot be moved to config_value structure to be per-repo.

repository.c void initialize_repository(struct repository *repo) · line 60

Quoted 11 lines

repository.c +1 -0

index c7e75215ac..d308cd78bf 100644
@@ -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);

Having a call to repo_config_values_init() when initializing an in-core repository instance is a reasonable design, and I see this step has an initialization of git_branch_track in that function. Shouldn't we be doing similar initialization in the same config_values_init() function for other members of the structure, namely, attributes_file_path and sparse_checkout?

The function also may be a good place to do an equivalent of calling git_config(git_default_config) there to grab values that are suitable for the given repository, but I didn't think things through.

Hi Git Community,
Over the course of my ongoing internship, which focused on moving global variables in environment.h into local scope, I have attempted to move some variables into the struct repo-settings.
However there have been some design concerns as regards the use of prepare_repo_settings() with respect to when and where to call the function, and also the change in behaviours when the variable is lazily loaded as discussed in [1] and [2].

After different deliberations, Phillip Wood proposed creating a new config struct [3], adding it to the repository struct and passing the repo struct to git_default_config() to store the parsed config values per repo. This ensures the current behaviours will be retained.

I have experimented with this approach for some values and I would appreciate feedbacks about this approach before we can move forward and use it for more variables related to git_default_config().

For now, the parsed value is stored in the_repository in git_default_*_config() and further efforts to pass the repository parameter to git_default_config() as the callback parameter will be looked into later on.
The link to the CI tests can be see in [4]

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/
3. https://lore.kernel.org/git/[email protected]/
4. https://github.com/git/git/actions/runs/20953059862

Changes in v2:
==============
- Renamed new struct to repo_config_values
- Moved struct and functions declaration and definition to environment.[ch]
- embedded the new struct in the repository struct thereby removing the need to allocate memory on the heap

The config value is parsed in git_default_core_config(), loaded

eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

Create a new struct repo_config_values to hold this value and other repository dependent values parsed by git_default_config() and can be accessed per repository via git_default_config().

This will ensure the current behaviour remains the same while also enabling the libification of Git.

It is important to note that git_default_config() is a wrapper to other git_default_*_config() such as git_default_core_config().

Therefore to access and modify this global variable, the change has to be made in the function which parses and stores the value in the global variable.

Suggested-by Phillip Wood <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Usman Akinyemi <[email protected]>

Signed-off-by Olamide Caleb Bello <[email protected]>

---
 attr.c        |  7 ++++---
 environment.c | 12 +++++++++---
 environment.h |  9 ++++++++-
 repository.c  |  1 +
 repository.h  |  4 ++++
 5 files changed, 26 insertions(+), 7 deletions(-)

attr.c +0 -0

index 4999b7e09d..fbb9eaffaf 100644

attr.c +3 -1

+	if (!cfg->attributes_file_path)
+		cfg->attributes_file_path = xdg_config_home("attributes");
 
-	return git_attributes_file;
+	return cfg->attributes_file_path;
 }
 
 int git_attr_system_is_enabled(void)

environment.c +4 -3

index a770b5921d..283db0a1a0 100644
@@ -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;

@@ -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_path);
+		return git_config_pathname(&cfg->attributes_file_path, var, value);
 	}
 
 	if (!strcmp(var, "core.bare")) {

environment.h +4 -1

+};
+
 /*
  * Wrapper of getenv() that returns a strdup value. This value is kept
  * in argv to be freed later.

@@ -107,6 +113,8 @@ const char *strip_namespace(const char *namespaced_ref);
 int git_default_config(const char *, const char *,
 		       const struct config_context *, void *);
 
+void repo_config_values_init(struct repo_config_values *cfg);
+
 /*
  * TODO: All the below state either explicitly or implicitly relies on
  * `the_repository`. We should eventually get rid of these and make the

@@ -152,7 +160,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;

repository.c +1 -0

index c7e75215ac..d308cd78bf 100644
@@ -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

repository.h +4 -0

index 6063c4b846..638a142577 100644
@@ -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;
 

Signature · 1 line

2.34.1

Quoted 1 line · Olamide Caleb Bello

The config value is parsed in git_default_core_config(), loaded

I assume you mean 'core.attributesFile' because it's in the title. But personnally I don't mind seeing the name repeated in the body to make it more clear.

Quoted 8 lines

eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

Create a new struct repo_config_values to hold this value and other repository dependent values parsed by git_default_config() and can be accessed per repository via git_default_config().

I'd suggest to split off the part after the second 'and' into a new sentence.

Quoted 3 lines

This will ensure the current behaviour remains the same while also enabling the libification of Git.

How is this true? Was that value already accessible through git_default_config()?

Quoted 2 lines

It is important to note that git_default_config() is a wrapper to other git_default_*_config() such as git_default_core_config().

I'd suggest to insert 'functions' before 'such'.

Quoted 4 lines

Therefore to access and modify this global variable, the change has to be made in the function which parses and stores the value in the global variable.

This doesn't clarify much for me. Do you mean 'git_attr_global_file()' and 'git_default_core_config()'?

Quoted 19 lines

Suggested-by Phillip Wood <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Usman Akinyemi <[email protected]>

Signed-off-by Olamide Caleb Bello <[email protected]>

---
 attr.c        |  7 ++++---
 environment.c | 12 +++++++++---
 environment.h |  9 ++++++++-
 repository.c  |  1 +
 repository.h  |  4 ++++
 5 files changed, 26 insertions(+), 7 deletions(-)

attr.c +0 -0

index 4999b7e09d..fbb9eaffaf 100644

attr.c const char *git_attr_system_file(void) · line 884

Quoted 6 lines

attr.c +1 -2

@@ -881,10 +881,11 @@ const char *git_attr_system_file(void)
 
 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;

Is 'cfg' guaranteed to be != NULL?

Quoted 39 lines

attr.c +3 -1

+     if (!cfg->attributes_file_path)
+             cfg->attributes_file_path = xdg_config_home("attributes");
 
-     return git_attributes_file;
+     return cfg->attributes_file_path;
 }
 
 int git_attr_system_is_enabled(void)

environment.c +4 -3

index a770b5921d..283db0a1a0 100644
@@ -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;

@@ -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_path);
+             return git_config_pathname(&cfg->attributes_file_path, var, value);
      }
 

environment.c int git_default_config(const char *var, const char *value, · line 764

Quoted 10 lines

environment.c +5 -0

      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;
 }
+
+void repo_config_values_init(struct repo_config_values *cfg)
+{
+     cfg->attributes_file_path = NULL;
+}

I assume the reason for adding this function becomes clear in a later commit?

Quoted 7 lines

environment.h +0 -0

index 51898c99cd..aea73ff25b 100644
@@ -84,6 +84,12 @@ extern const char * const local_repo_env[];
 

environment.h extern const char * const local_repo_env[]; · line 87

Quoted 3 lines

environment.h +1 -0

 struct strvec;
 
+/* Config values parsed by git_default_config() */

Mentioning here they get filled from git_default_config() doesn't feel really correct? Although I'm sure what comment would fit better, maybe just drop the comment above the struct. I see you have a similar comment in 'struct repository', where it *does* make sense.

environment.h extern const char * const local_repo_env[]; · line 89

Quoted 3 lines

environment.h +2 -0

+struct repo_config_values {
+     /* core config values */

I prefer emphasizing it's the "section 'core'" or something like that.

environment.h extern const char * const local_repo_env[]; · line 90

Quoted 2 lines

environment.h +1 -0

+     char *attributes_file_path;

Would it be overkill to append: /* core.attributesFile */? This can help when grepping through the codebase to find where some settings are being parsed into. What do you think?

Quoted 58 lines

environment.h +4 -1

+};
+
 /*
  * Wrapper of getenv() that returns a strdup value. This value is kept
  * in argv to be freed later.

@@ -107,6 +113,8 @@ const char *strip_namespace(const char *namespaced_ref);
 int git_default_config(const char *, const char *,
                     const struct config_context *, void *);
 
+void repo_config_values_init(struct repo_config_values *cfg);
+
 /*
  * TODO: All the below state either explicitly or implicitly relies on
  * `the_repository`. We should eventually get rid of these and make the

@@ -152,7 +160,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;

repository.c +1 -0

index c7e75215ac..d308cd78bf 100644
@@ -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

repository.h +4 -1

index 6063c4b846..638a142577 100644
@@ -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;
 
--

Quoted 3 lines

2.34.1

--
Cheers,
Toon

The config value is parsed in git_default_core_config(), loaded eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

environment.h +4 -1

+};
+
  /*
   * Wrapper of getenv() that returns a strdup value. This value is kept
   * in argv to be freed later.

@@ -107,6 +113,8 @@ const char *strip_namespace(const char *namespaced_ref);
  int git_default_config(const char *, const char *,
  		       const struct config_context *, void *);
 
+void repo_config_values_init(struct repo_config_values *cfg);
+
  /*
   * TODO: All the below state either explicitly or implicitly relies on
   * `the_repository`. We should eventually get rid of these and make the

@@ -152,7 +160,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;

repository.c +1 -0

index c7e75215ac..d308cd78bf 100644
@@ -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

repository.h +4 -0

index 6063c4b846..638a142577 100644
@@ -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;
 

Hi Olamide

Quoted 4 lines

The config value is parsed in git_default_core_config(), loaded eagerly and stored in the global variable git_attributes_file. Storing this value in a global variable can lead to unexpected behaviours when more than one Git repository run in the same Git process.

It would maybe be helpful to explain what the unexpected behavior is and how it is caused.

Quoted 9 lines

environment.h +0 -0

index 51898c99cd..aea73ff25b 100644
@@ -84,6 +84,12 @@ extern const char * const local_repo_env[];
 
  struct strvec;
 

environment.h extern const char * const local_repo_env[]; · line 90

Quoted 4 lines

environment.h +4 -0

+/* Config values parsed by git_default_config() */
+struct repo_config_values {
+     /* core config values */
+     char *attributes_file_path;

The variable we're converting is called "attributes_file", do we really need to add a "_path" suffix?

Apart from that everything here looks good

Thanks

Phillip

The config value core.sparseCheckout is parsed in git_default_core_config() and stored globally in core_appy_sparse_checkout. This could cause unintended behaviours

when different Git repositories running in the same process access this variable.

Move the parsed value into struct repo_config_values to retains current behaviours while achieving the repository scoped access.

Suggested-by Phillip Wood <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Usman Akinyemi <[email protected]>

Signed-off-by Olamide Caleb Bello <[email protected]>

---
 builtin/backfill.c        |  2 +-
 builtin/clone.c           |  2 +-
 builtin/grep.c            |  2 +-
 builtin/mv.c              |  2 +-
 builtin/sparse-checkout.c | 22 +++++++++++-----------
 builtin/worktree.c        |  2 +-
 dir.c                     |  2 +-
 environment.c             |  4 ++--
 environment.h             |  2 +-
 sparse-index.c            |  6 ++++--
 unpack-trees.c            |  2 +-
 wt-status.c               |  2 +-
 12 files changed, 26 insertions(+), 24 deletions(-)

builtin/backfill.c +1 -1

index e80fc1b694..5fc8c51ed1 100644
@@ -139,7 +139,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
 	repo_config(repo, git_default_config, NULL);
 
 	if (ctx.sparse < 0)
-		ctx.sparse = core_apply_sparse_checkout;
+		ctx.sparse = repo->config_values.sparse_checkout;
 
 	result = do_backfill(&ctx);
 	backfill_context_clear(&ctx);

builtin/clone.c +1 -1

index b19b302b06..b6b19e83d1 100644
@@ -623,7 +623,7 @@ static int git_sparse_checkout_init(const char *repo)
 	 * We must apply the setting in the current process
 	 * for the later checkout to use the sparse-checkout file.
 	 */
-	core_apply_sparse_checkout = 1;
+	the_repository->config_values.sparse_checkout = 1;
 
 	cmd.git_cmd = 1;
 	if (run_command(&cmd)) {

builtin/grep.c +1 -1

index 53cccf2d25..525edb5e9c 100644
@@ -482,7 +482,7 @@ static int grep_submodule(struct grep_opt *opt,
 	 *	"forget" the sparse-index feature switch. As a result, the index
 	 *	of these submodules are expanded unexpectedly.
 	 *
-	 * 2. "core_apply_sparse_checkout"
+	 * 2. "sparse_checkout"
 	 *	When running `grep` in the superproject, this setting is
 	 *	populated using the superproject's configs. However, once
 	 *	initialized, this config is globally accessible and is read by

builtin/mv.c +1 -1

index d43925097b..511620747b 100644
@@ -572,7 +572,7 @@ int cmd_mv(int argc,
 		rename_index_entry_at(the_repository->index, pos, dst);
 
 		if (ignore_sparse &&
-		    core_apply_sparse_checkout &&
+		    the_repository->config_values.sparse_checkout &&
 		    core_sparse_checkout_cone) {
 			/*
 			 * NEEDSWORK: we are *not* paying attention to

builtin/sparse-checkout.c +11 -11

index 15d51e60a8..1c2c39b968 100644
@@ -63,7 +63,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
 	int res;
 
 	setup_work_tree();
-	if (!core_apply_sparse_checkout)
+	if (!the_repository->config_values.sparse_checkout)
 		die(_("this worktree is not sparse"));
 
 	argc = parse_options(argc, argv, prefix,

@@ -400,11 +400,11 @@ static int set_config(struct repository *repo,
 
 static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
 	/* If not specified, use previous definition of cone mode */
-	if (*cone_mode == -1 && core_apply_sparse_checkout)
+	if (*cone_mode == -1 && the_repository->config_values.sparse_checkout)
 		*cone_mode = core_sparse_checkout_cone;
 
 	/* Set cone/non-cone mode appropriately */
-	core_apply_sparse_checkout = 1;
+	the_repository->config_values.sparse_checkout = 1;
 	if (*cone_mode == 1 || *cone_mode == -1) {
 		core_sparse_checkout_cone = 1;
 		return MODE_CONE_PATTERNS;

@@ -418,7 +418,7 @@ static int update_modes(struct repository *repo, int *cone_mode, int *sparse_ind
 	int mode, record_mode;
 
 	/* Determine if we need to record the mode; ensure sparse checkout on */
-	record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
+	record_mode = (*cone_mode != -1) || !repo->config_values.sparse_checkout;
 
 	mode = update_cone_mode(cone_mode);
 	if (record_mode && set_config(repo, mode))

@@ -699,9 +699,9 @@ static int modify_pattern_list(struct repository *repo,
 		break;
 	}
 
-	if (!core_apply_sparse_checkout) {
+	if (!repo->config_values.sparse_checkout) {
 		set_config(repo, MODE_ALL_PATTERNS);
-		core_apply_sparse_checkout = 1;
+		repo->config_values.sparse_checkout = 1;
 		changed_config = 1;
 	}
 

@@ -798,7 +798,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix,
 	int ret;
 
 	setup_work_tree();
-	if (!core_apply_sparse_checkout)
+	if (!repo->config_values.sparse_checkout)
 		die(_("no sparse-checkout to add to"));
 
 	repo_read_index(repo);

@@ -907,7 +907,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
 	};
 
 	setup_work_tree();
-	if (!core_apply_sparse_checkout)
+	if (!repo->config_values.sparse_checkout)
 		die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
 	reapply_opts.cone_mode = -1;

@@ -969,7 +969,7 @@ static int sparse_checkout_clean(int argc, const char **argv,
 	};
 
 	setup_work_tree();
-	if (!core_apply_sparse_checkout)
+	if (!repo->config_values.sparse_checkout)
 		die(_("must be in a sparse-checkout to clean directories"));
 	if (!core_sparse_checkout_cone)
 		die(_("must be in a cone-mode sparse-checkout to clean directories"));

@@ -1035,7 +1035,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
 	struct pattern_list pl;
 
 	/*
-	 * We do not exit early if !core_apply_sparse_checkout; due to the
+	 * We do not exit early if !repo->config_values.sparse_checkout; due to the
 	 * ability for users to manually muck things up between
 	 *   direct editing of .git/info/sparse-checkout
 	 *   running read-tree -m u HEAD or update-index --skip-worktree

@@ -1061,7 +1061,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
 	hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
 	hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
 	pl.use_cone_patterns = 0;
-	core_apply_sparse_checkout = 1;
+	repo->config_values.sparse_checkout = 1;
 
 	add_pattern("/*", empty_base, 0, &pl, 0);
 

builtin/worktree.c +1 -1

index fbdaf2eb2e..e401b8253e 100644
@@ -536,7 +536,7 @@ static int add_worktree(const char *path, const char *refname,
 	 * If the current worktree has sparse-checkout enabled, then copy
 	 * the sparse-checkout patterns from the current worktree.
 	 */
-	if (core_apply_sparse_checkout)
+	if (wt->repo->config_values.sparse_checkout)
 		copy_sparse_checkout(sb_repo.buf);
 
 	/*

dir.c +1 -1

index b00821f294..56b412a6d2 100644
@@ -1551,7 +1551,7 @@ enum pattern_match_result path_matches_pattern_list(
 
 int init_sparse_checkout_patterns(struct index_state *istate)
 {
-	if (!core_apply_sparse_checkout)
+	if (!istate->repo->config_values.sparse_checkout)
 		return 1;
 	if (istate->sparse_checkout_patterns)
 		return 0;

environment.c +2 -2

index 283db0a1a0..6633542750 100644
@@ -74,7 +74,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #endif
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 int grafts_keep_true_parents;
-int core_apply_sparse_checkout;
 int core_sparse_checkout_cone;
 int sparse_expect_files_outside_of_patterns;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */

@@ -546,7 +545,7 @@ static int git_default_core_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "core.sparsecheckout")) {
-		core_apply_sparse_checkout = git_config_bool(var, value);
+		cfg->sparse_checkout = git_config_bool(var, value);
 		return 0;
 	}
 

@@ -761,4 +760,5 @@ int git_default_config(const char *var, const char *value,
 void repo_config_values_init(struct repo_config_values *cfg)
 {
 	cfg->attributes_file_path = NULL;
+	cfg->sparse_checkout = 0;
 }

environment.h +1 -0

index aea73ff25b..3b5ff7094a 100644
@@ -88,6 +88,7 @@ struct strvec;
 struct repo_config_values {
 	/* core config values */
 	char *attributes_file_path;
+	int sparse_checkout;
 };
 
 /*

environment.h +0 -0

 extern int core_sparse_checkout_cone;
 extern int sparse_expect_files_outside_of_patterns;
 

sparse-index.c +4 -2

index 76f90da5f5..6dd8dd679d 100644
@@ -152,7 +152,8 @@ static int index_has_unmerged_entries(struct index_state *istate)
 
 int is_sparse_index_allowed(struct index_state *istate, int flags)
 {
-	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+	struct repo_config_values *cfg = &istate->repo->config_values;
+	if (!cfg->sparse_checkout || !core_sparse_checkout_cone)
 		return 0;
 
 	if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {

@@ -670,7 +671,8 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
 
 void clear_skip_worktree_from_present_files(struct index_state *istate)
 {
-	if (!core_apply_sparse_checkout ||
+	struct repo_config_values *cfg = &istate->repo->config_values;
+	if (!cfg->sparse_checkout ||
 	    sparse_expect_files_outside_of_patterns)
 		return;
 

unpack-trees.c +1 -1

index f38c761ab9..2bdfa1334c 100644
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	if (o->prefix)
 		update_sparsity_for_prefix(o->prefix, o->src_index);
 
-	if (!core_apply_sparse_checkout || !o->update)
+	if (!repo->config_values.sparse_checkout || !o->update)
 		o->skip_sparse_checkout = 1;
 	if (!o->skip_sparse_checkout) {
 		memset(&pl, 0, sizeof(pl));

wt-status.c +1 -1

index e12adb26b9..a2e388606f 100644
@@ -1764,7 +1764,7 @@ static void wt_status_check_sparse_checkout(struct repository *r,
 	int skip_worktree = 0;
 	int i;
 
-	if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+	if (!r->config_values.sparse_checkout || r->index->cache_nr == 0) {
 		/*
 		 * Don't compute percentage of checked out files if we
 		 * aren't in a sparse checkout or would get division by 0.

Signature · 1 line

2.34.1

Olamide Caleb Bello <[email protected]> writes:

It seems you have 'environment:' twice in the title?

Quoted 4 lines

The config value core.sparseCheckout is parsed in git_default_core_config() and stored globally in core_appy_sparse_checkout. This could cause unintended behaviours

s/core_appy_sparse_checkout/core_apply_sparse_checkout/ ?

Quoted 240 lines

when different Git repositories running in the same process access this variable.

Move the parsed value into struct repo_config_values to retains current behaviours while achieving the repository scoped access.

Suggested-by Phillip Wood <[email protected]>

Mentored-by Christian Couder <[email protected]>

Mentored-by Usman Akinyemi <[email protected]>

Signed-off-by Olamide Caleb Bello <[email protected]>

---
 builtin/backfill.c        |  2 +-
 builtin/clone.c           |  2 +-
 builtin/grep.c            |  2 +-
 builtin/mv.c              |  2 +-
 builtin/sparse-checkout.c | 22 +++++++++++-----------
 builtin/worktree.c        |  2 +-
 dir.c                     |  2 +-
 environment.c             |  4 ++--
 environment.h             |  2 +-
 sparse-index.c            |  6 ++++--
 unpack-trees.c            |  2 +-
 wt-status.c               |  2 +-
 12 files changed, 26 insertions(+), 24 deletions(-)

builtin/backfill.c +1 -1

index e80fc1b694..5fc8c51ed1 100644
@@ -139,7 +139,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
      repo_config(repo, git_default_config, NULL);
 
      if (ctx.sparse < 0)
-             ctx.sparse = core_apply_sparse_checkout;
+             ctx.sparse = repo->config_values.sparse_checkout;
 
      result = do_backfill(&ctx);
      backfill_context_clear(&ctx);

builtin/clone.c +1 -1

index b19b302b06..b6b19e83d1 100644
@@ -623,7 +623,7 @@ static int git_sparse_checkout_init(const char *repo)
       * We must apply the setting in the current process
       * for the later checkout to use the sparse-checkout file.
       */
-     core_apply_sparse_checkout = 1;
+     the_repository->config_values.sparse_checkout = 1;
 
      cmd.git_cmd = 1;
      if (run_command(&cmd)) {

builtin/grep.c +1 -1

index 53cccf2d25..525edb5e9c 100644
@@ -482,7 +482,7 @@ static int grep_submodule(struct grep_opt *opt,
       *      "forget" the sparse-index feature switch. As a result, the index
       *      of these submodules are expanded unexpectedly.
       *
-      * 2. "core_apply_sparse_checkout"
+      * 2. "sparse_checkout"
       *      When running `grep` in the superproject, this setting is
       *      populated using the superproject's configs. However, once
       *      initialized, this config is globally accessible and is read by

builtin/mv.c +1 -1

index d43925097b..511620747b 100644
@@ -572,7 +572,7 @@ int cmd_mv(int argc,
              rename_index_entry_at(the_repository->index, pos, dst);
 
              if (ignore_sparse &&
-                 core_apply_sparse_checkout &&
+                 the_repository->config_values.sparse_checkout &&
                  core_sparse_checkout_cone) {
                      /*
                       * NEEDSWORK: we are *not* paying attention to

builtin/sparse-checkout.c +11 -11

index 15d51e60a8..1c2c39b968 100644
@@ -63,7 +63,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
      int res;
 
      setup_work_tree();
-     if (!core_apply_sparse_checkout)
+     if (!the_repository->config_values.sparse_checkout)
              die(_("this worktree is not sparse"));
 
      argc = parse_options(argc, argv, prefix,

@@ -400,11 +400,11 @@ static int set_config(struct repository *repo,
 
 static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {
      /* If not specified, use previous definition of cone mode */
-     if (*cone_mode == -1 && core_apply_sparse_checkout)
+     if (*cone_mode == -1 && the_repository->config_values.sparse_checkout)
              *cone_mode = core_sparse_checkout_cone;
 
      /* Set cone/non-cone mode appropriately */
-     core_apply_sparse_checkout = 1;
+     the_repository->config_values.sparse_checkout = 1;
      if (*cone_mode == 1 || *cone_mode == -1) {
              core_sparse_checkout_cone = 1;
              return MODE_CONE_PATTERNS;

@@ -418,7 +418,7 @@ static int update_modes(struct repository *repo, int *cone_mode, int *sparse_ind
      int mode, record_mode;
 
      /* Determine if we need to record the mode; ensure sparse checkout on */
-     record_mode = (*cone_mode != -1) || !core_apply_sparse_checkout;
+     record_mode = (*cone_mode != -1) || !repo->config_values.sparse_checkout;
 
      mode = update_cone_mode(cone_mode);
      if (record_mode && set_config(repo, mode))

@@ -699,9 +699,9 @@ static int modify_pattern_list(struct repository *repo,
              break;
      }
 
-     if (!core_apply_sparse_checkout) {
+     if (!repo->config_values.sparse_checkout) {
              set_config(repo, MODE_ALL_PATTERNS);
-             core_apply_sparse_checkout = 1;
+             repo->config_values.sparse_checkout = 1;
              changed_config = 1;
      }
 

@@ -798,7 +798,7 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix,
      int ret;
 
      setup_work_tree();
-     if (!core_apply_sparse_checkout)
+     if (!repo->config_values.sparse_checkout)
              die(_("no sparse-checkout to add to"));
 
      repo_read_index(repo);

@@ -907,7 +907,7 @@ static int sparse_checkout_reapply(int argc, const char **argv,
      };
 
      setup_work_tree();
-     if (!core_apply_sparse_checkout)
+     if (!repo->config_values.sparse_checkout)
              die(_("must be in a sparse-checkout to reapply sparsity patterns"));
 
      reapply_opts.cone_mode = -1;

@@ -969,7 +969,7 @@ static int sparse_checkout_clean(int argc, const char **argv,
      };
 
      setup_work_tree();
-     if (!core_apply_sparse_checkout)
+     if (!repo->config_values.sparse_checkout)
              die(_("must be in a sparse-checkout to clean directories"));
      if (!core_sparse_checkout_cone)
              die(_("must be in a cone-mode sparse-checkout to clean directories"));

@@ -1035,7 +1035,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
      struct pattern_list pl;
 
      /*
-      * We do not exit early if !core_apply_sparse_checkout; due to the
+      * We do not exit early if !repo->config_values.sparse_checkout; due to the
       * ability for users to manually muck things up between
       *   direct editing of .git/info/sparse-checkout
       *   running read-tree -m u HEAD or update-index --skip-worktree

@@ -1061,7 +1061,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
      hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
      hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
      pl.use_cone_patterns = 0;
-     core_apply_sparse_checkout = 1;
+     repo->config_values.sparse_checkout = 1;
 
      add_pattern("/*", empty_base, 0, &pl, 0);
 

builtin/worktree.c +1 -1

index fbdaf2eb2e..e401b8253e 100644
@@ -536,7 +536,7 @@ static int add_worktree(const char *path, const char *refname,
       * If the current worktree has sparse-checkout enabled, then copy
       * the sparse-checkout patterns from the current worktree.
       */
-     if (core_apply_sparse_checkout)
+     if (wt->repo->config_values.sparse_checkout)
              copy_sparse_checkout(sb_repo.buf);
 
      /*

dir.c +1 -1

index b00821f294..56b412a6d2 100644
@@ -1551,7 +1551,7 @@ enum pattern_match_result path_matches_pattern_list(
 
 int init_sparse_checkout_patterns(struct index_state *istate)
 {
-     if (!core_apply_sparse_checkout)
+     if (!istate->repo->config_values.sparse_checkout)
              return 1;
      if (istate->sparse_checkout_patterns)
              return 0;

environment.c +2 -2

index 283db0a1a0..6633542750 100644
@@ -74,7 +74,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #endif
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 int grafts_keep_true_parents;
-int core_apply_sparse_checkout;
 int core_sparse_checkout_cone;
 int sparse_expect_files_outside_of_patterns;
 int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */

@@ -546,7 +545,7 @@ static int git_default_core_config(const char *var, const char *value,
      }
 
      if (!strcmp(var, "core.sparsecheckout")) {
-             core_apply_sparse_checkout = git_config_bool(var, value);
+             cfg->sparse_checkout = git_config_bool(var, value);
              return 0;
      }
 

@@ -761,4 +760,5 @@ int git_default_config(const char *var, const char *value,
 void repo_config_values_init(struct repo_config_values *cfg)
 {
      cfg->attributes_file_path = NULL;
+     cfg->sparse_checkout = 0;
 }

environment.h +1 -0

index aea73ff25b..3b5ff7094a 100644
@@ -88,6 +88,7 @@ struct strvec;
 struct repo_config_values {
      /* core config values */
      char *attributes_file_path;
+     int sparse_checkout;
 };
 
 /*

@@ -169,7 +170,6 @@ extern int precomposed_unicode;
 extern int protect_hfs;
 extern int protect_ntfs;
 

environment.h extern int precomposed_unicode; · line 172

Quoted 1 line

environment.h +0 -1

-extern int core_apply_sparse_checkout;

In the field you're adding to 'struct repo_config_values' you have dropped the 'core_' prefix, what the reason for that? If I understand it correctly also settings from other sections might end up in that struct, so wouldn't it be better to keep the prefix?

Quoted 55 lines

environment.h +0 -0

 extern int core_sparse_checkout_cone;
 extern int sparse_expect_files_outside_of_patterns;
 

sparse-index.c +4 -2

index 76f90da5f5..6dd8dd679d 100644
@@ -152,7 +152,8 @@ static int index_has_unmerged_entries(struct index_state *istate)
 
 int is_sparse_index_allowed(struct index_state *istate, int flags)
 {
-     if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+     struct repo_config_values *cfg = &istate->repo->config_values;
+     if (!cfg->sparse_checkout || !core_sparse_checkout_cone)
              return 0;
 
      if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {

@@ -670,7 +671,8 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
 
 void clear_skip_worktree_from_present_files(struct index_state *istate)
 {
-     if (!core_apply_sparse_checkout ||
+     struct repo_config_values *cfg = &istate->repo->config_values;
+     if (!cfg->sparse_checkout ||
          sparse_expect_files_outside_of_patterns)
              return;
 

unpack-trees.c +1 -1

index f38c761ab9..2bdfa1334c 100644
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
      if (o->prefix)
              update_sparsity_for_prefix(o->prefix, o->src_index);
 
-     if (!core_apply_sparse_checkout || !o->update)
+     if (!repo->config_values.sparse_checkout || !o->update)
              o->skip_sparse_checkout = 1;
      if (!o->skip_sparse_checkout) {
              memset(&pl, 0, sizeof(pl));

wt-status.c +1 -2

index e12adb26b9..a2e388606f 100644
@@ -1764,7 +1764,7 @@ static void wt_status_check_sparse_checkout(struct repository *r,
      int skip_worktree = 0;
      int i;
 
-     if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+     if (!r->config_values.sparse_checkout || r->index->cache_nr == 0) {
              /*
               * Don't compute percentage of checked out files if we
               * aren't in a sparse checkout or would get division by 0.
--

Quoted 3 lines

2.34.1

--
Cheers,
Toon

The config value core.sparseCheckout is parsed in git_default_core_config() and stored globally in core_appy_sparse_checkout. This could cause unintended behaviours when different Git repositories running in the same process access this variable.

Move the parsed value into struct repo_config_values to retains current behaviours while achieving the repository scoped access.

builtin/backfill.c +0 -0

  	result = do_backfill(&ctx);
  	backfill_context_clear(&ctx);

builtin/clone.c +1 -1

index b19b302b06..b6b19e83d1 100644
@@ -623,7 +623,7 @@ static int git_sparse_checkout_init(const char *repo)
  	 * We must apply the setting in the current process
  	 * for the later checkout to use the sparse-checkout file.
  	 */
-	core_apply_sparse_checkout = 1;
+	the_repository->config_values.sparse_checkout = 1;
 
  	cmd.git_cmd = 1;
  	if (run_command(&cmd)) {

builtin/grep.c +0 -0

index 53cccf2d25..525edb5e9c 100644

environment.h +0 -1

  };
 
  /*

@@ -169,7 +170,6 @@ extern int precomposed_unicode;
  extern int protect_hfs;
  extern int protect_ntfs;
 
-extern int core_apply_sparse_checkout;
  extern int core_sparse_checkout_cone;
  extern int sparse_expect_files_outside_of_patterns;
 

sparse-index.c +4 -2

index 76f90da5f5..6dd8dd679d 100644
@@ -152,7 +152,8 @@ static int index_has_unmerged_entries(struct index_state *istate)
 
  int is_sparse_index_allowed(struct index_state *istate, int flags)
  {
-	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
+	struct repo_config_values *cfg = &istate->repo->config_values;
+	if (!cfg->sparse_checkout || !core_sparse_checkout_cone)
  		return 0;
 
  	if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {

@@ -670,7 +671,8 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista
 
  void clear_skip_worktree_from_present_files(struct index_state *istate)
  {
-	if (!core_apply_sparse_checkout ||
+	struct repo_config_values *cfg = &istate->repo->config_values;
+	if (!cfg->sparse_checkout ||
  	    sparse_expect_files_outside_of_patterns)
  		return;
 

unpack-trees.c +1 -1

index f38c761ab9..2bdfa1334c 100644
@@ -1924,7 +1924,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
  	if (o->prefix)
  		update_sparsity_for_prefix(o->prefix, o->src_index);
 
-	if (!core_apply_sparse_checkout || !o->update)
+	if (!repo->config_values.sparse_checkout || !o->update)
  		o->skip_sparse_checkout = 1;
  	if (!o->skip_sparse_checkout) {
  		memset(&pl, 0, sizeof(pl));

wt-status.c +1 -1

index e12adb26b9..a2e388606f 100644
@@ -1764,7 +1764,7 @@ static void wt_status_check_sparse_checkout(struct repository *r,
  	int skip_worktree = 0;
  	int i;
 
-	if (!core_apply_sparse_checkout || r->index->cache_nr == 0) {
+	if (!r->config_values.sparse_checkout || r->index->cache_nr == 0) {
  		/*
  		 * Don't compute percentage of checked out files if we
  		 * aren't in a sparse checkout or would get division by 0.

Hi Olamide

Quoted 6 lines · Olamide Caleb Bello

The config value core.sparseCheckout is parsed in git_default_core_config() and stored globally in core_appy_sparse_checkout. This could cause unintended behaviours when different Git repositories running in the same process access this variable.

Quoted 2 lines

Move the parsed value into struct repo_config_values to retains current behaviours while achieving the repository scoped access.

It doesn't achieve repository scoped access though because we only ever populate the values in "the_repository", all other instances of "struct repository" are initialized by config_values_init() but not the config settings.

Quoted 8 lines

builtin/backfill.c +0 -0

index e80fc1b694..5fc8c51ed1 100644
@@ -139,7 +139,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
      repo_config(repo, git_default_config, NULL);
 

builtin/backfill.c int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit · line 142

Quoted 3 lines

builtin/backfill.c +1 -1

      if (ctx.sparse < 0)
-             ctx.sparse = core_apply_sparse_checkout;
+             ctx.sparse = repo->config_values.sparse_checkout;

Using "repo" rather than "the_repository" here is dangerous because only "the_repository" contains the parsed config. This applies throughout this patch.