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

From: Glen Choo <chooglen@google.com>
Date: May 31, 2022, 17:43 UTC
Message-ID: <kl6lv8tlegy4.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqbkvi1owx.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 13 quoted lines

>> Protected config is read using `read_very_early_config()`, which has
>> several downsides:
>>
>> - Every call to `read_very_early_config()` parses global and
>>   system-level config files anew, but this can be optimized by just
>>   parsing them once [1].
>> - Protected variables should respect "-c" because we can reasonably
>>   assume that it comes from the user. But, `read_very_early_config()`
>>   can't use "-c" because it is called so early that it does not have
>>   access to command line arguments.
>
> Now we are talking about protected "variable".  Is that a synonym
> for "config", or are there some distinctions between them?

Sorry, that's an old term I was toying with (this somehow snuck through my proofreading). I just meant "variable that is only read from protected config", aka a "protected config only variable".

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.

Show 8 quoted lines

>> - 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.

Show 5 quoted lines

> I am wondering if it is a cleaner solution *not* to hang the
> protected config as a configset in the_repository, but keep the
> configset as a separate global variable, perhaps static to config.c
> and is meant to be only accessed via git_protected_config() and the
> like.

I think your suggestion to use a global variable is better, as much as I want to avoid another global variable. Protected config would affect any repositories that we work with in-core, so using a global sounds ok.

environment.c might be a better place since we already make a concerted effort to put global config variables there instead of config.c.

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.

Show 14 quoted lines

>> @@ -295,6 +295,11 @@ void repo_clear(struct repository *repo)
>>  		FREE_AND_NULL(repo->remote_state);
>>  	}
>>  
>> +	if (repo->protected_config) {
>> +		git_configset_clear(repo->protected_config);
>> +		FREE_AND_NULL(repo->protected_config);
>> +	}
>> +
>
> This becomes necessary only because each repository instance has
> protected_config, even though we need only one instance, no matter
> how many repositories we are accessing in this single invocation of
> Git, no?

Yes.

Show 5 quoted lines

> How should "git config -l" interact with "protected config" and
> "protected variables", by the way?  Should a user be able to tell
> which ones are coming from protected scope?  Should we gain, next to
> --global, --system, etc., --protected option to list only the
> protected config/variable?

I'll have to think about this some more. My initial thoughts are that we should do this if we formalize 'protected' as a scope-like concept, but I don't see the lack of "--protected" as a significant hindrance to users because they can use "--global" and "--system" (albeit in two invocations instead of one).

Show 7 quoted lines

> This is another thing that I find iffy on terminology.  Should a
> random variable, like user.name, be a "protected config", if it is
> found in $HOME/.gitconfig?  If it comes from there, surely we can
> trust its value, but unlike things like safe.directory, there is no
> code that wants to enforce that we pay attention only to user.name
> that came from trusted scopes.  Should such a variable be called
> "protected variable"?

Ah.. I think it would be best to pretend that the "Protected variable" typo never happened. That term was destined to be confusing and meaningless.

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".