Hi Stolee! Thanks for coming to the Libification discussion today, it was nice to be able to discuss this idea with a bigger group. As is custom, I'll repost my own thoughts here. If folks are interested in a summary of the whole discussion, you can find the notes at https://docs.google.com/document/d/1mw_qPPgfQUv1gfKMwmvZjpYaOOzxcNH2N8bRbvBWfIQ/edit#heading=h.fcm7uhwlk55z "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > * Is this a worthwhile category of bug to protect against? If we look past the concrete bugs and consider the general problem of process-wide state being hard to use correctly, I think this is definitely worth solving. And in case anyone doubts the the current interface is hard to use correctly: the state lives in poorly scoped global variables that need to be manually initialized by calling git_config() at the right time with git_default_config. It's also hard to remember to do this for your builtin because some builtins call git_default_config in _their own_ outer config_fn_t, and others call git_config(git_default_config). > * Is wrapping global state in accessor methods the right way to protect > against that class of bugs? Lazy-loading via accessor methods seem slightly excessive. The crux of the problem is that we haven't initialized the values before they are read, so I think we can get away with plain fields in a struct as long as they are always initialized (e.g. the builtin author doesn't need to remember to do this). We could initialize all of the fields during the setup process, where you placed declare_config_available(), at which point config should be available. If we use config hash lookups to intialize the values we want, pre-initializing like this shouldn't be noticeably more wasteful compared to lazy-loading, since in either case, we are already parsing all of the config into the in-memory config sets and looking them up with hash lookups. Pre-initializing does a small bit more work upfront by assigning to the fields, but it means that accessing the settings later can be faster since we can avoid the "getter method" calls. > * Are there other benefits to doing this? Yes! I'm generally excited about encapsulating loose global variables into an appropriate abstraction, e.g. it was something I was thinking about this when I was working on protected config (which is loose global state, but also sort of a collection of repo-wide settings). It'll be extremely relevant for libification, since this paves the way to eventually encapsulate the process-wide state in a way that makes it separable from the library code. > * Are there reasons why this cure is worse than the disease? This seems quite similar to what we're doing with repo-settings.c (which also has its own problems with initialization), and I'd like to avoid having two similar-looking-but-different systems in the long run. For this to go forward, I'd like to see either an explicit goal to deprecate repo-settings.c (e.g. this new system handles repository settings in addition to process-wide settings), or a reasoned separation between them.
