Re: [PATCH v2 3/4] config: factor out global config file retrieval

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 19, 2024, 07:59 UTC
Message-ID: <Zaor5zNLKE3UXhHM@tanuki>
In-Reply-To: <7f0864ad-c846-42a6-8ddc-85d6be58a4ee@app.fastmail.com>

On Fri, Jan 19, 2024 at 08:40:51AM +0100, Kristoffer Haugsbakk wrote:

Show 23 quoted lines

> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:
> > But second, I think that the new function you introduce here has the
> > same issue as the old function that you refactored in the preceding
> > patch: `git_config_global()` isn't very descriptive, and it is also
> > inconsistent the new `git_config_global_paths()`. I'd propose to name
> > the new function something like `git_config_global_preferred_path()` or
> > `git_config_global_path()`.
> 
> The choice of `git_config_global` is mostly motivated by it working the
> same way as `git_config_system`:
> 
> ```
> given_config_source.file = git_system_config();
> […]
> given_config_source.file = git_global_config();
> ```
> 
> (The extra logic imposed by XDG for “global” is implied by `man git
> config`. I don’t know what the guidelines are for spelling that out or not
> in the internal functions.)
> 
> Your suggestion makes sense. But should `git_system_config` be renamed as
> well?

Yeah, you're right that `git_system_config()` is bad in the same way. In fact I think it's worse here because we have both `git_config_system()` and `git_system_config()`, which has certainly confused me multiple times in the past. So I'd be happy to see it renamed, as well, either now or in a follow-up patch series.

But as I said, I don't think it's a prereq for this patch series to land and others may have differing opinions. So please, go ahead as you deem fit (or wait for other opinions).

Patrick