| Message ID | 32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | maintenance: use XDG config if it exists | expand |
Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > if (use_global_config) { > - char *user_config, *xdg_config; > ... > - else if (use_system_config) { > + } else if (use_system_config) { > given_config_source.file = git_system_config(); > given_config_source.scope = CONFIG_SCOPE_SYSTEM; > } else if (use_local_config) { > diff --git a/config.c b/config.c > index ebc6a57e1c3..3cfeb3d8bd9 100644 > --- a/config.c > +++ b/config.c > @@ -1987,6 +1987,26 @@ char *git_system_config(void) > return system_config; > } > > +char *git_global_config(void) > +{ > +... > +} > + > void git_global_config_paths(char **user_out, char **xdg_out) > { > char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL")); The conversion above > diff --git a/config.h b/config.h > index e5e523553cc..625e932b993 100644 > --- a/config.h > +++ b/config.h > @@ -382,6 +382,10 @@ int config_error_nonbool(const char *); > #endif > > char *git_system_config(void); > +/** > + * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists. > + */ Sorry, but I am not sure what this comment wants to say. When $HOME is not set, we do get NULL out of this function. But interpolate_path() that makes git_global_config_paths() to return NULL in user_config does not do any existence check with stat() or access(), so even when we return a string that is "~/.gitconfig" expanded to '/home/user/.gitconfig", we are not certain if the file exists. So,... it is unclear what "uncertain"ty we are talking about in this case. > +char *git_global_config(void);
On Tue, Jan 16, 2024, at 22:39, Junio C Hamano wrote: > Kristoffer Haugsbakk <code@khaugsbakk.name> writes: >> char *git_system_config(void); >> +/** >> + * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists. >> + */ > > Sorry, but I am not sure what this comment wants to say. > > When $HOME is not set, we do get NULL out of this function. But > interpolate_path() that makes git_global_config_paths() to return > NULL in user_config does not do any existence check with stat() or > access(), so even when we return a string that is "~/.gitconfig" > expanded to '/home/user/.gitconfig", we are not certain if the file > exists. So,... it is unclear what "uncertain"ty we are talking > about in this case. I'll delete it. It was an attempt to refer to the comments about "It is unknown if HOME/.gitconfig exists". Cheers
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
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?
On Fri, Jan 19, 2024 at 08:40:51AM +0100, Kristoffer Haugsbakk wrote: > 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
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > 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(); > ``` I shared the above understanding with you, so I didn't find the name "not very descriptive" during my review. If only we had two more functions that can replace our uses of repo_git_path(r, "config") and repo_git_path(r, "config.worktree") [*] in the code, to obtain the path to the repository local and worktree local configuration files, the convention may have been more obvious. Side note: the worktree specific one is messier; there are code paths that use "%s/config.worktree" on gitdir as well---if we were to introduce helpers, we should catch and convert them, too. > Your suggestion makes sense. But should `git_system_config` be renamed as > well? I do not mind including "path" in the names of these functions, but I do agree that such renaming should be done consistently across the family of functions (which we currently have only two members, but still). Thanks.
On Friday, January 19, 2024 1:36 PM, Junio C Hamano wrote: >"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > >> 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(); ``` > >I shared the above understanding with you, so I didn't find the name "not very >descriptive" during my review. If only we had two more functions that can replace >our uses of repo_git_path(r, "config") and repo_git_path(r, "config.worktree") [*] in >the code, to obtain the path to the repository local and worktree local configuration >files, the convention may have been more obvious. > > Side note: the worktree specific one is messier; there are code > paths that use "%s/config.worktree" on gitdir as well---if we > were to introduce helpers, we should catch and convert them, too. > >> Your suggestion makes sense. But should `git_system_config` be renamed >> as well? > >I do not mind including "path" in the names of these functions, but I do agree that >such renaming should be done consistently across the family of functions (which >we currently have only two members, but still). Is this going to impact the libification effort? I am just curious what other on the team think. --Randall
Patrick Steinhardt <ps@pks.im> writes: > 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. OK, let's make a note #leftoverbits here and merge the topic down to 'next'. By the time it graduates to 'master', we may have a clean-up patch to rename them. Thanks, all.
diff --git a/builtin/config.c b/builtin/config.c index 6fff2655816..08fe36d4997 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix) } if (use_global_config) { - char *user_config, *xdg_config; - - git_global_config_paths(&user_config, &xdg_config); - if (!user_config) - /* - * It is unknown if HOME/.gitconfig exists, so - * we do not know if we should write to XDG - * location; error out even if XDG_CONFIG_HOME - * is set and points at a sane location. - */ + given_config_source.file = git_global_config(); + if (!given_config_source.file) die(_("$HOME not set")); - given_config_source.scope = CONFIG_SCOPE_GLOBAL; - - 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); - } - } - else if (use_system_config) { + } else if (use_system_config) { given_config_source.file = git_system_config(); given_config_source.scope = CONFIG_SCOPE_SYSTEM; } else if (use_local_config) { diff --git a/config.c b/config.c index ebc6a57e1c3..3cfeb3d8bd9 100644 --- a/config.c +++ b/config.c @@ -1987,6 +1987,26 @@ char *git_system_config(void) return system_config; } +char *git_global_config(void) +{ + char *user_config, *xdg_config; + + git_global_config_paths(&user_config, &xdg_config); + if (!user_config) { + free(xdg_config); + return NULL; + } + + if (access_or_warn(user_config, R_OK, 0) && xdg_config && + !access_or_warn(xdg_config, R_OK, 0)) { + free(user_config); + return xdg_config; + } else { + free(xdg_config); + return user_config; + } +} + void git_global_config_paths(char **user_out, char **xdg_out) { char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL")); diff --git a/config.h b/config.h index e5e523553cc..625e932b993 100644 --- a/config.h +++ b/config.h @@ -382,6 +382,10 @@ int config_error_nonbool(const char *); #endif char *git_system_config(void); +/** + * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists. + */ +char *git_global_config(void); void git_global_config_paths(char **user, char **xdg); int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
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”. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- Notes (series): v2: • Don’t die; return `NULL` builtin/config.c | 25 +++---------------------- config.c | 20 ++++++++++++++++++++ config.h | 4 ++++ 3 files changed, 27 insertions(+), 22 deletions(-)