On Fri, Apr 09, 2021 at 03:43:25PM +0200, Patrick Steinhardt wrote:
> There's two callsites which assemble global config paths, once in the
> config loading code and once in the git-config(1) builtin. We're about
> to implement a way to override global config paths via an environment
> variable which would require us to adjust both sites.
>
> Unify both code paths into a single `git_global_config()` function which
> returns both paths for `~/.gitconfig` and the XDG config file. This will
> make the subsequent patch which introduces the new envvar easier to
> implement.
Seems like a good step forward. There is one minor issue with the
implementation, though.
> diff --git a/builtin/config.c b/builtin/config.c
> index 02ed0b3fe7..604a0973a5 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -671,9 +671,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> }
>
> if (use_global_config) {
> - char *user_config = expand_user_path("~/.gitconfig", 0);
> - char *xdg_config = xdg_config_home("config");
> + const char *user_config, *xdg_config;
>
> + git_global_config(&user_config, &xdg_config);
The pointer out-parameters make sense here, since we need to return two
values. I notice they became const, so the function will hold on to
ownership of the memory.
> @@ -688,10 +688,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> if (access_or_warn(user_config, R_OK, 0) &&
> xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> given_config_source.file = xdg_config;
> - free(user_config);
> } else {
> given_config_source.file = user_config;
> - free(xdg_config);
> }
...which is why we drop these free() calls. So far so good.
> +void git_global_config(const char **user, const char **xdg)
> +{
> + static const char *user_config, *xdg_config;
> +
> + if (!user_config) {
> + user_config = expand_user_path("~/.gitconfig", 0);
> + xdg_config = xdg_config_home("config");
> + }
> +
> + *user = user_config;
> + *xdg = xdg_config;
> +}
And here in the implementation we hold on to the static values forever.
I think your "did we initialize already" check isn't robust, though.
expand_user_path() can return NULL, in which case every call would
trigger a re-initialization (even leaking xdg_config if it was set in
the last round).
So I think you'd need a separate "static int initialized" variable.
That said, I wonder if we should just pass ownership of the memory to
the caller. It is a minor inconvenience that they will have to free()
the result, but we're already doing that. And it removes any possibility
of thread unsafety.
I guess it doesn't match git_system_config() as well, then. But arguably
it should also just pass ownership (it also has only a handful of
callers, and freeing the result would not be a big deal).
I'm OK with either solution, though.
-Peff
