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

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

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