Re: [PATCH v3 2/5] config: read protected config with `git_protected_config()`

From: Junio C Hamano <gitster@pobox.com>
To: Glen Choo <chooglen@google.com>
Cc: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Taylor Blau <me@ttaylorr.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Derrick Stolee <derrickstolee@github.com>,
	Emily Shaffer <emilyshaffer@google.com>
Subject: Re: [PATCH v3 2/5] config: read protected config with `git_protected_config()`
Date: Wed, 01 Jun 2022 08:58:53 -0700	[thread overview]
Message-ID: <xmqqwne0tlxe.fsf@gitster.g> (raw)
In-Reply-To: <kl6lv8tlegy4.fsf@chooglen-macbookpro.roam.corp.google.com> (Glen Choo's message of "Tue, 31 May 2022 10:43:15 -0700")

Glen Choo <chooglen@google.com> writes:

> A goal in this version was to introduce as little jargon as possible, so
> - "protected config" refers to the set of config sources, and
> - "protected config only" refers to config variables/settings that are
>   only read from protected config.

OK.  Let's have such a clear pair of definitions somewhere in the
doc or at least in a proposed log message.

>
>>> - Protected config is stored in `the_repository` so that we don't need
>>>   to statically allocate it. But this might be confusing since protected
>>>   config ignores repository config by definition.
>>
>> Yes, it indeed is.  Is it because we were over-eager when we
>> introduced the "struct repository *repo" parameter to many functions
>> and the configuration system wants you to have some repository, even
>> when you know you are not reading from any repository?  
>
> Ah no, I was just trying to avoid yet-another global variable (since
> IIRC we want to move towards a more lib-like Git), and the_repository
> was a convenient global variable to (ab)use.

If this does not have to be known only inside config.c, until we
introduce a more global bag of things, which may have the current
the_repository as one of its components, I do not think it hurts to
have a file-scope static there.  Then, perhaps git_configset_get*()
helper functions can recognize cs==NULL as a sign that the caller
wants to grab from the "protected config", or something?  If we do
not want to expose the underying global variable to the public, that
is.

> As an aside, I wonder how we could get rid of all of the globals in
> environment.c in the long term. Maybe we would have yet-another all
> encompassing global, the_environment, and then figure out which
> variables belong to the repository and which belong to the environment.

I think we are on the same page, we'd probably need something called
the_world ;-)

> Instead, we can use "protected config" to refer to the config and
> "protected config only" to refer to variables. Since "protected config"
> is defined as (global + system + CLI) config, then yes, we would say
> that it is "protected config". But since we do not enforce that
> "user.name" _must_ come from only protected config, it is not "protected
> config only".

Very clear.  Thanks.