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

>
> 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 v3:
> ==============
> - Moved declaration and definition of repo_config_values_init into patch 1
> - Returned commit message in line 1 of builtin/backfill.c
>
> Olamide Caleb Bello (3):
>   environment: stop storing `core.attributesFile` globally
>   environment: 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   | 22 +++++++++++-----------
>  builtin/submodule--helper.c |  2 +-
>  builtin/worktree.c          |  2 +-
>  dir.c                       |  2 +-
>  environment.c               | 28 ++++++++++++++++++----------
>  environment.h               | 15 +++++++++++++--
>  repository.c                |  1 +
>  repository.h                |  4 ++++
>  sparse-index.c              |  6 ++++--
>  unpack-trees.c              |  2 +-
>  wt-status.c                 |  2 +-
>  20 files changed, 67 insertions(+), 42 deletions(-)
>
>  Range diff versus v2:
>  =====================
>
>  1:  b6f8deaa40 ! 1:  1aa41da833 environment: stop storing `core.attributesFile` globally
>     @@ Metadata
>       ## Commit message ##
>          environment: stop storing `core.attributesFile` globally
>
>     -    The config value parsed in git_default_core_config() is loaded eagerly
>     +    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.
>
>     -    Move this value into a `struct repo_config_values` which holds all the
>     -    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.
>     +    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()`.
>     @@ environment.c: static int git_default_core_config(const char *var, const char *v
>         }
>
>         if (!strcmp(var, "core.bare")) {
>     +@@ environment.c: 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;
>     ++}
>
>       ## environment.h ##
>      @@ environment.h: extern const char * const local_repo_env[];
>     @@ environment.h: extern const char * const local_repo_env[];
>       /*
>        * Wrapper of getenv() that returns a strdup value. This value is kept
>        * in argv to be freed later.
>     +@@ environment.h: 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
>      @@ environment.h: extern int assume_unchanged;
>       extern int warn_on_object_refname_ambiguity;
>       extern char *apply_default_whitespace;
>     @@ environment.h: extern int assume_unchanged;
>       extern int pack_compression_level;
>       extern unsigned long pack_size_limit_cfg;
>
>     + ## repository.c ##
>     +@@ repository.c: 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 ##
>      @@
>
> 2:  1e83c077f2 ! 2:  fd95169de4 environment: environment: stop using core.sparseCheckout globally
>     @@ Commit message
>          when different Git repositories running in the same process access this
>          variable.
>
>     -    Move the parsed value into `struct repo_config_values` which holds all the
>     -    values parsed by `git_default_config()` and can be accessed
>     -    per repo via `git_default_config()`. This retains current
>     +    Move the parsed value into `struct repo_config_values` to retains current
>          behaviours while achieving the repository scoped access.
>
>          Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>     @@ Commit message
>          Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
>
>       ## builtin/backfill.c ##
>     -@@
>     --/* We need this macro to access core_apply_sparse_checkout */
>     - #define USE_THE_REPOSITORY_VARIABLE
>     -
>     - #include "builtin.h"
>      @@ builtin/backfill.c: int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>         repo_config(repo, git_default_config, NULL);
>
>     @@ environment.c: static int git_default_core_config(const char *var, const char *v
>                 return 0;
>         }
>
>     +@@ 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->sparse_checkout = 0;
>     + }
>
>       ## environment.h ##
>      @@ environment.h: struct strvec;
> 3:  6e54e22ac7 ! 3:  9a411db9f8 environment: move "branch.autoSetupMerge" into `struct repo_config_values`
>     @@ Commit message
>          `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
>     +    Move this value into `struct repo_config_values` to retain current
>          behaviours while achieving repository scoped access.
>
>          Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>     @@ environment.c: static int git_default_i18n_config(const char *var, const char *v
>                 return 0;
>         }
>         if (!strcmp(var, "branch.autosetuprebase")) {
>     -@@ environment.c: 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)
>     -+{
>     +@@ environment.c: void repo_config_values_init(struct repo_config_values *cfg)
>     + {
>     +   cfg->attributes_file_path = NULL;
>     +   cfg->sparse_checkout = 0;
>      +  cfg->git_branch_track = BRANCH_TRACK_REMOTE;
>     -+}
>     + }
>
>       ## environment.h ##
>      @@
>     @@ environment.h: struct repo_config_values {
>       };
>
>       /*
>     -@@ environment.h: 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
>     -
>     - ## repository.c ##
>     -@@ repository.c: 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
>
>