Re: [PATCH v3 3/8] config.c: create config_reader and the_reader

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Mar 28 2023, Glen Choo via GitGitGadget wrote:

> From: Glen Choo <chooglen@google.com>
>
> Create "struct config_reader" to hold the state of the config source
> currently being read. Then, create a static instance of it,
> "the_reader", and use "the_reader.source" to replace references to
> "cf_global" in public functions.
>
> This doesn't create much immediate benefit (since we're mostly replacing
> static variables with a bigger static variable), but it prepares us for
> a future where this state doesn't have to be global; "struct
> config_reader" (or a similar struct) could be provided by the caller, or
> constructed internally by a function like "do_config_from()".
>
> 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".

I don't mean to just rehash previous discussion
(i.e. https://lore.kernel.org/git/230307.86wn3szrzu.gmgdl@evledraar.gmail.com/
and downthread). I get that you think sticking this in a "struct
repository *" here isn't clean, and would prefer to not conflate the
two.

Fair enough.

But I think this paragraph still does a bad job of justifying this
direction with reference to existing code.

Why? Because from reading it you get the impression that with
read_very_early_config() and read_protected_config() "config reading is
not scoped to a repository", but "scoped to" is doing a *lot* of work
here.

At the start of read_very_early_config() we do:

	struct config_options opts = { 0 };
	[...]
	opts.ignore_repo = 1;
	opts.ignore_worktree = 1;

And then call config_with_options(), which does:

	struct config_include_data inc = CONFIG_INCLUDE_INIT;

And that struct has:

	struct git_config_source *config_source;

Which in turn has:

	/* The repository if blob is not NULL; leave blank for the_repository */
	struct repository *repo;
	const char *blob;

The read_protected_config() is then another thin wrapper for
config_with_options().

So, so far the reader might be genuinely confused, since we already have
a "repo" in scope why can't we use it for this cache? Even if just
reading the system config etc.

For *those* cases I think what I *think* you're going for is that while
we have a "struct repository" already, we don't want to use it for our
"cache", and instead have a file-scoped one.

Personally, I don't see how it's cleaner to always use a file-scope
rather than piggy-back on the global we almost always have (or provide a
fallback), but let's not get on that topic again :)

Now, the case that *is* special on the other hand is
git_config_from_file(), there we really don't have a "repository" at
all, as it never gets the "struct config_include_data inc", or a
"git_config_source".

But if we dig a bit into those cases there's 3x users of
git_config_from_file() outside of config.c itself:

 * setup.c, to read only repo's "config.worktree"
 * setup.c, to read only repo "config"
 * sequencer.c, to read "sequencer/opts"

For the former two, I think the only thing that's needed is something
like this, along with a corresponding change to
do_git_config_sequence():

	diff --git a/config.h b/config.h
	index 7606246531a..b8a3de4eb93 100644
	--- a/config.h
	+++ b/config.h
	@@ -85,7 +85,10 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type,
	 
	 struct config_options {
	        unsigned int respect_includes : 1;
	+       unsigned int ignore_system : 1;
	+       unsigned int ignore_global : 1;
	        unsigned int ignore_repo : 1;
	+       unsigned int ignore_local : 1;
	        unsigned int ignore_worktree : 1;
	        unsigned int ignore_cmdline : 1;
	        unsigned int system_gently : 1;

I.e. we actually *do* have a repo there, we just haven't bridged the gap
of "ignore most of its config" so we can use config_with_options()
there.

The sequencer.c case is trickier, but presumably for such isolated
reading we could have a lower-level function which would return the
equivalent of a "key_value_info" on errors or whatever.


Anyway, I'm fine with this direction for now, but given the above & my
previous RFC
https://lore.kernel.org/git/RFC-cover-0.5-00000000000-20230317T042408Z-avarab@gmail.com/
I can't help but think we're taking two steps forward & one step
backwards for some of this.

I.e. are we assuming no "repo", but per the above we really do have one,
but we just don't pass it because we don't have a "read only the
worktree config part", or whatever?

Ditto the line number relaying for builtin/config.c, which as my RFC
showed we have one or two API users that care, which we can just
convert...