Re: [PATCH v3] environment: move access to "core.sparsecheckout" into repo_settings

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

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

> The setting "core.sparsecheckout" is stored in the global
> `core_apply_sparse_checkout` and is populated in config.c. Refactor the
> code to store it in the variable `sparse_checkout` in the struct
> `repo_settings`. Also, create functions to set and get the value of the
> setting and update all the occurrences.
>
> This also allows us to remove the definition `#define
> USE_THE_REPOSITORY_VARIABLE` from the file 'builtin/backfill.c'.
>
> This change is part of an ongoing effort to eliminate global variables,
> improve modularity and help libify the codebase.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---

It may make sense to move the sparse-checkout bit from the
process-wide global to per-repository settings.  One advantage of
having it at the global level is that the code that does not require
to be in a repository can refer to it, but the nature of this variable
requires a repository to make sense anyway, so that advantage does
not apply to it.

But looking at members of repo.settings, among 25+ of them, there
are only a handful of variables with getter-setter pair, and this
patch seems to add yet another for a simple boolean.  Among existing
setters and getters, prevailing pattern seem to be

	/* getter */
	if (!repo->settings.foo)
		repo_cfg_ulong(repo, "core.foo", &repo->settings.foo, FOO_DEFAULT);
	return repo->settings.foo;

	/* setter */
	repo->settings.foo = value;

but there are more involved ones.

This new one adopts the most expensive pattern from
get_warn_ambiguous_refs() where prepare_repo_settings() is used, as
if it fears to be fed a repo instance that hasn't been prepared yet,
and it even does so for its setter, which I think is probably a
mistake.

And there are tons of members in repo-settings that are initialized
in prepare_repo_settings() (perhaps reading from the configuration
file or assigned their default values) and without having an
explicit setter/getter pair.  If the code paths that depends on the
value of these members are functioning correctly (and they should,
given how widely Git is used these days and not having heard about
bugs coming from these variables), would not it mean that in the
program start up sequence, prepare_repo_settings() is called early
enough so that all the code paths that care about these repository
settings values do not have to worry about having a getter setter
pair at all?

What is it so special about sparse-checkout bit that cannot be using
the same "initialize in prepare_repo_settings() and access it
directly thereafter" model, or "ah, do you need to know about this
bit?  Let me see if I already have figured it out, and otherwise let
me read from the configuration, and give it back to you" model?

My gut feeling, if I have to choose between "lazy loading" and
"popluate in prepare_repo_settings() and then access the member
directly thereafter" for this variable, I may pick the latter for
this particular variable.