From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 19 Jan 2024 06:18:32 GMT
Subject: Re: [PATCH v2 3/4] config: factor out global config file retrieval
Message-ID: <ZaoUOPsze7rhtT2M@tanuki>
In-Reply-To: <32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name>

On Sun, Jan 14, 2024 at 10:43:18PM +0100, Kristoffer Haugsbakk wrote:
> Factor out code that retrieves the global config file so that we can use
> it in `gc.c` as well.
> 
> Use the old name from the previous commit since this function acts
> functionally the same as `git_system_config` but for “global”.

I was briefly wondering whether we also want to give this new function a
more descriptive name. For one, calling it `git_system_config()` which
we have just removed in the preceding set may easily lead to confusion
for any in-flight patch series because the parameters now got dropped
(or at least it looks like that).

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()`.

Sorry for not mentioning this in my first review round. Also, it's only
a minor concern, nothing that needs to block this series if either you
or others disagree with my opinion.

Patrick