Re: [RFC PATCH 0/5] bypass config.c global state with configset

From: Glen Choo <chooglen@google.com>
Date: Mar 17, 2023, 19:20 UTC
Message-ID: <kl6la60buquj.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <RFC-cover-0.5-00000000000-20230317T042408Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 15 quoted lines

> Maybe it still makes sense to go for this "the_reader" intermediate
> step, but I can't help but think that we could just go for it all in
> one leap, and that you've just got stuck on thinking that you needed
> to change "config_fn_t" for all its callers.
>
> As the 5/5 here shows we have various orthagonal uses of the
> "config_fn_t" in config.c, and can just implement a new callback type
> for the edge cases where we need the file & line info.
>
> This still leave the current_config_name() etc, which
> e.g. builtin/config.c still uses. In your series you've needed to add
> the new "reader" parameter for everything from do_config_from(), but
> if we're doing that can't we instead just go straight to passing a
> "struct key_value_info *" (perhaps with an added "name" field) all the
> way down, replacing "cf->linenr" etc?

In the end state, I also think we should be passing "struct key_value_info *" around instead of "cf", so I think we are seeing "the_reader" in the same way (as a transitional state).

I considered the "repo_config_kvi() + config_fn_kvi_t" as well, but I rejected it (before discussion on the list, whoops) because I didn't want to add _yet another_ set of parallel config APIs, e.g. we already have repo_config(), git_config(), configset*(), git_config_from_file*(). Multiplying that by 2 to add *_kvi() seems like way too much, especially when it seems clear that our current definition of config_fn_t has some problems.

Maybe we could deprecate the non-*_kvi(), and have both as a transitional state? It might work, but I think biting the bullet and changing config_fn_t would be easier actually.

I'll try applying your series on top of my 1/8 (and maybe 7/8, see next reply) and extending it to cover "cf" (instead of just current_config_kvi()) to see whether the *_kvi() approach saves a lot of unnecessary plumbing. I'd still feel very strongly about getting rid of all of the non *_kvi() versions, though, but maybe that would happen in a cleanup topic.

Show 9 quoted lines

> Similarly, you mention git_parse_int() wanting to report a filename
> and/or line number. I'm aware that it can do that, but it doesn't do
> so in the common case, e.g.:
>
> 	git -c format.filenameMaxLength=abc log
> 	fatal: bad numeric config value 'abc' for 'format.filenamemaxlength': invalid unit
>
> And the same goes for writing it to e.g. ~/.gitconfig. It's only if
> you use "git config --file" or similar that we'll report a filename.

That's true, but I think that's a bug, not a feature. See 7/8 [1] where I addressed it.

1.  https://lore.kernel.org/git/3c83d9535a037653c7de2d462a4df3a3c43a9308.1678925506.git.gitgitgadget@gmail.com/

Show 5 quoted lines

> We can just make config_set_callback() and configset_iter()
> non-static, so e.g. the builtin/config.c caller that implements
> "--show-origin" can keep its config_with_options(...) call, but
> instead of "streaming" the config, it'll buffer it up into a
> configset.

Hm, so to extrapolate, we could make it so that nobody outside of config.c uses the *config_from_file() APIs directly. Instead, all reads get buffered up into a configset. That might not be a bad idea. It would definitely help with some of your goals of config API surface reduction.

This would be friendlier in cases where we were already creating custom configsets (I know we have some of those, but I don't recall where), but in cases where we were reading the file directly (e.g. builtin/config.c), we'd be taking a memory and runtime hit. I'm not sure how I (or others) feel about that yet.