[Outreachy PATCH v4 0/3] store repo specific config values in new `struct repo_config_values`

[Olamide Caleb Bello <belkid98@gmail.com>]

Hi Git Community,
Over the course of my ongoing internship, which focused on moving repo specific
global variables in environment.h into local scope, I have attempted to move some
variables into the struct repo-settings.
However there has 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 parsed repo specific 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/43aaec10-2696-44c9-8728-2045b83dc5d3@gmail.com/
2. https://lore.kernel.org/git/a881499d-e236-4f8e-a217-b6bce69e3e3c@gmail.com/
3. https://lore.kernel.org/git/8899016f-eeef-404b-8da6-ff3a90e81cea@gmail.com/
4. https://gitlab.com/gitlab-community/gitlab-org/git/-/pipelines/2266020513

Changes in v4:
==============
- Changed variable name in the struct to attributes_file
- Modified commit messages in all patches to explain how the variable
  being global can be overwritten when the process has multiple
  repositories
- Changed the variable name in patch 2 to apply_sparse_checkout
- Changes the variable name in patch 3 to branch_track


Olamide Caleb Bello (3):
  environment: stop storing `core.attributesFile` globally
  environment: stop using core.sparseCheckout globally
  environment: move "branch.autoSetupMerge" into `struct
    repo_config_values`

 attr.c                      |  7 ++++---
 branch.h                    |  2 --
 builtin/backfill.c          |  2 +-
 builtin/branch.c            |  2 +-
 builtin/checkout.c          |  2 +-
 builtin/clone.c             |  2 +-
 builtin/grep.c              |  2 +-
 builtin/mv.c                |  2 +-
 builtin/push.c              |  2 +-
 builtin/sparse-checkout.c   | 23 ++++++++++++-----------
 builtin/submodule--helper.c |  2 +-
 builtin/worktree.c          |  2 +-
 dir.c                       |  2 +-
 environment.c               | 28 ++++++++++++++++++----------
 environment.h               | 14 ++++++++++++--
 repository.c                |  1 +
 repository.h                |  4 ++++
 sparse-index.c              |  6 ++++--
 unpack-trees.c              |  2 +-
 wt-status.c                 |  3 ++-
 20 files changed, 68 insertions(+), 42 deletions(-)

 Range diff versus v3:
 =====================
1:  1aa41da833 ! 1:  d28850bcdb environment: stop storing `core.attributesFile` globally
    @@ Metadata
      ## Commit message ##
         environment: stop storing `core.attributesFile` globally
     
    -    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.
    +    The `core.attributeFile` 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 it being overwritten by another repository 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()`.
    +    other repository dependent values parsed by `git_default_config()`.
    +    For now the value can be accessed via the_repository in
    +    `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()`.
    +    `git_default_*_config()` functions 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.
    +    the change has to be made `git_default_core_config()`.
     
         Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
         Mentored-by: Christian Couder <christian.couder@gmail.com>
    @@ attr.c: const char *git_attr_system_file(void)
     -	if (!git_attributes_file)
     -		git_attributes_file = xdg_config_home("attributes");
     +	struct repo_config_values *cfg = &the_repository->config_values;
    -+	if (!cfg->attributes_file_path)
    -+		cfg->attributes_file_path = xdg_config_home("attributes");
    ++	if (!cfg->attributes_file)
    ++		cfg->attributes_file = xdg_config_home("attributes");
      
     -	return git_attributes_file;
    -+	return cfg->attributes_file_path;
    ++	return cfg->attributes_file;
      }
      
      int git_attr_system_is_enabled(void)
    @@ environment.c: static int git_default_core_config(const char *var, const char *v
      	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);
    ++		FREE_AND_NULL(cfg->attributes_file);
    ++		return git_config_pathname(&cfg->attributes_file, var, value);
      	}
      
      	if (!strcmp(var, "core.bare")) {
    @@ environment.c: 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->attributes_file = NULL;
     +}
     
      ## environment.h ##
    @@ environment.h: extern const char * const local_repo_env[];
      
      struct strvec;
      
    -+/* Config values parsed by git_default_config() */
     +struct repo_config_values {
    -+	/* core config values */
    -+	char *attributes_file_path;
    ++	/* section "core" config values */
    ++	char *attributes_file;
     +};
     +
      /*
2:  fd95169de4 ! 2:  e44ce7be3d environment: environment: stop using core.sparseCheckout globally
    @@ Metadata
     Author: Olamide Caleb Bello <belkid98@gmail.com>
     
      ## Commit message ##
    -    environment: environment: stop using core.sparseCheckout globally
    +    environment: stop using core.sparseCheckout globally
     
         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.
    +    `core_apply_sparse_checkout`. This could cause it to be overwritten
    +    by another repository when different Git repositories run in the same
    +    process.
     
    -    Move the parsed value into `struct repo_config_values` to retains current
    -    behaviours while achieving the repository scoped access.
    +    Move the parsed value into `struct repo_config_values` in the_repository
    +    to retain current behaviours and move towards libifying Git.
     
         Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
         Mentored-by: Christian Couder <christian.couder@gmail.com>
    @@ builtin/backfill.c: int cmd_backfill(int argc, const char **argv, const char *pr
      
      	if (ctx.sparse < 0)
     -		ctx.sparse = core_apply_sparse_checkout;
    -+		ctx.sparse = repo->config_values.sparse_checkout;
    ++		ctx.sparse = the_repository->config_values.apply_sparse_checkout;
      
      	result = do_backfill(&ctx);
      	backfill_context_clear(&ctx);
    @@ builtin/clone.c: static int git_sparse_checkout_init(const char *repo)
      	 * for the later checkout to use the sparse-checkout file.
      	 */
     -	core_apply_sparse_checkout = 1;
    -+	the_repository->config_values.sparse_checkout = 1;
    ++	the_repository->config_values.apply_sparse_checkout = 1;
      
      	cmd.git_cmd = 1;
      	if (run_command(&cmd)) {
    @@ builtin/grep.c: static int grep_submodule(struct grep_opt *opt,
      	 *	of these submodules are expanded unexpectedly.
      	 *
     -	 * 2. "core_apply_sparse_checkout"
    -+	 * 2. "sparse_checkout"
    ++	 * 2. "config_values.apply_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: int cmd_mv(int argc,
      
      		if (ignore_sparse &&
     -		    core_apply_sparse_checkout &&
    -+		    the_repository->config_values.sparse_checkout &&
    ++		    the_repository->config_values.apply_sparse_checkout &&
      		    core_sparse_checkout_cone) {
      			/*
      			 * NEEDSWORK: we are *not* paying attention to
    @@ builtin/sparse-checkout.c: static int sparse_checkout_list(int argc, const char
      
      	setup_work_tree();
     -	if (!core_apply_sparse_checkout)
    -+	if (!the_repository->config_values.sparse_checkout)
    ++	if (!the_repository->config_values.apply_sparse_checkout)
      		die(_("this worktree is not sparse"));
      
      	argc = parse_options(argc, argv, prefix,
    @@ builtin/sparse-checkout.c: 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)
    ++	if (*cone_mode == -1 && the_repository->config_values.apply_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;
    ++	the_repository->config_values.apply_sparse_checkout = 1;
      	if (*cone_mode == 1 || *cone_mode == -1) {
      		core_sparse_checkout_cone = 1;


-- 
2.34.1