Re: [Outreachy PATCH v2 3/3] environment: move "branch.autoSetupMerge" into `struct repo_config_values`

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

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

> 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 <phillip.wood123@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> ---

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.


> 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);

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.

Thanks.