| Message ID | 147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | maintenance: use XDG config if it exists | expand |
On Wed, Oct 18, 2023 at 10:28:40PM +0200, 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”. > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > --- > builtin/config.c | 25 ++----------------------- > config.c | 24 ++++++++++++++++++++++++ > config.h | 1 + > 3 files changed, 27 insertions(+), 23 deletions(-) > > diff --git a/builtin/config.c b/builtin/config.c > index 6fff2655816..df06b766fad 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -708,30 +708,9 @@ 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. > - */ > - die(_("$HOME not set")); > - > + given_config_source.file = git_global_config(); > 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 d2cdda96edd..2ff766c56ff 100644 > --- a/config.c > +++ b/config.c > @@ -2111,6 +2111,30 @@ 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) > + /* > + * It is unknown if HOME/.gitconfig exists, so > + * we do not know if we should write to XDG Nit: we don't know about the intent of the caller, so they may not want to write to the file but only read it. > + * location; error out even if XDG_CONFIG_HOME > + * is set and points at a sane location. > + */ > + die(_("$HOME not set")); Is it sensible to `die()` here in this new function that behaves more like a library function? I imagine it would be more sensible to indicate the error to the user and let them handle it accordingly. Patrick > + > + 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 9f04de8ee3e..5cf961b548d 100644 > --- a/config.h > +++ b/config.h > @@ -394,6 +394,7 @@ int config_error_nonbool(const char *); > #endif > > char *git_system_config(void); > +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); > -- > 2.42.0.2.g879ad04204 >
On Mon, Oct 23, 2023 at 11:58:01AM +0200, Patrick Steinhardt wrote: > On Wed, Oct 18, 2023 at 10:28:40PM +0200, 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”. > > > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > --- > > builtin/config.c | 25 ++----------------------- > > config.c | 24 ++++++++++++++++++++++++ > > config.h | 1 + > > 3 files changed, 27 insertions(+), 23 deletions(-) > > > > diff --git a/builtin/config.c b/builtin/config.c > > index 6fff2655816..df06b766fad 100644 > > --- a/builtin/config.c > > +++ b/builtin/config.c > > @@ -708,30 +708,9 @@ 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. > > - */ > > - die(_("$HOME not set")); > > - > > + given_config_source.file = git_global_config(); > > 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 d2cdda96edd..2ff766c56ff 100644 > > --- a/config.c > > +++ b/config.c > > @@ -2111,6 +2111,30 @@ 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) > > + /* > > + * It is unknown if HOME/.gitconfig exists, so > > + * we do not know if we should write to XDG > > Nit: we don't know about the intent of the caller, so they may not want > to write to the file but only read it. I was going to suggest that we allow the caller to pass in the flags that they wish for git_global_config() to pass down to access(2), but was surprised to see that we always use R_OK. But thinking on it for a moment longer, I realized that we don't care about write-level permissions for the config, since we want to instead open $GIT_DIR/config.lock for writing, and then rename() it into place, meaning we only care about whether or not we have write permissions on $GIT_DIR itself. I think in the existing location of this code, the "if we should write" portion of the comment is premature, since we don't know for sure whether or not we are writing. So I'd be fine with leaving it as-is, but changing the comment seems easy enough to do... > > + * location; error out even if XDG_CONFIG_HOME > > + * is set and points at a sane location. > > + */ > > + die(_("$HOME not set")); > > Is it sensible to `die()` here in this new function that behaves more > like a library function? I imagine it would be more sensible to indicate > the error to the user and let them handle it accordingly. Agreed. Thanks, Taylor
Hi Taylor and Patrick On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote: >> Nit: we don't know about the intent of the caller, so they may not want >> to write to the file but only read it. > > I was going to suggest that we allow the caller to pass in the flags > that they wish for git_global_config() to pass down to access(2), but > was surprised to see that we always use R_OK. > > But thinking on it for a moment longer, I realized that we don't care > about write-level permissions for the config, since we want to instead > open $GIT_DIR/config.lock for writing, and then rename() it into place, > meaning we only care about whether or not we have write permissions on > $GIT_DIR itself. > > I think in the existing location of this code, the "if we should write" > portion of the comment is premature, since we don't know for sure > whether or not we are writing. So I'd be fine with leaving it as-is, but > changing the comment seems easy enough to do... > >> > + * location; error out even if XDG_CONFIG_HOME >> > + * is set and points at a sane location. >> > + */ >> > + die(_("$HOME not set")); >> >> Is it sensible to `die()` here in this new function that behaves more >> like a library function? I imagine it would be more sensible to indicate >> the error to the user and let them handle it accordingly. > > Agreed. > > Thanks, > Taylor What do you guys think the signature of `git_global_config` should be?
On Tue, Oct 24, 2023 at 03:23:11PM +0200, Kristoffer Haugsbakk wrote: > Hi Taylor and Patrick > > On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote: > >> Nit: we don't know about the intent of the caller, so they may not want > >> to write to the file but only read it. > > > > I was going to suggest that we allow the caller to pass in the flags > > that they wish for git_global_config() to pass down to access(2), but > > was surprised to see that we always use R_OK. > > > > But thinking on it for a moment longer, I realized that we don't care > > about write-level permissions for the config, since we want to instead > > open $GIT_DIR/config.lock for writing, and then rename() it into place, > > meaning we only care about whether or not we have write permissions on > > $GIT_DIR itself. > > > > I think in the existing location of this code, the "if we should write" > > portion of the comment is premature, since we don't know for sure > > whether or not we are writing. So I'd be fine with leaving it as-is, but > > changing the comment seems easy enough to do... > > > >> > + * location; error out even if XDG_CONFIG_HOME > >> > + * is set and points at a sane location. > >> > + */ > >> > + die(_("$HOME not set")); > >> > >> Is it sensible to `die()` here in this new function that behaves more > >> like a library function? I imagine it would be more sensible to indicate > >> the error to the user and let them handle it accordingly. > > > > Agreed. > > > > Thanks, > > Taylor > > What do you guys think the signature of `git_global_config` should be? Either of the following: - `int git_global_config(char **out_pat)` - `char **git_global_config(void)` In the first case you'd signal error via a non-zero return value, whereas in the second case you would signal it via a `NULL` return value. To decide which one to go with I'd recommend to check whether there is any similar precedent in "config.h" and what style that precedent uses. Patrick
On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote: >> What do you guys think the signature of `git_global_config` should be? > > Either of the following: > > - `int git_global_config(char **out_pat)` > - `char **git_global_config(void)` > > In the first case you'd signal error via a non-zero return value, > whereas in the second case you would signal it via a `NULL` return > value. > > To decide which one to go with I'd recommend to check whether there is > any similar precedent in "config.h" and what style that precedent uses. Okay thanks. So no parameter for determining whether one is writing or just reading the file. Cheers
On Wed, Oct 25, 2023 at 09:33:23AM +0200, Kristoffer Haugsbakk wrote: > On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote: > >> What do you guys think the signature of `git_global_config` should be? > > > > Either of the following: > > > > - `int git_global_config(char **out_pat)` > > - `char **git_global_config(void)` > > > > In the first case you'd signal error via a non-zero return value, > > whereas in the second case you would signal it via a `NULL` return > > value. > > > > To decide which one to go with I'd recommend to check whether there is > > any similar precedent in "config.h" and what style that precedent uses. > > Okay thanks. So no parameter for determining whether one is writing or > just reading the file. This parameter would only exist for the purpose of the error message, right? If so, I think that'd be overkill. If we want to have differing errors depending on how the function is called the best way to handle that would likely be to generate the error message at the callsite instead of in the library itself. Patrick
Patrick Steinhardt <ps@pks.im> writes: > This parameter would only exist for the purpose of the error message, > right? If so, I think that'd be overkill. If we want to have differing > errors depending on how the function is called the best way to handle > that would likely be to generate the error message at the callsite > instead of in the library itself. We would need to make sure the lower-level helpers need to be able to tell what kind of failure they saw (in other words, why they are failing) to the callers, which may require a bit of designing the error return convention and plumbing through necessary pieces of information, but the longer term payoff would be great. I do not think this is such a case, but if the lower-level needs to fail differently (e.g., the thing not existing is acceptable when writing as we will create a new one, but is a fail-worthy error when reading), then the caller needs to give that down the callchain, though.
diff --git a/builtin/config.c b/builtin/config.c index 6fff2655816..df06b766fad 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -708,30 +708,9 @@ 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. - */ - die(_("$HOME not set")); - + given_config_source.file = git_global_config(); 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 d2cdda96edd..2ff766c56ff 100644 --- a/config.c +++ b/config.c @@ -2111,6 +2111,30 @@ 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) + /* + * 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. + */ + die(_("$HOME not set")); + + 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 9f04de8ee3e..5cf961b548d 100644 --- a/config.h +++ b/config.h @@ -394,6 +394,7 @@ int config_error_nonbool(const char *); #endif char *git_system_config(void); +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> --- builtin/config.c | 25 ++----------------------- config.c | 24 ++++++++++++++++++++++++ config.h | 1 + 3 files changed, 27 insertions(+), 23 deletions(-)