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

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

On Fri, Mar 17 2023, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> [...]
>> 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.

I'm pretty sure that in the end this wouldn't matter, i.e. the time it
takes to parse the config is trivial, and the users of these APIs like
"git config -l --show-origin" aren't performance-senitive.

But for the general case & if you're concerned about this a trivial
addition on top of what I suggested would be to pass a streaming
callback to config_set_callback(), i.e. you could get the values you'd
get from configset_iter() as we parse them.