maintenance: use XDG config if it exists
cover maintenance: use XDG config if it exists
maintenance: use XDG config if it exists
I use the conventional XDG config path for the global configuration. This path is always used except for git maintenance register.
§ Other discussions
While working on this series I found that Phillip Wood[1] had pointed out that xdg_config is never used. However that was on a series where this was the existing behavior (not new), so this wasn't acted upon.
🔗 1: https://lore.kernel.org/git/[email protected]/
§ Patches
• 1–3: Preparatory
• 4: The desired change
§ CC
• Patrick Steinhardt: config changes
• Derrick Stolee: maintenance changes
Kristoffer Haugsbakk (4):
config: format newlines
config: rename global config function
config: factor out global config file retrieval
maintenance: use XDG config if it exists
builtin/config.c | 26 ++------------------------ builtin/gc.c | 23 +++++------------------ builtin/var.c | 2 +- config.c | 30 ++++++++++++++++++++++++++---- config.h | 3 ++- t/t7900-maintenance.sh | 21 +++++++++++++++++++++ 6 files changed, 57 insertions(+), 48 deletions(-)
--
2.42.0.2.g879ad04204
PATCH 1/4 config: format newlines
Remove unneeded newlines according to clang-format.
---
Notes (series):
Honestly the formatter changing these lines over and over again was just annoying. And we're visiting the file anyway.
builtin/config.c | 1 - config.c | 2 -- 2 files changed, 3 deletions(-)
builtin/config.c +0 -1
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix) given_config_source.scope =CONFIG_SCOPE_COMMAND ; } -if (respect_includes_opt ==-1 ) config_options.respect_includes = !given_config_source.file;else
config.c +0 -2
@@ -96,7 +96,6 @@ static long config_file_ftell(struct config_source *conf)return ftell (conf->u.file); } -static int config_buf_fgetc (struct config_source *conf) {if (conf->u.buf.pos < conf->u.buf.len)
@@ -3564,7 +3563,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, write_err_out: ret =write_error (get_lock_file_path (&lock)); goto out_free; - } void git_config_set_multivar_in_file(const char *config_filename,
Signature · 1 line
2.42.0.2.g879ad04204
PATCH 2/4 config: rename global config function
In reply to Kristoffer HaugsbakkRename this function to a more descriptive name since we want to use the existing name for a new function.
--- builtin/config.c | 2 +- builtin/gc.c | 4 ++-- builtin/var.c | 2 +- config.c | 4 ++-- config.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
builtin/config.c +1 -1
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)if (use_global_config) {char *user_config, *xdg_config; -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config);if (!user_config)/* * It is unknown if HOME/.gitconfig exists, so
builtin/gc.c +2 -2
@@ -1529,7 +1529,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)char *user_config =NULL , *xdg_config =NULL ;if (!config_file) { -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config); config_file = user_config;if (!user_config)die (_ ("$HOME not set" ));
@@ -1597,7 +1597,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiint rc;char *user_config =NULL , *xdg_config =NULL ;if (!config_file) { -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config); config_file = user_config;if (!user_config)die (_ ("$HOME not set" ));
builtin/var.c +1 -1
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)char *user, *xdg;size_t unused; -git_global_config (&user, &xdg); +git_global_config_paths (&user, &xdg);if (xdg && *xdg) {normalize_path_copy (xdg, xdg);strbuf_addf (&buf,"%s\n" , xdg);
config.c +2 -2
@@ -2111,7 +2111,7 @@ char *git_system_config(void)return system_config; } -void git_global_config (char **user_out,char **xdg_out) +void git_global_config_paths (char **user_out,char **xdg_out) {char *user_config =xstrdup_or_null (getenv ("GIT_CONFIG_GLOBAL" ));char *xdg_config =NULL ;
@@ -2186,7 +2186,7 @@ static int do_git_config_sequence(const struct config_options *opts, data,CONFIG_SCOPE_SYSTEM , NULL); -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config);if (xdg_config && !access_or_die (xdg_config,R_OK ,ACCESS_EACCES_OK )) ret += git_config_from_file_with_options(fn, xdg_config, data,
config.h +1 -1
@@ -394,7 +394,7 @@ int config_error_nonbool(const char *);#endif char *git_system_config (void ); -void git_global_config (char **user,char **xdg); +void git_global_config_paths (char **user,char **xdg);int git_config_parse_parameter (const char *,config_fn_t fn,void *data);
Signature · 1 line
2.42.0.2.g879ad04204
PATCH 3/4 config: factor out global config file retrieval
In reply to Kristoffer HaugsbakkFactor 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”.
--- builtin/config.c | 25 ++----------------------- config.c | 24 ++++++++++++++++++++++++ config.h | 1 + 3 files changed, 27 insertions(+), 23 deletions(-)
builtin/config.c +2 -23
@@ -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) {
config.c +24 -0
@@ -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" ));
config.h +1 -0
@@ -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);
Signature · 1 line
2.42.0.2.g879ad04204
PATCH 4/4 maintenance: use XDG config if it exists
In reply to Kristoffer Haugsbakkgit maintenance register registers the repository in the user's global config. $XDG_CONFIG_HOME/git/config is supposed to be used if ~/.gitconfig does not exist. However, this command creates a ~/.gitconfig file and writes to that one even though the XDG variant exists.
This used to work correctly until 50a044f1e4 (gc: replace config subprocesses with API calls, 2022-09-27), when the command started calling the config API instead of git-config(1).
Also change unregister accordingly.
--- builtin/gc.c | 23 +++++------------------ t/t7900-maintenance.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 18 deletions(-)
builtin/gc.c +5 -18
@@ -1526,19 +1526,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)if (!found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; -if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config; -if (!user_config) -die (_ ("$HOME not set" )); - } +if (!config_file) + config_file =git_global_config (); rc =git_config_set_multivar_in_file_gently ( config_file,"maintenance.repo" , maintpath,CONFIG_REGEX_NONE ,0 ); -free (user_config); -free (xdg_config);if (rc) die(_("unable to add '%s' value of '%s'"),
@@ -1595,18 +1588,12 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiif (found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; -if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config; -if (!user_config) -die (_ ("$HOME not set" )); - } + +if (!config_file) + config_file =git_global_config (); rc =git_config_set_multivar_in_file_gently ( config_file, key,NULL , maintpath,CONFIG_FLAGS_MULTI_REPLACE |CONFIG_FLAGS_FIXED_VALUE ); -free (user_config); -free (xdg_config);if (rc && (!force || rc ==CONFIG_NOTHING_SET ))
t/t7900-maintenance.sh +21 -0
@@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' ' test_subcommand ! git maintenance run --auto --quiet <false ' +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' + XDG_CONFIG_HOME=.config && + test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config && + export "XDG_CONFIG_HOME" && + mkdir -p "$XDG_CONFIG_HOME"/git && + touch "$XDG_CONFIG_HOME"/git/config && + git maintenance register && + git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual && + pwd >expect && + test_cmp expect actual +' + +test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' ' + test_when_finished git maintenance unregister && + test_path_is_missing "$XDG_CONFIG_HOME"/git/config && + git maintenance register && + git config --global --get maintenance.repo >actual && + pwd >expect && + test_cmp expect actual +' + test_expect_success 'maintenance.<task>.enabled' ' git config maintenance.gc.enabled false && git config maintenance.commit-graph.enabled true &&
Signature · 1 line
2.42.0.2.g879ad04204
Re: [PATCH v1 3/4] config: factor out global config file retrieval
In reply to Kristoffer HaugsbakkQuoted 58 lines · Kristoffer Haugsbakk
Factor out code that retrieves the global config file so that we can use it in
gc.cas well.Use the old name from the previous commit since this function acts functionally the same as
git_system_configbut for “global”.Signed-off-by Kristoffer Haugsbakk <[email protected]>--- builtin/config.c | 25 ++----------------------- config.c | 24 ++++++++++++++++++++++++ config.h | 1 + 3 files changed, 27 insertions(+), 23 deletions(-)builtin/config.c +2
-23@@ -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) {config.c +0
-0@@ -2111,6 +2111,30 @@ char *git_system_config(void)return system_config; }
config.c char *git_system_config(void) · line 2122
Quoted 10 lines
config.c +9
-0+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.
config.c char *git_system_config(void) · line 2126
Quoted 4 lines
config.c +4
-0+* 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
Quoted 29 lines
config.c +11
-0+ +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" ));config.h +1
-0@@ -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);Signature · 1 line
2.42.0.2.g879ad04204
Quoted 32 lines · Kristoffer Haugsbakk
git maintenance registerregisters the repository in the user's global config.$XDG_CONFIG_HOME/git/configis supposed to be used if~/.gitconfigdoes not exist. However, this command creates a~/.gitconfigfile and writes to that one even though the XDG variant exists.This used to work correctly until
50a044f1e4(gc: replace config subprocesses with API calls, 2022-09-27), when the command started calling the config API instead of git-config(1).Also change
unregisteraccordingly.Signed-off-by Kristoffer Haugsbakk <[email protected]>--- builtin/gc.c | 23 +++++------------------ t/t7900-maintenance.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 18 deletions(-)builtin/gc.c +0
-4@@ -1526,19 +1526,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)if (!found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; -if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config;
builtin/gc.c static int maintenance_register(int argc, const char **argv, const char *prefix) · line 1541
Quoted 10 lines
builtin/gc.c +2
-5-if (!user_config) -die (_ ("$HOME not set" )); - } +if (!config_file) + config_file =git_global_config (); rc =git_config_set_multivar_in_file_gently ( config_file,"maintenance.repo" , maintpath,CONFIG_REGEX_NONE ,0 ); -free (user_config); -free (xdg_config);
Don't we have to free config_file now?
Quoted 11 lines
builtin/gc.c +0
-5if (rc) die(_("unable to add '%s' value of '%s'"),@@ -1595,18 +1588,12 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiif (found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; -if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config; -if (!user_config)
builtin/gc.c static int maintenance_unregister(int argc, const char **argv, const char *prefi · line 1609
Quoted 10 lines
builtin/gc.c +3
-4-die (_ ("$HOME not set" )); - } + +if (!config_file) + config_file =git_global_config (); rc =git_config_set_multivar_in_file_gently ( config_file, key,NULL , maintpath,CONFIG_FLAGS_MULTI_REPLACE |CONFIG_FLAGS_FIXED_VALUE ); -free (user_config); -free (xdg_config);
Same here.
Quoted 7 lines
builtin/gc.c +0
-0if (rc && (!force || rc ==CONFIG_NOTHING_SET ))t/t7900-maintenance.sh +0
-0
t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 73
Quoted 7 lines
t/t7900-maintenance.sh +4
-0@@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' ' test_subcommand ! git maintenance run --auto --quiet <false ' +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' + XDG_CONFIG_HOME=.config && + test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config && + export "XDG_CONFIG_HOME" &&
Style: there is no need to quote here.
Also, I think we need to unset this variable at the end of this test as tests don't run in a subshell. In theory, we should also be able to leave this variable unset completely as it will be derived from HOME automatically anyway.
Quoted 7 lines
t/t7900-maintenance.sh +7
-0+ mkdir -p "$XDG_CONFIG_HOME"/git && + touch "$XDG_CONFIG_HOME"/git/config && + git maintenance register && + git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual && + pwd >expect && + test_cmp expect actual +'
t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 90
Quoted 10 lines
t/t7900-maintenance.sh +10
-0+ +test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' ' + test_when_finished git maintenance unregister && + test_path_is_missing "$XDG_CONFIG_HOME"/git/config && + git maintenance register && + git config --global --get maintenance.repo >actual && + pwd >expect && + test_cmp expect actual +' +
As you also change the code of git maintenance unregister, should we test its behaviour, as well?
Patrick
Quoted 6 lines
t/t7900-maintenance.sh +0
-0test_expect_success 'maintenance.<task>.enabled' ' git config maintenance.gc.enabled false && git config maintenance.commit-graph.enabled true &&Signature · 1 line
2.42.0.2.g879ad04204
Quoted 20 lines · Patrick Steinhardt
Quoted 14 lines · Kristoffer Haugsbakk
git maintenance registerregisters the repository in the user's global config.$XDG_CONFIG_HOME/git/configis supposed to be used if~/.gitconfigdoes not exist. However, this command creates a~/.gitconfigfile and writes to that one even though the XDG variant exists.This used to work correctly until
50a044f1e4(gc: replace config subprocesses with API calls, 2022-09-27), when the command started calling the config API instead of git-config(1).Also change
unregisteraccordingly.Signed-off-by Kristoffer Haugsbakk <[email protected]>---
t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 73
Quoted 4 lines
t/t7900-maintenance.sh +4
-0+test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' ' + XDG_CONFIG_HOME=.config && + test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config && + export "XDG_CONFIG_HOME" &&
Quoted 2 lines
Also, I think we need to unset this variable at the end of this test as tests don't run in a subshell. [...]
Yup, well spotted. Almost the entire body of this test should be in a subshell to ensure that the environment variable does not live beyond the end of this test. But test_when_finished() can't be used in a subshell, so a little care is needed:
test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
test_when_finished rm -r .config/git/config &&
(
XDG_CONFIG_HOME=.config &&
...
)
'Quoted 2 lines
t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 75
Quoted 2 lines
t/t7900-maintenance.sh +2
-0+ mkdir -p "$XDG_CONFIG_HOME"/git && + touch "$XDG_CONFIG_HOME"/git/config &&
If the timestamp of the file is not significant, then we use > to create it rather than touch:
Quoted 7 lines
"$XDG_CONFIG_HOME"/git/config &&
Quoted 5 lines
t/t7900-maintenance.sh +5
-0+ git maintenance register && + git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual && + pwd >expect && + test_cmp expect actual +'
Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
In reply to Patrick SteinhardtQuoted 60 lines · Patrick Steinhardt
Quoted 59 lines · Kristoffer Haugsbakk
Factor out code that retrieves the global config file so that we can use it in
gc.cas well.Use the old name from the previous commit since this function acts functionally the same as
git_system_configbut for “global”.Signed-off-by Kristoffer Haugsbakk <[email protected]>--- builtin/config.c | 25 ++----------------------- config.c | 24 ++++++++++++++++++++++++ config.h | 1 + 3 files changed, 27 insertions(+), 23 deletions(-)builtin/config.c +2
-23@@ -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) {config.c +0
-0@@ -2111,6 +2111,30 @@ char *git_system_config(void)return system_config; }
Quoted 12 lines
config.c char *git_system_config(void) · line 2122
Quoted 9 lines
config.c +9
-0+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...
Quoted 8 lines
config.c char *git_system_config(void) · line 2126
Quoted 4 lines
config.c +4
-0+* 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
Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
Hi Taylor and Patrick
Quoted 27 lines · Taylor Blau
Quoted 2 lines
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 useR_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...
Quoted 8 lines
config.c char *git_system_config(void) · line 2126
Quoted 4 lines
config.c +4
-0+* 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.
Quoted 4 lines
Agreed.
Thanks,
Taylor
What do you guys think the signature of git_global_config should be?
Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
Quoted 30 lines · Kristoffer Haugsbakk
Hi Taylor and Patrick
Quoted 27 lines · Taylor Blau
Quoted 2 lines
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 useR_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...
Quoted 8 lines
config.c char *git_system_config(void) · line 2126
Quoted 4 lines
config.c +4
-0+* 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.
Quoted 6 lines
Quoted 4 lines
Agreed.
Thanks,
TaylorWhat do you guys think the signature of
git_global_configshould 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
Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
Quoted 7 lines · Patrick Steinhardt
Quoted 1 line
What do you guys think the signature of
git_global_configshould be?Either of the following:
-
int git_global_config(char **out_pat)
-char **git_global_config(void)
Quoted 6 lines
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
NULLreturn 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
Signature · 1 line
Kristoffer Haugsbakk
Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
Quoted 12 lines · Kristoffer Haugsbakk
Quoted 11 lines · Patrick Steinhardt
Quoted 1 line
What do you guys think the signature of
git_global_configshould 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
NULLreturn value.
Quoted 5 lines
Quoted 2 lines
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
Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
Quoted 5 lines · Patrick Steinhardt
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.
v2 cover maintenance: use XDG config if it exists
In reply to Kristoffer HaugsbakkI use the conventional XDG config path for the global configuration. This path is always used except for git maintenance register and unregister.
§ Changes since v1
• Free config_file
• https://lore.kernel.org/git/ZTZDsIcrB0zwHlFR@tanuki/
• Return NULL instead of dying
• https://lore.kernel.org/git/ZTZDqToqcsDiS5AP@tanuki/
• Tests
• Test unregister
• Use subshells
• Style
§ Patches
• 1–3: Preparatory
• 4: The desired change
§ CC
• Patrick Steinhardt: config changes; v1 feedback
• Derrick Stolee: maintenance changes
• Eric Sunshine: v1 feedback
• Taylor Blau: v1 feedback
Kristoffer Haugsbakk (4):
config: format newlines
config: rename global config function
config: factor out global config file retrieval
maintenance: use XDG config if it exists
builtin/config.c | 26 +++--------------------- builtin/gc.c | 27 ++++++++++++------------- builtin/var.c | 2 +- config.c | 26 ++++++++++++++++++++---- config.h | 6 +++++- t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 89 insertions(+), 43 deletions(-)
Range-diff against v1:
1: 39934cb7e50 = 1: d5f6c8d62ec config: format newlines
2: 48a5357f97c = 2: cbc5fde0094 config: rename global config function
3: 147c767443c ! 3: 32e5ec7d866 config: factor out global config file retrieval
@@ Commit message
Signed-off-by: Kristoffer Haugsbakk <[email protected]>
+
+ ## Notes (series) ##
+ v2:
+ • Don’t die; return `NULL`
+
## builtin/config.c ##
@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
}
@@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
- * 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();
++ 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) &&
@@ config.c: char *git_system_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 (!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)) {
@@ config.h: 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);4: 1e2376a4b99 < -: ----------- maintenance: use XDG config if it exists -: ----------- > 4: 8bd67c5bf01 maintenance: use XDG config if it exists
--
2.43.0
PATCH v2 1/4 config: format newlines
Remove unneeded newlines according to clang-format.
---
Notes (series):
Honestly the formatter changing these lines over and over again was just annoying. And we're visiting the file anyway.
builtin/config.c | 1 - config.c | 2 -- 2 files changed, 3 deletions(-)
builtin/config.c +0 -1
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix) given_config_source.scope =CONFIG_SCOPE_COMMAND ; } -if (respect_includes_opt ==-1 ) config_options.respect_includes = !given_config_source.file;else
config.c +0 -2
@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)return ftell (conf->u.file); } -static int config_buf_fgetc (struct config_source *conf) {if (conf->u.buf.pos < conf->u.buf.len)
@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, write_err_out: ret =write_error (get_lock_file_path (&lock)); goto out_free; - } void git_config_set_multivar_in_file(const char *config_filename,
Signature · 1 line
2.43.0
PATCH v2 2/4 config: rename global config function
In reply to Kristoffer HaugsbakkRename this function to a more descriptive name since we want to use the existing name for a new function.
--- builtin/config.c | 2 +- builtin/gc.c | 4 ++-- builtin/var.c | 2 +- config.c | 4 ++-- config.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
builtin/config.c +1 -1
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)if (use_global_config) {char *user_config, *xdg_config; -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config);if (!user_config)/* * It is unknown if HOME/.gitconfig exists, so
builtin/gc.c +2 -2
@@ -1546,7 +1546,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)char *user_config =NULL , *xdg_config =NULL ;if (!config_file) { -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config); config_file = user_config;if (!user_config)die (_ ("$HOME not set" ));
@@ -1614,7 +1614,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiint rc;char *user_config =NULL , *xdg_config =NULL ;if (!config_file) { -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config); config_file = user_config;if (!user_config)die (_ ("$HOME not set" ));
builtin/var.c +1 -1
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)char *user, *xdg;size_t unused; -git_global_config (&user, &xdg); +git_global_config_paths (&user, &xdg);if (xdg && *xdg) {normalize_path_copy (xdg, xdg);strbuf_addf (&buf,"%s\n" , xdg);
config.c +2 -2
@@ -1987,7 +1987,7 @@ char *git_system_config(void)return system_config; } -void git_global_config (char **user_out,char **xdg_out) +void git_global_config_paths (char **user_out,char **xdg_out) {char *user_config =xstrdup_or_null (getenv ("GIT_CONFIG_GLOBAL" ));char *xdg_config =NULL ;
@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts, data,CONFIG_SCOPE_SYSTEM , NULL); -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config);if (xdg_config && !access_or_die (xdg_config,R_OK ,ACCESS_EACCES_OK )) ret += git_config_from_file_with_options(fn, xdg_config, data,
config.h +1 -1
@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);#endif char *git_system_config (void ); -void git_global_config (char **user,char **xdg); +void git_global_config_paths (char **user,char **xdg);int git_config_parse_parameter (const char *,config_fn_t fn,void *data);
Signature · 1 line
2.43.0
PATCH v2 3/4 config: factor out global config file retrieval
In reply to Kristoffer HaugsbakkFactor 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”.
---
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(-)
builtin/config.c +3 -22
@@ -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) {
config.c +20 -0
@@ -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" ));
config.h +4 -0
@@ -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);
Signature · 1 line
2.43.0
PATCH v2 4/4 maintenance: use XDG config if it exists
In reply to Kristoffer Haugsbakkgit maintenance register registers the repository in the user's global config. $XDG_CONFIG_HOME/git/config is supposed to be used if ~/.gitconfig does not exist. However, this command creates a ~/.gitconfig file and writes to that one even though the XDG variant exists.
This used to work correctly until 50a044f1e4 (gc: replace config subprocesses with API calls, 2022-09-27), when the command started calling the config API instead of git-config(1).
Also change unregister accordingly.
---
Notes (series):
v2:
• Add unregister tests
• Use subshell when exporting an env. variable
• Style in tests
• Free variables properly
builtin/gc.c | 27 ++++++++++++------------- t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 14 deletions(-)
builtin/gc.c +13 -14
@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)if (!found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; +char *global_config_file =NULL ;if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config; -if (!user_config) -die (_ ("$HOME not set" )); + global_config_file =git_global_config (); + config_file = global_config_file; } +if (!config_file) +die (_ ("$HOME not set" )); rc =git_config_set_multivar_in_file_gently ( config_file,"maintenance.repo" , maintpath,CONFIG_REGEX_NONE ,0 ); -free (user_config); -free (xdg_config); +free (global_config_file);if (rc) die(_("unable to add '%s' value of '%s'"),
@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiif (found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; +char *global_config_file =NULL ; +if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config; -if (!user_config) -die (_ ("$HOME not set" )); + global_config_file =git_global_config (); + config_file = global_config_file; } +if (!config_file) +die (_ ("$HOME not set" )); rc =git_config_set_multivar_in_file_gently ( config_file, key,NULL , maintpath,CONFIG_FLAGS_MULTI_REPLACE |CONFIG_FLAGS_FIXED_VALUE ); -free (user_config); -free (xdg_config); +free (global_config_file);if (rc && (!force || rc ==CONFIG_NOTHING_SET ))
t/t7900-maintenance.sh +45 -0
51 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v2 3/4] config: factor out global config file retrieval
In reply to Kristoffer HaugsbakkQuoted 14 lines · Kristoffer Haugsbakk
builtin/config.c +1
-2if (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) {config.c +0
-0@@ -1987,6 +1987,26 @@ char *git_system_config(void)return system_config;
config.c char *git_system_config(void) · line 1997
Quoted 10 lines
config.c +5
-0} +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
config.h int config_error_nonbool(const char *); · line 387
Quoted 11 lines
config.h +3
-0@@ -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.
Quoted 1 line
config.h +1
-0+char *git_global_config (void );
Re: [PATCH v2 3/4] config: factor out global config file retrieval
Quoted 8 lines · Junio C Hamano
Quoted 4 lines · Kristoffer Haugsbakk
char
*git_system_config(void);
+/**
+ * ReturnsNULLif is uncertain whether or notHOME/.gitconfigexists.
+ */Sorry, but I am not sure what this comment wants to say.
Quoted 7 lines
When $HOME is not set, we do get NULL out of this function. But
interpolate_path()that makesgit_global_config_paths()to return NULL inuser_configdoes not do any existence check withstat()oraccess(), 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
Signature · 1 line
Kristoffer Haugsbakk
Quoted 14 lines · Kristoffer Haugsbakk
builtin/gc.c +1
-3@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)if (!found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; +char *global_config_file =NULL ;if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config;
builtin/gc.c static int maintenance_register(int argc, const char **argv, const char *prefix) · line 1556
Quoted 10 lines
builtin/gc.c +4
-2-if (!user_config) -die (_ ("$HOME not set" )); + global_config_file =git_global_config (); + config_file = global_config_file; } +if (!config_file) +die (_ ("$HOME not set" )); rc =git_config_set_multivar_in_file_gently ( config_file,"maintenance.repo" , maintpath,CONFIG_REGEX_NONE ,0 );
OK. We used to ask for both user and xdg and without using xdg at all, as long as $HOME is set, we used $HOME/.gitconfig even if it did not exist.
What we want to happen is we pick XDG is XDG exists *and* $HOME/.gitconfig does not. And that is exactly what git_global_config() gives us.
Nicely done.
Quoted 10 lines
builtin/gc.c +2
-3@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiif (found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; +char *global_config_file =NULL ; +if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config;
builtin/gc.c static int maintenance_unregister(int argc, const char **argv, const char *prefi · line 1624
Quoted 10 lines
builtin/gc.c +4
-2-if (!user_config) -die (_ ("$HOME not set" )); + global_config_file =git_global_config (); + config_file = global_config_file; } +if (!config_file) +die (_ ("$HOME not set" )); rc =git_config_set_multivar_in_file_gently ( config_file, key,NULL , maintpath,CONFIG_FLAGS_MULTI_REPLACE |CONFIG_FLAGS_FIXED_VALUE );
Ditto.
v3 cover maintenance: use XDG config if it exists
In reply to Kristoffer HaugsbakkI use the conventional XDG config path for the global configuration. This path is always used except for git maintenance register and unregister.
§ Changes since v2 (by patch)
• config: factor out global config file retrieval
• Remove doc on git_global_config
• https://lore.kernel.org/git/[email protected]/
§ Patches
• 1–3: Preparatory
• 4: The desired change
§ CC
• Patrick Steinhardt: config changes; v1 feedback
• Derrick Stolee: maintenance changes
• Eric Sunshine: v1 feedback
• Taylor Blau: v1 feedback
• Junio C Hamano: v2 feedback
§ CI
https://github.com/LemmingAvalanche/git/actions/runs/7521230119
Kristoffer Haugsbakk (4):
config: format newlines
config: rename global config function
config: factor out global config file retrieval
maintenance: use XDG config if it exists
builtin/config.c | 26 +++--------------------- builtin/gc.c | 27 ++++++++++++------------- builtin/var.c | 2 +- config.c | 26 ++++++++++++++++++++---- config.h | 3 ++- t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 86 insertions(+), 43 deletions(-)
Range-diff against v2:
1: d5f6c8d62ec = 1: 1c92b772ef4 config: format newlines
2: cbc5fde0094 = 2: 269490794bc config: rename global config function
3: 32e5ec7d866 ! 3: 0643a85892c config: factor out global config file retrieval
@@ Commit message
## Notes (series) ##
+ v3:
+ • Remove doc on `git_global_config`
+ • https://lore.kernel.org/git/[email protected]/
v2:
• Don’t die; return `NULL`
@@ config.h: 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);4: 8bd67c5bf01 = 4: e0880af0a31 maintenance: use XDG config if it exists --
2.43.0
PATCH v3 1/4 config: format newlines
Remove unneeded newlines according to clang-format.
---
Notes (series):
Honestly the formatter changing these lines over and over again was just annoying. And we're visiting the file anyway.
builtin/config.c | 1 - config.c | 2 -- 2 files changed, 3 deletions(-)
builtin/config.c +0 -1
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix) given_config_source.scope =CONFIG_SCOPE_COMMAND ; } -if (respect_includes_opt ==-1 ) config_options.respect_includes = !given_config_source.file;else
config.c +0 -2
@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)return ftell (conf->u.file); } -static int config_buf_fgetc (struct config_source *conf) {if (conf->u.buf.pos < conf->u.buf.len)
@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, write_err_out: ret =write_error (get_lock_file_path (&lock)); goto out_free; - } void git_config_set_multivar_in_file(const char *config_filename,
Signature · 1 line
2.43.0
PATCH v3 2/4 config: rename global config function
In reply to Kristoffer HaugsbakkRename this function to a more descriptive name since we want to use the existing name for a new function.
--- builtin/config.c | 2 +- builtin/gc.c | 4 ++-- builtin/var.c | 2 +- config.c | 4 ++-- config.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
builtin/config.c +1 -1
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)if (use_global_config) {char *user_config, *xdg_config; -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config);if (!user_config)/* * It is unknown if HOME/.gitconfig exists, so
builtin/gc.c +2 -2
@@ -1546,7 +1546,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)char *user_config =NULL , *xdg_config =NULL ;if (!config_file) { -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config); config_file = user_config;if (!user_config)die (_ ("$HOME not set" ));
@@ -1614,7 +1614,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiint rc;char *user_config =NULL , *xdg_config =NULL ;if (!config_file) { -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config); config_file = user_config;if (!user_config)die (_ ("$HOME not set" ));
builtin/var.c +1 -1
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)char *user, *xdg;size_t unused; -git_global_config (&user, &xdg); +git_global_config_paths (&user, &xdg);if (xdg && *xdg) {normalize_path_copy (xdg, xdg);strbuf_addf (&buf,"%s\n" , xdg);
config.c +2 -2
@@ -1987,7 +1987,7 @@ char *git_system_config(void)return system_config; } -void git_global_config (char **user_out,char **xdg_out) +void git_global_config_paths (char **user_out,char **xdg_out) {char *user_config =xstrdup_or_null (getenv ("GIT_CONFIG_GLOBAL" ));char *xdg_config =NULL ;
@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts, data,CONFIG_SCOPE_SYSTEM , NULL); -git_global_config (&user_config, &xdg_config); +git_global_config_paths (&user_config, &xdg_config);if (xdg_config && !access_or_die (xdg_config,R_OK ,ACCESS_EACCES_OK )) ret += git_config_from_file_with_options(fn, xdg_config, data,
config.h +1 -1
@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);#endif char *git_system_config (void ); -void git_global_config (char **user,char **xdg); +void git_global_config_paths (char **user,char **xdg);int git_config_parse_parameter (const char *,config_fn_t fn,void *data);
Signature · 1 line
2.43.0
PATCH v3 3/4 config: factor out global config file retrieval
In reply to Kristoffer HaugsbakkFactor 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”.
---
Notes (series):
v3:
• Remove doc on git_global_config
• https://lore.kernel.org/git/[email protected]/
v2:
• Don’t die; return NULL
builtin/config.c | 25 +++---------------------- config.c | 20 ++++++++++++++++++++ config.h | 1 + 3 files changed, 24 insertions(+), 22 deletions(-)
builtin/config.c +3 -22
@@ -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) {
config.c +20 -0
@@ -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" ));
config.h +1 -0
@@ -382,6 +382,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);
Signature · 1 line
2.43.0
PATCH v3 4/4 maintenance: use XDG config if it exists
In reply to Kristoffer Haugsbakkgit maintenance register registers the repository in the user's global config. $XDG_CONFIG_HOME/git/config is supposed to be used if ~/.gitconfig does not exist. However, this command creates a ~/.gitconfig file and writes to that one even though the XDG variant exists.
This used to work correctly until 50a044f1e4 (gc: replace config subprocesses with API calls, 2022-09-27), when the command started calling the config API instead of git-config(1).
Also change unregister accordingly.
---
Notes (series):
v2:
• Add unregister tests
• Use subshell when exporting an env. variable
• Style in tests
• Free variables properly
builtin/gc.c | 27 ++++++++++++------------- t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 14 deletions(-)
builtin/gc.c +13 -14
@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)if (!found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; +char *global_config_file =NULL ;if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config; -if (!user_config) -die (_ ("$HOME not set" )); + global_config_file =git_global_config (); + config_file = global_config_file; } +if (!config_file) +die (_ ("$HOME not set" )); rc =git_config_set_multivar_in_file_gently ( config_file,"maintenance.repo" , maintpath,CONFIG_REGEX_NONE ,0 ); -free (user_config); -free (xdg_config); +free (global_config_file);if (rc) die(_("unable to add '%s' value of '%s'"),
@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefiif (found) {int rc; -char *user_config =NULL , *xdg_config =NULL ; +char *global_config_file =NULL ; +if (!config_file) { -git_global_config_paths (&user_config, &xdg_config); - config_file = user_config; -if (!user_config) -die (_ ("$HOME not set" )); + global_config_file =git_global_config (); + config_file = global_config_file; } +if (!config_file) +die (_ ("$HOME not set" )); rc =git_config_set_multivar_in_file_gently ( config_file, key,NULL , maintpath,CONFIG_FLAGS_MULTI_REPLACE |CONFIG_FLAGS_FIXED_VALUE ); -free (user_config); -free (xdg_config); +free (global_config_file);if (rc && (!force || rc ==CONFIG_NOTHING_SET ))
t/t7900-maintenance.sh +45 -0
51 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v2 3/4] config: factor out global config file retrieval
In reply to Kristoffer HaugsbakkQuoted 5 lines · Kristoffer Haugsbakk
Factor out code that retrieves the global config file so that we can use it in
gc.cas well.Use the old name from the previous commit since this function acts functionally the same as
git_system_configbut 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
Re: [PATCH v2 3/4] config: factor out global config file retrieval
Quoted 6 lines · Patrick Steinhardt
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 newgit_config_global_paths(). I'd propose to name the new function something likegit_config_global_preferred_path()orgit_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?
Signature · 1 line
Kristoffer Haugsbakk
Re: [PATCH v2 3/4] config: factor out global config file retrieval
Quoted 17 lines · Kristoffer Haugsbakk
Quoted 6 lines · Patrick Steinhardt
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 newgit_config_global_paths(). I'd propose to name the new function something likegit_config_global_preferred_path()orgit_config_global_path().The choice of
git_config_globalis mostly motivated by it working the same way asgit_config_system:``` given_config_source.file = git_system_config(); […] given_config_source.file = git_global_config(); ```
Quoted 6 lines
(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_configbe 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
Re: [PATCH v2 3/4] config: factor out global config file retrieval
In reply to Kristoffer HaugsbakkQuoted 8 lines · Kristoffer Haugsbakk
Quoted 6 lines · Patrick Steinhardt
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 newgit_config_global_paths(). I'd propose to name the new function something likegit_config_global_preferred_path()orgit_config_global_path().
Quoted 8 lines
The choice of
git_config_globalis mostly motivated by it working the same way asgit_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.
Quoted 2 lines
Your suggestion makes sense. But should
git_system_configbe 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.
RE: [PATCH v2 3/4] config: factor out global config file retrieval
Quoted 27 lines · Junio C Hamano
Quoted 8 lines · Kristoffer Haugsbakk
Quoted 6 lines · Patrick Steinhardt
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 newgit_config_global_paths(). I'd propose to name the new function something likegit_config_global_preferred_path()orgit_config_global_path().Quoted 6 lines
The choice of
git_config_globalis mostly motivated by it working the same way asgit_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")andrepo_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.
Quoted 6 lines
Quoted 2 lines
Your suggestion makes sense. But should
git_system_configbe 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
Re: [PATCH v2 3/4] config: factor out global config file retrieval
In reply to Patrick SteinhardtQuoted 5 lines · Patrick Steinhardt
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 bothgit_config_system()andgit_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.