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.
