Re: [PATCH 0/6] [RFC] config.c: use struct for config reading state

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 8, 2023, 09:17 UTC
Message-ID: <230308.867cvrziac.gmgdl@evledraar.gmail.com>
In-Reply-To: <kl6lcz5ki36g.fsf@chooglen-macbookpro.roam.corp.google.com>

On Tue, Mar 07 2023, Glen Choo wrote:

Show 24 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> If you include "populate from system-wide, per-user, and repository
>> specific configuration files" as part of the API being libified,
>> your configsets cannot avoid being tied to a repository.  But I do
>> not think the reader needs to be in the repository.
>>
>> [...]
>>
>> Isn't the use of the reader object purely transitory while you
>> populate the keys and values in a configset from a single file?  At
>> the layer to read and populate a configset from a single "source"
>> file, you still do not need repository.
>>
>> [...]
>> Only when you say "I have a 'repo' instance and I want to read the
>> config variables from appropriate places", you call such a "read and
>> populate configset from a single source" helper three or four times
>> to populate repo->config.  Once a configset is populated, it or its
>> contents do not depend on the reader instance to function, so I do
>> not see how it benefits to have the reader in the repository object.
>
> Yes, exactly. Having a config_set on the repository makes sense, but I
> don't see a good reason to have the reader on the repository.

Isn't Junio suggesting something different here?

I hadn't looked closely at this aspect of it, and just took it as a given that we needed to persist this data outside of the configset lifetime.

If that's not the case then we don't need it in the file scope, nor a "struct repository" or whatever, and could just have it materialized by git_config_check_init(), no? I.e. when we create the configset we'd create it, and throw it away after the configset is created?

I.e. to address this note in your initial RFC:

	I think we can get rid of global state in-tree as well. That requires a fair
	amount of work though, so I'd like to get thoughts on that before starting
	work.

> If Ævar sends his series soon, it would be fruitful to see how that
> series interacts with this one :)

I tried merging this topic with that, and it didn't conflict textually
or semantically. I just raised it because I think with your 3/6 you're
needlessly tying yourself in knots, i.e. with this part:
	
	A more typical approach would be to put this struct on "the_repository",
	but that's a worse fit for this use case since config reading is not
	scoped to a repository. E.g. we can read config before the repository is
	known ("read_very_early_config()"), blatantly ignore the repo
	("read_protected_config()"), or read only from a file
	("git_config_from_file()"). This is especially evident in t5318 and
	t9210, where test-tool and scalar parse config but don't fully
	initialize "the_repository".

Out of those examples read_very_early_config() and read_protected_config() already call config_with_options(), which optionally uses a "struct repository" via the "repo" member of "struct git_config_source".

I think we may have gotten lost in the weeds over what amounts to an asthetic preference about how to do polymorphism in C. I'd be fine with mocking up the "struct repository", but you could equally prepare and pass some new "config_state" struct that would contain the required information (including the configset).

As well as this in the CL:

	The catch (aka the reason I stopped halfway through) is that I
	couldn't find a way to expose "struct config_reader" state
	without some fairly big changes, complexity-wise or LoC-wise[...]

I didn't look into exactly why config_fn_t would need your new "reader", but if you accept that we could stick such a thing into "the_repository" then there's no catch or need for the churn to change all those callbacks.

Of course something that wants to use the API as a "real" library would need to use some alternate mechanism, as you reference in adding a new "config_state_fn_t". You note:

	but I couldn't find a great way to do this kind of 'switching
	between config callback types' elegantly.

So, I don't know, but I was suggesting looking into doing that based on "the_repository" in play...