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

[Glen Choo <chooglen@google.com>]

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

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

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

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