Re: [PATCH 0/6] [RFC] config.c: use struct for config reading state

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

On Wed, Mar 01 2023, Glen Choo via GitGitGadget wrote:

> This RFC is preparation for config.[ch] to be libified as as part of the
> libification effort that Emily described in [1]. One of the first goals is
> to read config from a file, but the trouble with how config.c is written
> today is that all reading operations rely on global state, so before turning
> that into a library, we'd want to make that state non-global.
>
> This series gets us about halfway there; it does enough plumbing for a
> workable-but-kinda-ugly library interface, but with a little bit more work,
> I think we can get rid of global state in-tree as well. That requires a fair
> amount of work though, so I'd like to get thoughts on that before starting
> work.
>
> = Description
>
> This series extracts the global config reading state into "struct
> config_reader" and plumbs it through the config reading machinery. It's very
> similar to how we've plumbed "struct repository" and other 'context objects'
> in the past, except:
>
>  * The global state (named "the_reader") for the git process lives in a
>    config.c static variable, and not on "the_repository". See 3/6 for the
>    rationale.

I agree with the overall direction, but don't think that rationale in
3/6 is sufficient to go in this "the_reader" direction, as opposed to
sticking with and extending "the_repository" approach.

For orthagonal reasons (getting rid of some of the API duplication) I've
been carrying a patch to get rid of the "configset" part of the *public*
API, i.e. to have API users always use the "repo_config_*()" or
"git_config_*()" variants, that patch is at:
https://github.com/avar/git/commit/0233297a359bbda43a902dd0213aacdca82faa34

All of the rationale in your 3/6 is true now, but as that patch shows
the reason for why we have "the_repository" is for the trivial reason
that we want to access the repo's "config" member.

It's a bit distasteful, but that change argues that just mocking up a
"struct repository" with a "config" member pointing to a new configset
is better than maintaining an entirely different API just for those
cases where we need to parse a one-off file or whatever.

I think that going in that direction neatly solves the issues you're
noting here and in your 3/6, i.e. we'd always have this in the "repo"
object, so we'd just stick the persistent "reader" variables in the
"struct repository"'s "config" member.