Re: [PATCH v4 1/3] config: rename `git_etc_config()`

From: Jeff King <peff@peff.net>
Date: Apr 13, 2021, 07:25 UTC
Message-ID: <YHVHUef1sEtDNcy2@coredump.intra.peff.net>
In-Reply-To: <34bdbc27d618d7467d2caf6844d8c06bdcb8545b.1618297711.git.ps@pks.im>

On Tue, Apr 13, 2021 at 09:11:44AM +0200, Patrick Steinhardt wrote:

> While at it, the function is also refactored to pass memory ownership to
> the caller. This is done to better match semantics of
> `git_global_config()`, which is going to be introduced in the next
> commit.

I think this turned out nicely. There are only two callers, one of which is already handling freeing the global config. In the other:

Show 13 quoted lines

> diff --git a/builtin/config.c b/builtin/config.c
> index f71fa39b38..02ed0b3fe7 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -695,7 +695,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  	else if (use_system_config) {
> -		given_config_source.file = git_etc_gitconfig();
> +		given_config_source.file = git_system_config();
>  		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
>  	} else if (use_local_config) {
>  		given_config_source.file = git_pathdup("config");

We "leak" the result, but I think it is actually an improvement. As you can see in the context, we are sometimes allocating the field in other code paths, so this is takes us closer to a world where we can actually call free(given_config_source.file) to fix the leak in all of the other code paths. :)

(I don't think any of that needs to be dealt with in this series, of course).

-Peff