{"thread":{"id":"60395","subject":"[PATCH v1 0/4] maintenance: use XDG config if it exists","startedAt":"2023-10-18T20:29:44Z","lastAt":"2024-01-19T23:04:13Z","messageCount":33,"participants":["Kristoffer Haugsbakk","Patrick Steinhardt","Eric Sunshine","Taylor Blau","Junio C Hamano","rsbecker@nexbridge.com"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"483450","messageId":"cover.1697660181.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":null,"subject":"[PATCH v1 0/4] maintenance: use XDG config if it exists","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-18T20:28:37Z","receivedAt":"2023-10-18T20:29:44Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"maintenance: use XDG config if it exists\n\nI use the conventional XDG config path for the global configuration. This\npath is always used except for `git maintenance register`.\n\n§ Other discussions\n\nWhile working on this series I found that Phillip Wood[1] had pointed out\nthat `xdg_config` is never used. However that was on a series where this\nwas the existing behavior (not new), so this wasn't acted upon.\n\n🔗 1: https://lore.kernel.org/git/448cc6ed-c441-85a3-2780-0c07e56f53f8@dunelm.org.uk/\n\n§ Patches\n\n• 1–3: Preparatory\n• 4: The desired change\n\n§ CC\n\n• Patrick Steinhardt: `config` changes\n• Derrick Stolee: `maintenance` changes\n\nKristoffer Haugsbakk (4):\n config: format newlines\n config: rename global config function\n config: factor out global config file retrieval\n maintenance: use XDG config if it exists\n\n builtin/config.c | 26 ++------------------------\n builtin/gc.c | 23 +++++------------------\n builtin/var.c | 2 +-\n config.c | 30 ++++++++++++++++++++++++++----\n config.h | 3 ++-\n t/t7900-maintenance.sh | 21 +++++++++++++++++++++\n 6 files changed, 57 insertions(+), 48 deletions(-)\n\n--\n2.42.0.2.g879ad04204\n"},{"id":"483451","messageId":"39934cb7e50ad0a5b287b13d0cdcf2f87a96d6f6.1697660181.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1697660181.git.code@khaugsbakk.name","subject":"[PATCH v1 1/4] config: format newlines","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-18T20:28:38Z","receivedAt":"2023-10-18T20:29:46Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"\nRemove unneeded newlines according to `clang-format`.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n Honestly the formatter changing these lines over and over again was just\n annoying. And we're visiting the file anyway.\n\n builtin/config.c | 1 -\n config.c | 2 --\n 2 files changed, 3 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 11a4d4ef141..87d0dc92d99 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tgiven_config_source.scope = CONFIG_SCOPE_COMMAND;\n \t}\n \n-\n \tif (respect_includes_opt == -1)\n \t\tconfig_options.respect_includes = !given_config_source.file;\n \telse\ndiff --git a/config.c b/config.c\nindex 3846a37be97..19f832818f1 100644\n--- a/config.c\n+++ b/config.c\n@@ -96,7 +96,6 @@ static long config_file_ftell(struct config_source *conf)\n \treturn ftell(conf->u.file);\n }\n \n-\n static int config_buf_fgetc(struct config_source *conf)\n {\n \tif (conf->u.buf.pos < conf->u.buf.len)\n@@ -3564,7 +3563,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n write_err_out:\n \tret = write_error(get_lock_file_path(&lock));\n \tgoto out_free;\n-\n }\n \n void git_config_set_multivar_in_file(const char *config_filename,\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483452","messageId":"48a5357f97cec2c5babc6512e0b30bcb8f7d201f.1697660181.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1697660181.git.code@khaugsbakk.name","subject":"[PATCH v1 2/4] config: rename global config function","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-18T20:28:39Z","receivedAt":"2023-10-18T20:29:48Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"\nRename this function to a more descriptive name since we want to use the\nexisting name for a new function.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n builtin/config.c | 2 +-\n builtin/gc.c | 4 ++--\n builtin/var.c | 2 +-\n config.c | 4 ++--\n config.h | 2 +-\n 5 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 87d0dc92d99..6fff2655816 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (use_global_config) {\n \t\tchar *user_config, *xdg_config;\n \n-\t\tgit_global_config(&user_config, &xdg_config);\n+\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\tif (!user_config)\n \t\t\t/*\n \t\t\t * It is unknown if HOME/.gitconfig exists, so\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 5c4315f0d81..17fc031f63a 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1529,7 +1529,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \t\tchar *user_config = NULL, *xdg_config = NULL;\n \n \t\tif (!config_file) {\n-\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\t\tconfig_file = user_config;\n \t\t\tif (!user_config)\n \t\t\t\tdie(_(\"$HOME not set\"));\n@@ -1597,7 +1597,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tint rc;\n \t\tchar *user_config = NULL, *xdg_config = NULL;\n \t\tif (!config_file) {\n-\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\t\tconfig_file = user_config;\n \t\t\tif (!user_config)\n \t\t\t\tdie(_(\"$HOME not set\"));\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 74161bdf1c6..8e18b50b1e5 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)\n \tchar *user, *xdg;\n \tsize_t unused;\n \n-\tgit_global_config(&user, &xdg);\n+\tgit_global_config_paths(&user, &xdg);\n \tif (xdg && *xdg) {\n \t\tnormalize_path_copy(xdg, xdg);\n \t\tstrbuf_addf(&buf, \"%s\\n\", xdg);\ndiff --git a/config.c b/config.c\nindex 19f832818f1..d2cdda96edd 100644\n--- a/config.c\n+++ b/config.c\n@@ -2111,7 +2111,7 @@ char *git_system_config(void)\n \treturn system_config;\n }\n \n-void git_global_config(char **user_out, char **xdg_out)\n+void git_global_config_paths(char **user_out, char **xdg_out)\n {\n \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\n \tchar *xdg_config = NULL;\n@@ -2186,7 +2186,7 @@ static int do_git_config_sequence(const struct config_options *opts,\n \t\t\t\t\t\t\t data, CONFIG_SCOPE_SYSTEM,\n \t\t\t\t\t\t\t NULL);\n \n-\tgit_global_config(&user_config, &xdg_config);\n+\tgit_global_config_paths(&user_config, &xdg_config);\n \n \tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file_with_options(fn, xdg_config, data,\ndiff --git a/config.h b/config.h\nindex 6332d749047..9f04de8ee3e 100644\n--- a/config.h\n+++ b/config.h\n@@ -394,7 +394,7 @@ int config_error_nonbool(const char *);\n #endif\n \n char *git_system_config(void);\n-void git_global_config(char **user, char **xdg);\n+void git_global_config_paths(char **user, char **xdg);\n \n int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n \n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483453","messageId":"147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1697660181.git.code@khaugsbakk.name","subject":"[PATCH v1 3/4] config: factor out global config file retrieval","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-18T20:28:40Z","receivedAt":"2023-10-18T20:29:49Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"\nFactor out code that retrieves the global config file so that we can use\nit in `gc.c` as well.\n\nUse the old name from the previous commit since this function acts\nfunctionally the same as `git_system_config` but for “global”.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n builtin/config.c | 25 ++-----------------------\n config.c | 24 ++++++++++++++++++++++++\n config.h | 1 +\n 3 files changed, 27 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 6fff2655816..df06b766fad 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_global_config) {\n-\t\tchar *user_config, *xdg_config;\n-\n-\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\tif (!user_config)\n-\t\t\t/*\n-\t\t\t * It is unknown if HOME/.gitconfig exists, so\n-\t\t\t * we do not know if we should write to XDG\n-\t\t\t * location; error out even if XDG_CONFIG_HOME\n-\t\t\t * is set and points at a sane location.\n-\t\t\t */\n-\t\t\tdie(_(\"$HOME not set\"));\n-\n+\t\tgiven_config_source.file = git_global_config();\n \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n-\n-\t\tif (access_or_warn(user_config, R_OK, 0) &&\n-\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n-\t\t\tgiven_config_source.file = xdg_config;\n-\t\t\tfree(user_config);\n-\t\t} else {\n-\t\t\tgiven_config_source.file = user_config;\n-\t\t\tfree(xdg_config);\n-\t\t}\n-\t}\n-\telse if (use_system_config) {\n+\t} else if (use_system_config) {\n \t\tgiven_config_source.file = git_system_config();\n \t\tgiven_config_source.scope = CONFIG_SCOPE_SYSTEM;\n \t} else if (use_local_config) {\ndiff --git a/config.c b/config.c\nindex d2cdda96edd..2ff766c56ff 100644\n--- a/config.c\n+++ b/config.c\n@@ -2111,6 +2111,30 @@ char *git_system_config(void)\n \treturn system_config;\n }\n \n+char *git_global_config(void)\n+{\n+\tchar *user_config, *xdg_config;\n+\n+\tgit_global_config_paths(&user_config, &xdg_config);\n+\tif (!user_config)\n+\t\t/*\n+\t\t * It is unknown if HOME/.gitconfig exists, so\n+\t\t * we do not know if we should write to XDG\n+\t\t * location; error out even if XDG_CONFIG_HOME\n+\t\t * is set and points at a sane location.\n+\t\t */\n+\t\tdie(_(\"$HOME not set\"));\n+\n+\tif (access_or_warn(user_config, R_OK, 0) && xdg_config &&\n+\t !access_or_warn(xdg_config, R_OK, 0)) {\n+\t\tfree(user_config);\n+\t\treturn xdg_config;\n+\t} else {\n+\t\tfree(xdg_config);\n+\t\treturn user_config;\n+\t}\n+}\n+\n void git_global_config_paths(char **user_out, char **xdg_out)\n {\n \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\ndiff --git a/config.h b/config.h\nindex 9f04de8ee3e..5cf961b548d 100644\n--- a/config.h\n+++ b/config.h\n@@ -394,6 +394,7 @@ int config_error_nonbool(const char *);\n #endif\n \n char *git_system_config(void);\n+char *git_global_config(void);\n void git_global_config_paths(char **user, char **xdg);\n \n int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483454","messageId":"1e2376a4b998b5b182cc5f72afc7282134bcdf2c.1697660181.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1697660181.git.code@khaugsbakk.name","subject":"[PATCH v1 4/4] maintenance: use XDG config if it exists","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-18T20:28:41Z","receivedAt":"2023-10-18T20:29:50Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"\n`git maintenance register` registers the repository in the user's global\nconfig. `$XDG_CONFIG_HOME/git/config` is supposed to be used if\n`~/.gitconfig` does not exist. However, this command creates a\n`~/.gitconfig` file and writes to that one even though the XDG variant\nexists.\n\nThis used to work correctly until 50a044f1e4 (gc: replace config\nsubprocesses with API calls, 2022-09-27), when the command started calling\nthe config API instead of git-config(1).\n\nAlso change `unregister` accordingly.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n builtin/gc.c | 23 +++++------------------\n t/t7900-maintenance.sh | 21 +++++++++++++++++++++\n 2 files changed, 26 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 17fc031f63a..7b780f2ab38 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1526,19 +1526,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \n \tif (!found) {\n \t\tint rc;\n-\t\tchar *user_config = NULL, *xdg_config = NULL;\n \n-\t\tif (!config_file) {\n-\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\t\tconfig_file = user_config;\n-\t\t\tif (!user_config)\n-\t\t\t\tdie(_(\"$HOME not set\"));\n-\t\t}\n+\t\tif (!config_file)\n+\t\t\tconfig_file = git_global_config();\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n \t\t\tCONFIG_REGEX_NONE, 0);\n-\t\tfree(user_config);\n-\t\tfree(xdg_config);\n \n \t\tif (rc)\n \t\t\tdie(_(\"unable to add '%s' value of '%s'\"),\n@@ -1595,18 +1588,12 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \n \tif (found) {\n \t\tint rc;\n-\t\tchar *user_config = NULL, *xdg_config = NULL;\n-\t\tif (!config_file) {\n-\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\t\tconfig_file = user_config;\n-\t\t\tif (!user_config)\n-\t\t\t\tdie(_(\"$HOME not set\"));\n-\t\t}\n+\n+\t\tif (!config_file)\n+\t\t\tconfig_file = git_global_config();\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, key, NULL, maintpath,\n \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n-\t\tfree(user_config);\n-\t\tfree(xdg_config);\n \n \t\tif (rc &&\n \t\t (!force || rc == CONFIG_NOTHING_SET))\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 487e326b3fa..a11e6c61520 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' '\n \ttest_subcommand ! git maintenance run --auto --quiet actual &&\n+\tpwd >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '\n+\ttest_when_finished git maintenance unregister &&\n+\ttest_path_is_missing \"$XDG_CONFIG_HOME\"/git/config &&\n+\tgit maintenance register &&\n+\tgit config --global --get maintenance.repo >actual &&\n+\tpwd >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'maintenance..enabled' '\n \tgit config maintenance.gc.enabled false &&\n \tgit config maintenance.commit-graph.enabled true &&\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483656","messageId":"ZTZDqToqcsDiS5AP@tanuki","threadId":"60395","inReplyTo":"147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name","subject":"Re: [PATCH v1 3/4] config: factor out global config file retrieval","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-23T09:58:01Z","receivedAt":"2023-10-23T09:58:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:\n> \n> Factor out code that retrieves the global config file so that we can use\n> it in `gc.c` as well.\n> \n> Use the old name from the previous commit since this function acts\n> functionally the same as `git_system_config` but for “global”.\n> \n> Signed-off-by: Kristoffer Haugsbakk \n> ---\n> builtin/config.c | 25 ++-----------------------\n> config.c | 24 ++++++++++++++++++++++++\n> config.h | 1 +\n> 3 files changed, 27 insertions(+), 23 deletions(-)\n> \n> diff --git a/builtin/config.c b/builtin/config.c\n> index 6fff2655816..df06b766fad 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tif (use_global_config) {\n> -\t\tchar *user_config, *xdg_config;\n> -\n> -\t\tgit_global_config_paths(&user_config, &xdg_config);\n> -\t\tif (!user_config)\n> -\t\t\t/*\n> -\t\t\t * It is unknown if HOME/.gitconfig exists, so\n> -\t\t\t * we do not know if we should write to XDG\n> -\t\t\t * location; error out even if XDG_CONFIG_HOME\n> -\t\t\t * is set and points at a sane location.\n> -\t\t\t */\n> -\t\t\tdie(_(\"$HOME not set\"));\n> -\n> +\t\tgiven_config_source.file = git_global_config();\n> \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n> -\n> -\t\tif (access_or_warn(user_config, R_OK, 0) &&\n> -\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n> -\t\t\tgiven_config_source.file = xdg_config;\n> -\t\t\tfree(user_config);\n> -\t\t} else {\n> -\t\t\tgiven_config_source.file = user_config;\n> -\t\t\tfree(xdg_config);\n> -\t\t}\n> -\t}\n> -\telse if (use_system_config) {\n> +\t} else if (use_system_config) {\n> \t\tgiven_config_source.file = git_system_config();\n> \t\tgiven_config_source.scope = CONFIG_SCOPE_SYSTEM;\n> \t} else if (use_local_config) {\n> diff --git a/config.c b/config.c\n> index d2cdda96edd..2ff766c56ff 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2111,6 +2111,30 @@ char *git_system_config(void)\n> \treturn system_config;\n> }\n> \n> +char *git_global_config(void)\n> +{\n> +\tchar *user_config, *xdg_config;\n> +\n> +\tgit_global_config_paths(&user_config, &xdg_config);\n> +\tif (!user_config)\n> +\t\t/*\n> +\t\t * It is unknown if HOME/.gitconfig exists, so\n> +\t\t * we do not know if we should write to XDG\n\nNit: we don't know about the intent of the caller, so they may not want\nto write to the file but only read it.\n\n> +\t\t * location; error out even if XDG_CONFIG_HOME\n> +\t\t * is set and points at a sane location.\n> +\t\t */\n> +\t\tdie(_(\"$HOME not set\"));\n\nIs it sensible to `die()` here in this new function that behaves more\nlike a library function? I imagine it would be more sensible to indicate\nthe error to the user and let them handle it accordingly.\n\nPatrick\n\n> +\n> +\tif (access_or_warn(user_config, R_OK, 0) && xdg_config &&\n> +\t !access_or_warn(xdg_config, R_OK, 0)) {\n> +\t\tfree(user_config);\n> +\t\treturn xdg_config;\n> +\t} else {\n> +\t\tfree(xdg_config);\n> +\t\treturn user_config;\n> +\t}\n> +}\n> +\n> void git_global_config_paths(char **user_out, char **xdg_out)\n> {\n> \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\n> diff --git a/config.h b/config.h\n> index 9f04de8ee3e..5cf961b548d 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -394,6 +394,7 @@ int config_error_nonbool(const char *);\n> #endif\n> \n> char *git_system_config(void);\n> +char *git_global_config(void);\n> void git_global_config_paths(char **user, char **xdg);\n> \n> int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n> -- \n> 2.42.0.2.g879ad04204\n> \n"},{"id":"483657","messageId":"ZTZDsIcrB0zwHlFR@tanuki","threadId":"60395","inReplyTo":"1e2376a4b998b5b182cc5f72afc7282134bcdf2c.1697660181.git.code@khaugsbakk.name","subject":"Re: [PATCH v1 4/4] maintenance: use XDG config if it exists","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-23T09:58:08Z","receivedAt":"2023-10-23T09:58:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 18, 2023 at 10:28:41PM +0200, Kristoffer Haugsbakk wrote:\n> \n> `git maintenance register` registers the repository in the user's global\n> config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if\n> `~/.gitconfig` does not exist. However, this command creates a\n> `~/.gitconfig` file and writes to that one even though the XDG variant\n> exists.\n> \n> This used to work correctly until 50a044f1e4 (gc: replace config\n> subprocesses with API calls, 2022-09-27), when the command started calling\n> the config API instead of git-config(1).\n> \n> Also change `unregister` accordingly.\n> \n> Signed-off-by: Kristoffer Haugsbakk \n> ---\n> builtin/gc.c | 23 +++++------------------\n> t/t7900-maintenance.sh | 21 +++++++++++++++++++++\n> 2 files changed, 26 insertions(+), 18 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 17fc031f63a..7b780f2ab38 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1526,19 +1526,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n> \n> \tif (!found) {\n> \t\tint rc;\n> -\t\tchar *user_config = NULL, *xdg_config = NULL;\n> \n> -\t\tif (!config_file) {\n> -\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n> -\t\t\tconfig_file = user_config;\n> -\t\t\tif (!user_config)\n> -\t\t\t\tdie(_(\"$HOME not set\"));\n> -\t\t}\n> +\t\tif (!config_file)\n> +\t\t\tconfig_file = git_global_config();\n> \t\trc = git_config_set_multivar_in_file_gently(\n> \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n> \t\t\tCONFIG_REGEX_NONE, 0);\n> -\t\tfree(user_config);\n> -\t\tfree(xdg_config);\n\nDon't we have to free `config_file` now?\n\n> \t\tif (rc)\n> \t\t\tdie(_(\"unable to add '%s' value of '%s'\"),\n> @@ -1595,18 +1588,12 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n> \n> \tif (found) {\n> \t\tint rc;\n> -\t\tchar *user_config = NULL, *xdg_config = NULL;\n> -\t\tif (!config_file) {\n> -\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n> -\t\t\tconfig_file = user_config;\n> -\t\t\tif (!user_config)\n> -\t\t\t\tdie(_(\"$HOME not set\"));\n> -\t\t}\n> +\n> +\t\tif (!config_file)\n> +\t\t\tconfig_file = git_global_config();\n> \t\trc = git_config_set_multivar_in_file_gently(\n> \t\t\tconfig_file, key, NULL, maintpath,\n> \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n> -\t\tfree(user_config);\n> -\t\tfree(xdg_config);\n\nSame here.\n\n> \t\tif (rc &&\n> \t\t (!force || rc == CONFIG_NOTHING_SET))\n> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\n> index 487e326b3fa..a11e6c61520 100755\n> --- a/t/t7900-maintenance.sh\n> +++ b/t/t7900-maintenance.sh\n> @@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' '\n> \ttest_subcommand ! git maintenance run --auto --quiet '\n> \n> +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '\n> +\tXDG_CONFIG_HOME=.config &&\n> +\ttest_when_finished rm -r \"$XDG_CONFIG_HOME\"/git/config &&\n> +\texport \"XDG_CONFIG_HOME\" &&\n\nStyle: there is no need to quote here.\n\nAlso, I think we need to unset this variable at the end of this test as\ntests don't run in a subshell. In theory, we should also be able to\nleave this variable unset completely as it will be derived from HOME\nautomatically anyway.\n\n> +\tmkdir -p \"$XDG_CONFIG_HOME\"/git &&\n> +\ttouch \"$XDG_CONFIG_HOME\"/git/config &&\n> +\tgit maintenance register &&\n> +\tgit config --file=\"$XDG_CONFIG_HOME\"/git/config --get maintenance.repo >actual &&\n> +\tpwd >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '\n> +\ttest_when_finished git maintenance unregister &&\n> +\ttest_path_is_missing \"$XDG_CONFIG_HOME\"/git/config &&\n> +\tgit maintenance register &&\n> +\tgit config --global --get maintenance.repo >actual &&\n> +\tpwd >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n\nAs you also change the code of `git maintenance unregister`, should we\ntest its behaviour, as well?\n\nPatrick\n\n> test_expect_success 'maintenance..enabled' '\n> \tgit config maintenance.gc.enabled false &&\n> \tgit config maintenance.commit-graph.enabled true &&\n> -- \n> 2.42.0.2.g879ad04204\n> \n"},{"id":"483665","messageId":"CAPig+cTx_wbs_b6he2O1EyUwZFp+5T8u6400_h87oCHxADb98A@mail.gmail.com","threadId":"60395","inReplyTo":"ZTZDsIcrB0zwHlFR@tanuki","subject":"Re: [PATCH v1 4/4] maintenance: use XDG config if it exists","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-23T11:39:25Z","receivedAt":"2023-10-23T11:39:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 23, 2023 at 5:58 AM Patrick Steinhardt wrote:\n> On Wed, Oct 18, 2023 at 10:28:41PM +0200, Kristoffer Haugsbakk wrote:\n> > `git maintenance register` registers the repository in the user's global\n> > config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if\n> > `~/.gitconfig` does not exist. However, this command creates a\n> > `~/.gitconfig` file and writes to that one even though the XDG variant\n> > exists.\n> >\n> > This used to work correctly until 50a044f1e4 (gc: replace config\n> > subprocesses with API calls, 2022-09-27), when the command started calling\n> > the config API instead of git-config(1).\n> >\n> > Also change `unregister` accordingly.\n> >\n> > Signed-off-by: Kristoffer Haugsbakk \n> > ---\n> > +test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '\n> > + XDG_CONFIG_HOME=.config &&\n> > + test_when_finished rm -r \"$XDG_CONFIG_HOME\"/git/config &&\n> > + export \"XDG_CONFIG_HOME\" &&\n>\n> Also, I think we need to unset this variable at the end of this test as\n> tests don't run in a subshell. [...]\n\nYup, well spotted. Almost the entire body of this test should be in a\nsubshell to ensure that the environment variable does not live beyond\nthe end of this test. But test_when_finished() can't be used in a\nsubshell, so a little care is needed:\n\n test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '\n test_when_finished rm -r .config/git/config &&\n (\n XDG_CONFIG_HOME=.config &&\n ...\n )\n '\n\n> > + mkdir -p \"$XDG_CONFIG_HOME\"/git &&\n> > + touch \"$XDG_CONFIG_HOME\"/git/config &&\n\nIf the timestamp of the file is not significant, then we use `>` to\ncreate it rather than `touch`:\n\n >\"$XDG_CONFIG_HOME\"/git/config &&\n\n> > + git maintenance register &&\n> > + git config --file=\"$XDG_CONFIG_HOME\"/git/config --get maintenance.repo >actual &&\n> > + pwd >expect &&\n> > + test_cmp expect actual\n> > +'\n"},{"id":"483689","messageId":"ZTav2u1JWmLexEHL@nand.local","threadId":"60395","inReplyTo":"ZTZDqToqcsDiS5AP@tanuki","subject":"Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-23T17:40:00Z","receivedAt":"2023-10-23T17:40:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 23, 2023 at 11:58:01AM +0200, Patrick Steinhardt wrote:\n> On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:\n> >\n> > Factor out code that retrieves the global config file so that we can use\n> > it in `gc.c` as well.\n> >\n> > Use the old name from the previous commit since this function acts\n> > functionally the same as `git_system_config` but for “global”.\n> >\n> > Signed-off-by: Kristoffer Haugsbakk \n> > ---\n> > builtin/config.c | 25 ++-----------------------\n> > config.c | 24 ++++++++++++++++++++++++\n> > config.h | 1 +\n> > 3 files changed, 27 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/builtin/config.c b/builtin/config.c\n> > index 6fff2655816..df06b766fad 100644\n> > --- a/builtin/config.c\n> > +++ b/builtin/config.c\n> > @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> > \t}\n> >\n> > \tif (use_global_config) {\n> > -\t\tchar *user_config, *xdg_config;\n> > -\n> > -\t\tgit_global_config_paths(&user_config, &xdg_config);\n> > -\t\tif (!user_config)\n> > -\t\t\t/*\n> > -\t\t\t * It is unknown if HOME/.gitconfig exists, so\n> > -\t\t\t * we do not know if we should write to XDG\n> > -\t\t\t * location; error out even if XDG_CONFIG_HOME\n> > -\t\t\t * is set and points at a sane location.\n> > -\t\t\t */\n> > -\t\t\tdie(_(\"$HOME not set\"));\n> > -\n> > +\t\tgiven_config_source.file = git_global_config();\n> > \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n> > -\n> > -\t\tif (access_or_warn(user_config, R_OK, 0) &&\n> > -\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n> > -\t\t\tgiven_config_source.file = xdg_config;\n> > -\t\t\tfree(user_config);\n> > -\t\t} else {\n> > -\t\t\tgiven_config_source.file = user_config;\n> > -\t\t\tfree(xdg_config);\n> > -\t\t}\n> > -\t}\n> > -\telse if (use_system_config) {\n> > +\t} else if (use_system_config) {\n> > \t\tgiven_config_source.file = git_system_config();\n> > \t\tgiven_config_source.scope = CONFIG_SCOPE_SYSTEM;\n> > \t} else if (use_local_config) {\n> > diff --git a/config.c b/config.c\n> > index d2cdda96edd..2ff766c56ff 100644\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -2111,6 +2111,30 @@ char *git_system_config(void)\n> > \treturn system_config;\n> > }\n> >\n> > +char *git_global_config(void)\n> > +{\n> > +\tchar *user_config, *xdg_config;\n> > +\n> > +\tgit_global_config_paths(&user_config, &xdg_config);\n> > +\tif (!user_config)\n> > +\t\t/*\n> > +\t\t * It is unknown if HOME/.gitconfig exists, so\n> > +\t\t * we do not know if we should write to XDG\n>\n> Nit: we don't know about the intent of the caller, so they may not want\n> to write to the file but only read it.\n\nI was going to suggest that we allow the caller to pass in the flags\nthat they wish for git_global_config() to pass down to access(2), but\nwas surprised to see that we always use R_OK.\n\nBut thinking on it for a moment longer, I realized that we don't care\nabout write-level permissions for the config, since we want to instead\nopen $GIT_DIR/config.lock for writing, and then rename() it into place,\nmeaning we only care about whether or not we have write permissions on\n$GIT_DIR itself.\n\nI think in the existing location of this code, the \"if we should write\"\nportion of the comment is premature, since we don't know for sure\nwhether or not we are writing. So I'd be fine with leaving it as-is, but\nchanging the comment seems easy enough to do...\n\n> > +\t\t * location; error out even if XDG_CONFIG_HOME\n> > +\t\t * is set and points at a sane location.\n> > +\t\t */\n> > +\t\tdie(_(\"$HOME not set\"));\n>\n> Is it sensible to `die()` here in this new function that behaves more\n> like a library function? I imagine it would be more sensible to indicate\n> the error to the user and let them handle it accordingly.\n\nAgreed.\n\nThanks,\nTaylor\n"},{"id":"483783","messageId":"87badbe0-de18-4f8a-9589-314cea46065e@app.fastmail.com","threadId":"60395","inReplyTo":"ZTav2u1JWmLexEHL@nand.local","subject":"Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-24T13:23:11Z","receivedAt":"2023-10-24T13:26:16Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi Taylor and Patrick\n\nOn Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:\n>> Nit: we don't know about the intent of the caller, so they may not want\n>> to write to the file but only read it.\n>\n> I was going to suggest that we allow the caller to pass in the flags\n> that they wish for git_global_config() to pass down to access(2), but\n> was surprised to see that we always use R_OK.\n>\n> But thinking on it for a moment longer, I realized that we don't care\n> about write-level permissions for the config, since we want to instead\n> open $GIT_DIR/config.lock for writing, and then rename() it into place,\n> meaning we only care about whether or not we have write permissions on\n> $GIT_DIR itself.\n>\n> I think in the existing location of this code, the \"if we should write\"\n> portion of the comment is premature, since we don't know for sure\n> whether or not we are writing. So I'd be fine with leaving it as-is, but\n> changing the comment seems easy enough to do...\n>\n>> > +\t\t * location; error out even if XDG_CONFIG_HOME\n>> > +\t\t * is set and points at a sane location.\n>> > +\t\t */\n>> > +\t\tdie(_(\"$HOME not set\"));\n>>\n>> Is it sensible to `die()` here in this new function that behaves more\n>> like a library function? I imagine it would be more sensible to indicate\n>> the error to the user and let them handle it accordingly.\n>\n> Agreed.\n>\n> Thanks,\n> Taylor\n\nWhat do you guys think the signature of `git_global_config` should be?\n"},{"id":"483829","messageId":"ZTip7JWm-WRWTImU@tanuki","threadId":"60395","inReplyTo":"87badbe0-de18-4f8a-9589-314cea46065e@app.fastmail.com","subject":"Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-25T05:38:52Z","receivedAt":"2023-10-25T05:39:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Oct 24, 2023 at 03:23:11PM +0200, Kristoffer Haugsbakk wrote:\n> Hi Taylor and Patrick\n> \n> On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:\n> >> Nit: we don't know about the intent of the caller, so they may not want\n> >> to write to the file but only read it.\n> >\n> > I was going to suggest that we allow the caller to pass in the flags\n> > that they wish for git_global_config() to pass down to access(2), but\n> > was surprised to see that we always use R_OK.\n> >\n> > But thinking on it for a moment longer, I realized that we don't care\n> > about write-level permissions for the config, since we want to instead\n> > open $GIT_DIR/config.lock for writing, and then rename() it into place,\n> > meaning we only care about whether or not we have write permissions on\n> > $GIT_DIR itself.\n> >\n> > I think in the existing location of this code, the \"if we should write\"\n> > portion of the comment is premature, since we don't know for sure\n> > whether or not we are writing. So I'd be fine with leaving it as-is, but\n> > changing the comment seems easy enough to do...\n> >\n> >> > +\t\t * location; error out even if XDG_CONFIG_HOME\n> >> > +\t\t * is set and points at a sane location.\n> >> > +\t\t */\n> >> > +\t\tdie(_(\"$HOME not set\"));\n> >>\n> >> Is it sensible to `die()` here in this new function that behaves more\n> >> like a library function? I imagine it would be more sensible to indicate\n> >> the error to the user and let them handle it accordingly.\n> >\n> > Agreed.\n> >\n> > Thanks,\n> > Taylor\n> \n> What do you guys think the signature of `git_global_config` should be?\n\nEither of the following:\n\n - `int git_global_config(char **out_pat)`\n - `char **git_global_config(void)`\n\nIn the first case you'd signal error via a non-zero return value,\nwhereas in the second case you would signal it via a `NULL` return\nvalue.\n\nTo decide which one to go with I'd recommend to check whether there is\nany similar precedent in \"config.h\" and what style that precedent uses.\n\nPatrick\n"},{"id":"483834","messageId":"2b764f52-d3ae-467f-a915-fb73beb247bb@app.fastmail.com","threadId":"60395","inReplyTo":"ZTip7JWm-WRWTImU@tanuki","subject":"Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-25T07:33:23Z","receivedAt":"2023-10-25T07:33:49Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:\n>> What do you guys think the signature of `git_global_config` should be?\n>\n> Either of the following:\n>\n> - `int git_global_config(char **out_pat)`\n> - `char **git_global_config(void)`\n>\n> In the first case you'd signal error via a non-zero return value,\n> whereas in the second case you would signal it via a `NULL` return\n> value.\n>\n> To decide which one to go with I'd recommend to check whether there is\n> any similar precedent in \"config.h\" and what style that precedent uses.\n\nOkay thanks. So no parameter for determining whether one is writing or\njust reading the file.\n\nCheers\n\n-- \nKristoffer Haugsbakk\n"},{"id":"483842","messageId":"ZTjMMC1GiPJUXnQm@tanuki","threadId":"60395","inReplyTo":"2b764f52-d3ae-467f-a915-fb73beb247bb@app.fastmail.com","subject":"Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-10-25T08:05:04Z","receivedAt":"2023-10-25T08:05:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Oct 25, 2023 at 09:33:23AM +0200, Kristoffer Haugsbakk wrote:\n> On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:\n> >> What do you guys think the signature of `git_global_config` should be?\n> >\n> > Either of the following:\n> >\n> > - `int git_global_config(char **out_pat)`\n> > - `char **git_global_config(void)`\n> >\n> > In the first case you'd signal error via a non-zero return value,\n> > whereas in the second case you would signal it via a `NULL` return\n> > value.\n> >\n> > To decide which one to go with I'd recommend to check whether there is\n> > any similar precedent in \"config.h\" and what style that precedent uses.\n> \n> Okay thanks. So no parameter for determining whether one is writing or\n> just reading the file.\n\nThis parameter would only exist for the purpose of the error message,\nright? If so, I think that'd be overkill. If we want to have differing\nerrors depending on how the function is called the best way to handle\nthat would likely be to generate the error message at the callsite\ninstead of in the library itself.\n\nPatrick\n"},{"id":"483997","messageId":"xmqq8r7ooyc8.fsf@gitster.g","threadId":"60395","inReplyTo":"ZTjMMC1GiPJUXnQm@tanuki","subject":"Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-27T15:54:15Z","receivedAt":"2023-10-27T15:54:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> This parameter would only exist for the purpose of the error message,\n> right? If so, I think that'd be overkill. If we want to have differing\n> errors depending on how the function is called the best way to handle\n> that would likely be to generate the error message at the callsite\n> instead of in the library itself.\n\nWe would need to make sure the lower-level helpers need to be able\nto tell what kind of failure they saw (in other words, why they are\nfailing) to the callers, which may require a bit of designing the\nerror return convention and plumbing through necessary pieces of\ninformation, but the longer term payoff would be great.\n\nI do not think this is such a case, but if the lower-level needs to\nfail differently (e.g., the thing not existing is acceptable when\nwriting as we will create a new one, but is a fail-worthy error when\nreading), then the caller needs to give that down the callchain,\nthough.\n"},{"id":"486782","messageId":"cover.1705267839.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1697660181.git.code@khaugsbakk.name","subject":"[PATCH v2 0/4] maintenance: use XDG config if it exists","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-14T21:43:15Z","receivedAt":"2024-01-14T21:44:11Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"I use the conventional XDG config path for the global configuration. This\npath is always used except for `git maintenance register` and\n`unregister`.\n\n§ Changes since v1\n\n• Free `config_file`\n • https://lore.kernel.org/git/ZTZDsIcrB0zwHlFR@tanuki/\n• Return `NULL` instead of dying\n • https://lore.kernel.org/git/ZTZDqToqcsDiS5AP@tanuki/\n• Tests\n • Test unregister\n • Use subshells\n • Style\n\n§ Patches\n\n• 1–3: Preparatory\n• 4: The desired change\n\n§ CC\n\n• Patrick Steinhardt: `config` changes; v1 feedback\n• Derrick Stolee: `maintenance` changes\n• Eric Sunshine: v1 feedback\n• Taylor Blau: v1 feedback\n\nKristoffer Haugsbakk (4):\n config: format newlines\n config: rename global config function\n config: factor out global config file retrieval\n maintenance: use XDG config if it exists\n\n builtin/config.c | 26 +++---------------------\n builtin/gc.c | 27 ++++++++++++-------------\n builtin/var.c | 2 +-\n config.c | 26 ++++++++++++++++++++----\n config.h | 6 +++++-\n t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 89 insertions(+), 43 deletions(-)\n\nRange-diff against v1:\n1: 39934cb7e50 = 1: d5f6c8d62ec config: format newlines\n2: 48a5357f97c = 2: cbc5fde0094 config: rename global config function\n3: 147c767443c ! 3: 32e5ec7d866 config: factor out global config file retrieval\n @@ Commit message\n\n Signed-off-by: Kristoffer Haugsbakk \n\n +\n + ## Notes (series) ##\n + v2:\n + • Don’t die; return `NULL`\n +\n ## builtin/config.c ##\n @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix\n -\t\t\t * location; error out even if XDG_CONFIG_HOME\n -\t\t\t * is set and points at a sane location.\n -\t\t\t */\n --\t\t\tdie(_(\"$HOME not set\"));\n --\n +\t\tgiven_config_source.file = git_global_config();\n ++\t\tif (!given_config_source.file)\n + \t\t\tdie(_(\"$HOME not set\"));\n +-\n \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n -\n -\t\tif (access_or_warn(user_config, R_OK, 0) &&\n @@ config.c: char *git_system_config(void)\n +\tchar *user_config, *xdg_config;\n +\n +\tgit_global_config_paths(&user_config, &xdg_config);\n -+\tif (!user_config)\n -+\t\t/*\n -+\t\t * It is unknown if HOME/.gitconfig exists, so\n -+\t\t * we do not know if we should write to XDG\n -+\t\t * location; error out even if XDG_CONFIG_HOME\n -+\t\t * is set and points at a sane location.\n -+\t\t */\n -+\t\tdie(_(\"$HOME not set\"));\n ++\tif (!user_config) {\n ++\t\tfree(xdg_config);\n ++\t\treturn NULL;\n ++\t}\n +\n +\tif (access_or_warn(user_config, R_OK, 0) && xdg_config &&\n +\t !access_or_warn(xdg_config, R_OK, 0)) {\n @@ config.h: int config_error_nonbool(const char *);\n #endif\n\n char *git_system_config(void);\n ++/**\n ++ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.\n ++ */\n +char *git_global_config(void);\n void git_global_config_paths(char **user, char **xdg);\n\n4: 1e2376a4b99 < -: ----------- maintenance: use XDG config if it exists\n-: ----------- > 4: 8bd67c5bf01 maintenance: use XDG config if it exists\n--\n2.43.0\n"},{"id":"486783","messageId":"d5f6c8d62ecc21859bdfbdfa3a601d7778ed444c.1705267839.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705267839.git.code@khaugsbakk.name","subject":"[PATCH v2 1/4] config: format newlines","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-14T21:43:16Z","receivedAt":"2024-01-14T21:44:13Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Remove unneeded newlines according to `clang-format`.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n Honestly the formatter changing these lines over and over again was just\n annoying. And we're visiting the file anyway.\n\n builtin/config.c | 1 -\n config.c | 2 --\n 2 files changed, 3 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 11a4d4ef141..87d0dc92d99 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tgiven_config_source.scope = CONFIG_SCOPE_COMMAND;\n \t}\n \n-\n \tif (respect_includes_opt == -1)\n \t\tconfig_options.respect_includes = !given_config_source.file;\n \telse\ndiff --git a/config.c b/config.c\nindex 9ff6ae1cb90..d26e16e3ce3 100644\n--- a/config.c\n+++ b/config.c\n@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)\n \treturn ftell(conf->u.file);\n }\n \n-\n static int config_buf_fgetc(struct config_source *conf)\n {\n \tif (conf->u.buf.pos < conf->u.buf.len)\n@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n write_err_out:\n \tret = write_error(get_lock_file_path(&lock));\n \tgoto out_free;\n-\n }\n \n void git_config_set_multivar_in_file(const char *config_filename,\n-- \n2.43.0\n\n"},{"id":"486784","messageId":"cbc5fde0094d11ee6c222ad8c88765b4c656301f.1705267839.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705267839.git.code@khaugsbakk.name","subject":"[PATCH v2 2/4] config: rename global config function","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-14T21:43:17Z","receivedAt":"2024-01-14T21:44:15Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Rename this function to a more descriptive name since we want to use the\nexisting name for a new function.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n builtin/config.c | 2 +-\n builtin/gc.c | 4 ++--\n builtin/var.c | 2 +-\n config.c | 4 ++--\n config.h | 2 +-\n 5 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 87d0dc92d99..6fff2655816 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (use_global_config) {\n \t\tchar *user_config, *xdg_config;\n \n-\t\tgit_global_config(&user_config, &xdg_config);\n+\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\tif (!user_config)\n \t\t\t/*\n \t\t\t * It is unknown if HOME/.gitconfig exists, so\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 7c11d5ebef0..c078751824c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1546,7 +1546,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \t\tchar *user_config = NULL, *xdg_config = NULL;\n \n \t\tif (!config_file) {\n-\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\t\tconfig_file = user_config;\n \t\t\tif (!user_config)\n \t\t\t\tdie(_(\"$HOME not set\"));\n@@ -1614,7 +1614,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tint rc;\n \t\tchar *user_config = NULL, *xdg_config = NULL;\n \t\tif (!config_file) {\n-\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\t\tconfig_file = user_config;\n \t\t\tif (!user_config)\n \t\t\t\tdie(_(\"$HOME not set\"));\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 8cf7dd9e2e5..cf5567208a2 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)\n \tchar *user, *xdg;\n \tsize_t unused;\n \n-\tgit_global_config(&user, &xdg);\n+\tgit_global_config_paths(&user, &xdg);\n \tif (xdg && *xdg) {\n \t\tnormalize_path_copy(xdg, xdg);\n \t\tstrbuf_addf(&buf, \"%s\\n\", xdg);\ndiff --git a/config.c b/config.c\nindex d26e16e3ce3..ebc6a57e1c3 100644\n--- a/config.c\n+++ b/config.c\n@@ -1987,7 +1987,7 @@ char *git_system_config(void)\n \treturn system_config;\n }\n \n-void git_global_config(char **user_out, char **xdg_out)\n+void git_global_config_paths(char **user_out, char **xdg_out)\n {\n \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\n \tchar *xdg_config = NULL;\n@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts,\n \t\t\t\t\t\t\t data, CONFIG_SCOPE_SYSTEM,\n \t\t\t\t\t\t\t NULL);\n \n-\tgit_global_config(&user_config, &xdg_config);\n+\tgit_global_config_paths(&user_config, &xdg_config);\n \n \tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file_with_options(fn, xdg_config, data,\ndiff --git a/config.h b/config.h\nindex 14f881ecfaf..e5e523553cc 100644\n--- a/config.h\n+++ b/config.h\n@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);\n #endif\n \n char *git_system_config(void);\n-void git_global_config(char **user, char **xdg);\n+void git_global_config_paths(char **user, char **xdg);\n \n int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n \n-- \n2.43.0\n\n"},{"id":"486785","messageId":"32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705267839.git.code@khaugsbakk.name","subject":"[PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-14T21:43:18Z","receivedAt":"2024-01-14T21:44:17Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Factor out code that retrieves the global config file so that we can use\nit in `gc.c` as well.\n\nUse the old name from the previous commit since this function acts\nfunctionally the same as `git_system_config` but for “global”.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n • Don’t die; return `NULL`\n\n builtin/config.c | 25 +++----------------------\n config.c | 20 ++++++++++++++++++++\n config.h | 4 ++++\n 3 files changed, 27 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 6fff2655816..08fe36d4997 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_global_config) {\n-\t\tchar *user_config, *xdg_config;\n-\n-\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\tif (!user_config)\n-\t\t\t/*\n-\t\t\t * It is unknown if HOME/.gitconfig exists, so\n-\t\t\t * we do not know if we should write to XDG\n-\t\t\t * location; error out even if XDG_CONFIG_HOME\n-\t\t\t * is set and points at a sane location.\n-\t\t\t */\n+\t\tgiven_config_source.file = git_global_config();\n+\t\tif (!given_config_source.file)\n \t\t\tdie(_(\"$HOME not set\"));\n-\n \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n-\n-\t\tif (access_or_warn(user_config, R_OK, 0) &&\n-\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n-\t\t\tgiven_config_source.file = xdg_config;\n-\t\t\tfree(user_config);\n-\t\t} else {\n-\t\t\tgiven_config_source.file = user_config;\n-\t\t\tfree(xdg_config);\n-\t\t}\n-\t}\n-\telse if (use_system_config) {\n+\t} else if (use_system_config) {\n \t\tgiven_config_source.file = git_system_config();\n \t\tgiven_config_source.scope = CONFIG_SCOPE_SYSTEM;\n \t} else if (use_local_config) {\ndiff --git a/config.c b/config.c\nindex ebc6a57e1c3..3cfeb3d8bd9 100644\n--- a/config.c\n+++ b/config.c\n@@ -1987,6 +1987,26 @@ char *git_system_config(void)\n \treturn system_config;\n }\n \n+char *git_global_config(void)\n+{\n+\tchar *user_config, *xdg_config;\n+\n+\tgit_global_config_paths(&user_config, &xdg_config);\n+\tif (!user_config) {\n+\t\tfree(xdg_config);\n+\t\treturn NULL;\n+\t}\n+\n+\tif (access_or_warn(user_config, R_OK, 0) && xdg_config &&\n+\t !access_or_warn(xdg_config, R_OK, 0)) {\n+\t\tfree(user_config);\n+\t\treturn xdg_config;\n+\t} else {\n+\t\tfree(xdg_config);\n+\t\treturn user_config;\n+\t}\n+}\n+\n void git_global_config_paths(char **user_out, char **xdg_out)\n {\n \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\ndiff --git a/config.h b/config.h\nindex e5e523553cc..625e932b993 100644\n--- a/config.h\n+++ b/config.h\n@@ -382,6 +382,10 @@ int config_error_nonbool(const char *);\n #endif\n \n char *git_system_config(void);\n+/**\n+ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.\n+ */\n+char *git_global_config(void);\n void git_global_config_paths(char **user, char **xdg);\n \n int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n-- \n2.43.0\n\n"},{"id":"486786","messageId":"8bd67c5bf01ca10fbf575dfa2cf88f8c88b48276.1705267839.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705267839.git.code@khaugsbakk.name","subject":"[PATCH v2 4/4] maintenance: use XDG config if it exists","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-14T21:43:19Z","receivedAt":"2024-01-14T21:44:19Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"`git maintenance register` registers the repository in the user's global\nconfig. `$XDG_CONFIG_HOME/git/config` is supposed to be used if\n`~/.gitconfig` does not exist. However, this command creates a\n`~/.gitconfig` file and writes to that one even though the XDG variant\nexists.\n\nThis used to work correctly until 50a044f1e4 (gc: replace config\nsubprocesses with API calls, 2022-09-27), when the command started calling\nthe config API instead of git-config(1).\n\nAlso change `unregister` accordingly.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n • Add `unregister` tests\n • Use subshell when exporting an env. variable\n • Style in tests\n • Free variables properly\n\n builtin/gc.c | 27 ++++++++++++-------------\n t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c078751824c..cb80ced6cb5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \n \tif (!found) {\n \t\tint rc;\n-\t\tchar *user_config = NULL, *xdg_config = NULL;\n+\t\tchar *global_config_file = NULL;\n \n \t\tif (!config_file) {\n-\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\t\tconfig_file = user_config;\n-\t\t\tif (!user_config)\n-\t\t\t\tdie(_(\"$HOME not set\"));\n+\t\t\tglobal_config_file = git_global_config();\n+\t\t\tconfig_file = global_config_file;\n \t\t}\n+\t\tif (!config_file)\n+\t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n \t\t\tCONFIG_REGEX_NONE, 0);\n-\t\tfree(user_config);\n-\t\tfree(xdg_config);\n+\t\tfree(global_config_file);\n \n \t\tif (rc)\n \t\t\tdie(_(\"unable to add '%s' value of '%s'\"),\n@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \n \tif (found) {\n \t\tint rc;\n-\t\tchar *user_config = NULL, *xdg_config = NULL;\n+\t\tchar *global_config_file = NULL;\n+\n \t\tif (!config_file) {\n-\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\t\tconfig_file = user_config;\n-\t\t\tif (!user_config)\n-\t\t\t\tdie(_(\"$HOME not set\"));\n+\t\t\tglobal_config_file = git_global_config();\n+\t\t\tconfig_file = global_config_file;\n \t\t}\n+\t\tif (!config_file)\n+\t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, key, NULL, maintpath,\n \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n-\t\tfree(user_config);\n-\t\tfree(xdg_config);\n+\t\tfree(global_config_file);\n \n \t\tif (rc &&\n \t\t (!force || rc == CONFIG_NOTHING_SET))\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 00d29871e65..0943dfa18a3 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -67,6 +67,51 @@ test_expect_success 'maintenance.auto config option' '\n \ttest_subcommand ! git maintenance run --auto --quiet $XDG_CONFIG_HOME/git/config &&\n+\t\tgit maintenance register &&\n+\t\tgit config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&\n+\t\tpwd >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '\n+\ttest_when_finished git maintenance unregister &&\n+\ttest_path_is_missing $XDG_CONFIG_HOME/git/config &&\n+\tgit maintenance register &&\n+\tgit config --global --get maintenance.repo >actual &&\n+\tpwd >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'unregister uses XDG_CONFIG_HOME config if it exists' '\n+\ttest_when_finished rm -r .config/git/config &&\n+\t(\n+\t\tXDG_CONFIG_HOME=.config &&\n+\t\texport XDG_CONFIG_HOME &&\n+\t\tmkdir -p $XDG_CONFIG_HOME/git &&\n+\t\t>$XDG_CONFIG_HOME/git/config &&\n+\t\tgit maintenance register &&\n+\t\tgit maintenance unregister &&\n+\t\ttest_must_fail git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&\n+\t\ttest_must_be_empty actual\n+\t)\n+'\n+\n+test_expect_success 'unregister does not need XDG_CONFIG_HOME config to exist' '\n+\ttest_path_is_missing $XDG_CONFIG_HOME/git/config &&\n+\tgit maintenance register &&\n+\tgit maintenance unregister &&\n+\ttest_must_fail git config --global --get maintenance.repo >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'maintenance..enabled' '\n \tgit config maintenance.gc.enabled false &&\n \tgit config maintenance.commit-graph.enabled true &&\n-- \n2.43.0\n\n"},{"id":"486868","messageId":"xmqqcyu1yn36.fsf@gitster.g","threadId":"60395","inReplyTo":"32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name","subject":"Re: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T21:39:41Z","receivedAt":"2024-01-16T21:39:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk writes:\n\n> \tif (use_global_config) {\n> -\t\tchar *user_config, *xdg_config;\n> ...\n> -\telse if (use_system_config) {\n> +\t} else if (use_system_config) {\n> \t\tgiven_config_source.file = git_system_config();\n> \t\tgiven_config_source.scope = CONFIG_SCOPE_SYSTEM;\n> \t} else if (use_local_config) {\n> diff --git a/config.c b/config.c\n> index ebc6a57e1c3..3cfeb3d8bd9 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1987,6 +1987,26 @@ char *git_system_config(void)\n> \treturn system_config;\n> }\n> \n> +char *git_global_config(void)\n> +{\n> +...\n> +}\n> +\n> void git_global_config_paths(char **user_out, char **xdg_out)\n> {\n> \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\n\nThe conversion above\n\n> diff --git a/config.h b/config.h\n> index e5e523553cc..625e932b993 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -382,6 +382,10 @@ int config_error_nonbool(const char *);\n> #endif\n> \n> char *git_system_config(void);\n> +/**\n> + * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.\n> + */\n\nSorry, but I am not sure what this comment wants to say.\n\nWhen $HOME is not set, we do get NULL out of this function. But\ninterpolate_path() that makes git_global_config_paths() to return\nNULL in user_config does not do any existence check with stat() or\naccess(), so even when we return a string that is \"~/.gitconfig\"\nexpanded to '/home/user/.gitconfig\", we are not certain if the file\nexists. So,... it is unclear what \"uncertain\"ty we are talking\nabout in this case.\n\n> +char *git_global_config(void);\n"},{"id":"486869","messageId":"c87b3d93-74db-4377-a57c-80f766d46e7f@app.fastmail.com","threadId":"60395","inReplyTo":"xmqqcyu1yn36.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-16T21:46:33Z","receivedAt":"2024-01-16T21:46:57Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Jan 16, 2024, at 22:39, Junio C Hamano wrote:\n> Kristoffer Haugsbakk writes:\n>> char *git_system_config(void);\n>> +/**\n>> + * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.\n>> + */\n>\n> Sorry, but I am not sure what this comment wants to say.\n>\n> When $HOME is not set, we do get NULL out of this function. But\n> interpolate_path() that makes git_global_config_paths() to return\n> NULL in user_config does not do any existence check with stat() or\n> access(), so even when we return a string that is \"~/.gitconfig\"\n> expanded to '/home/user/.gitconfig\", we are not certain if the file\n> exists. So,... it is unclear what \"uncertain\"ty we are talking\n> about in this case.\n\nI'll delete it. It was an attempt to refer to the comments about\n\"It is unknown if HOME/.gitconfig exists\".\n\nCheers\n\n-- \nKristoffer Haugsbakk\n"},{"id":"486870","messageId":"xmqq7ck9ymi3.fsf@gitster.g","threadId":"60395","inReplyTo":"8bd67c5bf01ca10fbf575dfa2cf88f8c88b48276.1705267839.git.code@khaugsbakk.name","subject":"Re: [PATCH v2 4/4] maintenance: use XDG config if it exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T21:52:20Z","receivedAt":"2024-01-16T21:52:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk writes:\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c078751824c..cb80ced6cb5 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n> \n> \tif (!found) {\n> \t\tint rc;\n> -\t\tchar *user_config = NULL, *xdg_config = NULL;\n> +\t\tchar *global_config_file = NULL;\n> \n> \t\tif (!config_file) {\n> -\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n> -\t\t\tconfig_file = user_config;\n> -\t\t\tif (!user_config)\n> -\t\t\t\tdie(_(\"$HOME not set\"));\n> +\t\t\tglobal_config_file = git_global_config();\n> +\t\t\tconfig_file = global_config_file;\n> \t\t}\n> +\t\tif (!config_file)\n> +\t\t\tdie(_(\"$HOME not set\"));\n> \t\trc = git_config_set_multivar_in_file_gently(\n> \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n> \t\t\tCONFIG_REGEX_NONE, 0);\n\nOK. We used to ask for both user and xdg and without using xdg at\nall, as long as $HOME is set, we used $HOME/.gitconfig even if it\ndid not exist.\n\nWhat we want to happen is we pick XDG is XDG exists *and* $HOME/.gitconfig\ndoes not. And that is exactly what git_global_config() gives us.\n\nNicely done.\n\n> @@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n> \n> \tif (found) {\n> \t\tint rc;\n> -\t\tchar *user_config = NULL, *xdg_config = NULL;\n> +\t\tchar *global_config_file = NULL;\n> +\n> \t\tif (!config_file) {\n> -\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n> -\t\t\tconfig_file = user_config;\n> -\t\t\tif (!user_config)\n> -\t\t\t\tdie(_(\"$HOME not set\"));\n> +\t\t\tglobal_config_file = git_global_config();\n> +\t\t\tconfig_file = global_config_file;\n> \t\t}\n> +\t\tif (!config_file)\n> +\t\t\tdie(_(\"$HOME not set\"));\n> \t\trc = git_config_set_multivar_in_file_gently(\n> \t\t\tconfig_file, key, NULL, maintpath,\n> \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n\nDitto.\n"},{"id":"486982","messageId":"cover.1705593810.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1697660181.git.code@khaugsbakk.name","subject":"[PATCH v3 0/4] maintenance: use XDG config if it exists","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-18T16:12:48Z","receivedAt":"2024-01-18T16:13:42Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"I use the conventional XDG config path for the global configuration. This\npath is always used except for `git maintenance register` and\n`unregister`.\n\n§ Changes since v2 (by patch)\n\n• config: factor out global config file retrieval\n • Remove doc on `git_global_config`\n • https://lore.kernel.org/git/c87b3d93-74db-4377-a57c-80f766d46e7f@app.fastmail.com/\n\n§ Patches\n\n• 1–3: Preparatory\n• 4: The desired change\n\n§ CC\n\n• Patrick Steinhardt: `config` changes; v1 feedback\n• Derrick Stolee: `maintenance` changes\n• Eric Sunshine: v1 feedback\n• Taylor Blau: v1 feedback\n• Junio C Hamano: v2 feedback\n\n§ CI\n\nhttps://github.com/LemmingAvalanche/git/actions/runs/7521230119\n\nKristoffer Haugsbakk (4):\n config: format newlines\n config: rename global config function\n config: factor out global config file retrieval\n maintenance: use XDG config if it exists\n\n builtin/config.c | 26 +++---------------------\n builtin/gc.c | 27 ++++++++++++-------------\n builtin/var.c | 2 +-\n config.c | 26 ++++++++++++++++++++----\n config.h | 3 ++-\n t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 86 insertions(+), 43 deletions(-)\n\nRange-diff against v2:\n1: d5f6c8d62ec = 1: 1c92b772ef4 config: format newlines\n2: cbc5fde0094 = 2: 269490794bc config: rename global config function\n3: 32e5ec7d866 ! 3: 0643a85892c config: factor out global config file retrieval\n @@ Commit message\n\n\n ## Notes (series) ##\n + v3:\n + • Remove doc on `git_global_config`\n + • https://lore.kernel.org/git/c87b3d93-74db-4377-a57c-80f766d46e7f@app.fastmail.com/\n v2:\n • Don’t die; return `NULL`\n\n @@ config.h: int config_error_nonbool(const char *);\n #endif\n\n char *git_system_config(void);\n -+/**\n -+ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.\n -+ */\n +char *git_global_config(void);\n void git_global_config_paths(char **user, char **xdg);\n\n4: 8bd67c5bf01 = 4: e0880af0a31 maintenance: use XDG config if it exists\n--\n2.43.0\n"},{"id":"486983","messageId":"1c92b772ef48a91e76b51fd58d941cfdcad93aec.1705593810.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705593810.git.code@khaugsbakk.name","subject":"[PATCH v3 1/4] config: format newlines","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-18T16:12:49Z","receivedAt":"2024-01-18T16:13:44Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Remove unneeded newlines according to `clang-format`.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n Honestly the formatter changing these lines over and over again was just\n annoying. And we're visiting the file anyway.\n\n builtin/config.c | 1 -\n config.c | 2 --\n 2 files changed, 3 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 11a4d4ef141..87d0dc92d99 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tgiven_config_source.scope = CONFIG_SCOPE_COMMAND;\n \t}\n \n-\n \tif (respect_includes_opt == -1)\n \t\tconfig_options.respect_includes = !given_config_source.file;\n \telse\ndiff --git a/config.c b/config.c\nindex 9ff6ae1cb90..d26e16e3ce3 100644\n--- a/config.c\n+++ b/config.c\n@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)\n \treturn ftell(conf->u.file);\n }\n \n-\n static int config_buf_fgetc(struct config_source *conf)\n {\n \tif (conf->u.buf.pos < conf->u.buf.len)\n@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n write_err_out:\n \tret = write_error(get_lock_file_path(&lock));\n \tgoto out_free;\n-\n }\n \n void git_config_set_multivar_in_file(const char *config_filename,\n-- \n2.43.0\n\n"},{"id":"486984","messageId":"269490794bc17053c151d6773c7157cfb30e35bb.1705593810.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705593810.git.code@khaugsbakk.name","subject":"[PATCH v3 2/4] config: rename global config function","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-18T16:12:50Z","receivedAt":"2024-01-18T16:13:46Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Rename this function to a more descriptive name since we want to use the\nexisting name for a new function.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n builtin/config.c | 2 +-\n builtin/gc.c | 4 ++--\n builtin/var.c | 2 +-\n config.c | 4 ++--\n config.h | 2 +-\n 5 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 87d0dc92d99..6fff2655816 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (use_global_config) {\n \t\tchar *user_config, *xdg_config;\n \n-\t\tgit_global_config(&user_config, &xdg_config);\n+\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\tif (!user_config)\n \t\t\t/*\n \t\t\t * It is unknown if HOME/.gitconfig exists, so\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 7c11d5ebef0..c078751824c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1546,7 +1546,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \t\tchar *user_config = NULL, *xdg_config = NULL;\n \n \t\tif (!config_file) {\n-\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\t\tconfig_file = user_config;\n \t\t\tif (!user_config)\n \t\t\t\tdie(_(\"$HOME not set\"));\n@@ -1614,7 +1614,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tint rc;\n \t\tchar *user_config = NULL, *xdg_config = NULL;\n \t\tif (!config_file) {\n-\t\t\tgit_global_config(&user_config, &xdg_config);\n+\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n \t\t\tconfig_file = user_config;\n \t\t\tif (!user_config)\n \t\t\t\tdie(_(\"$HOME not set\"));\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 8cf7dd9e2e5..cf5567208a2 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)\n \tchar *user, *xdg;\n \tsize_t unused;\n \n-\tgit_global_config(&user, &xdg);\n+\tgit_global_config_paths(&user, &xdg);\n \tif (xdg && *xdg) {\n \t\tnormalize_path_copy(xdg, xdg);\n \t\tstrbuf_addf(&buf, \"%s\\n\", xdg);\ndiff --git a/config.c b/config.c\nindex d26e16e3ce3..ebc6a57e1c3 100644\n--- a/config.c\n+++ b/config.c\n@@ -1987,7 +1987,7 @@ char *git_system_config(void)\n \treturn system_config;\n }\n \n-void git_global_config(char **user_out, char **xdg_out)\n+void git_global_config_paths(char **user_out, char **xdg_out)\n {\n \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\n \tchar *xdg_config = NULL;\n@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts,\n \t\t\t\t\t\t\t data, CONFIG_SCOPE_SYSTEM,\n \t\t\t\t\t\t\t NULL);\n \n-\tgit_global_config(&user_config, &xdg_config);\n+\tgit_global_config_paths(&user_config, &xdg_config);\n \n \tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file_with_options(fn, xdg_config, data,\ndiff --git a/config.h b/config.h\nindex 14f881ecfaf..e5e523553cc 100644\n--- a/config.h\n+++ b/config.h\n@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);\n #endif\n \n char *git_system_config(void);\n-void git_global_config(char **user, char **xdg);\n+void git_global_config_paths(char **user, char **xdg);\n \n int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n \n-- \n2.43.0\n\n"},{"id":"486985","messageId":"0643a85892cf8a7732593646f1e8a4d0e37d38b7.1705593810.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705593810.git.code@khaugsbakk.name","subject":"[PATCH v3 3/4] config: factor out global config file retrieval","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-18T16:12:51Z","receivedAt":"2024-01-18T16:13:49Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Factor out code that retrieves the global config file so that we can use\nit in `gc.c` as well.\n\nUse the old name from the previous commit since this function acts\nfunctionally the same as `git_system_config` but for “global”.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v3:\n • Remove doc on `git_global_config`\n • https://lore.kernel.org/git/c87b3d93-74db-4377-a57c-80f766d46e7f@app.fastmail.com/\n v2:\n • Don’t die; return `NULL`\n\n builtin/config.c | 25 +++----------------------\n config.c | 20 ++++++++++++++++++++\n config.h | 1 +\n 3 files changed, 24 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 6fff2655816..08fe36d4997 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_global_config) {\n-\t\tchar *user_config, *xdg_config;\n-\n-\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\tif (!user_config)\n-\t\t\t/*\n-\t\t\t * It is unknown if HOME/.gitconfig exists, so\n-\t\t\t * we do not know if we should write to XDG\n-\t\t\t * location; error out even if XDG_CONFIG_HOME\n-\t\t\t * is set and points at a sane location.\n-\t\t\t */\n+\t\tgiven_config_source.file = git_global_config();\n+\t\tif (!given_config_source.file)\n \t\t\tdie(_(\"$HOME not set\"));\n-\n \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n-\n-\t\tif (access_or_warn(user_config, R_OK, 0) &&\n-\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n-\t\t\tgiven_config_source.file = xdg_config;\n-\t\t\tfree(user_config);\n-\t\t} else {\n-\t\t\tgiven_config_source.file = user_config;\n-\t\t\tfree(xdg_config);\n-\t\t}\n-\t}\n-\telse if (use_system_config) {\n+\t} else if (use_system_config) {\n \t\tgiven_config_source.file = git_system_config();\n \t\tgiven_config_source.scope = CONFIG_SCOPE_SYSTEM;\n \t} else if (use_local_config) {\ndiff --git a/config.c b/config.c\nindex ebc6a57e1c3..3cfeb3d8bd9 100644\n--- a/config.c\n+++ b/config.c\n@@ -1987,6 +1987,26 @@ char *git_system_config(void)\n \treturn system_config;\n }\n \n+char *git_global_config(void)\n+{\n+\tchar *user_config, *xdg_config;\n+\n+\tgit_global_config_paths(&user_config, &xdg_config);\n+\tif (!user_config) {\n+\t\tfree(xdg_config);\n+\t\treturn NULL;\n+\t}\n+\n+\tif (access_or_warn(user_config, R_OK, 0) && xdg_config &&\n+\t !access_or_warn(xdg_config, R_OK, 0)) {\n+\t\tfree(user_config);\n+\t\treturn xdg_config;\n+\t} else {\n+\t\tfree(xdg_config);\n+\t\treturn user_config;\n+\t}\n+}\n+\n void git_global_config_paths(char **user_out, char **xdg_out)\n {\n \tchar *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\ndiff --git a/config.h b/config.h\nindex e5e523553cc..5dba984f770 100644\n--- a/config.h\n+++ b/config.h\n@@ -382,6 +382,7 @@ int config_error_nonbool(const char *);\n #endif\n \n char *git_system_config(void);\n+char *git_global_config(void);\n void git_global_config_paths(char **user, char **xdg);\n \n int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n-- \n2.43.0\n\n"},{"id":"486986","messageId":"e0880af0a31aff06ce1d3da97aea568d3e2641b4.1705593810.git.code@khaugsbakk.name","threadId":"60395","inReplyTo":"cover.1705593810.git.code@khaugsbakk.name","subject":"[PATCH v3 4/4] maintenance: use XDG config if it exists","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-18T16:12:52Z","receivedAt":"2024-01-18T16:13:51Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"`git maintenance register` registers the repository in the user's global\nconfig. `$XDG_CONFIG_HOME/git/config` is supposed to be used if\n`~/.gitconfig` does not exist. However, this command creates a\n`~/.gitconfig` file and writes to that one even though the XDG variant\nexists.\n\nThis used to work correctly until 50a044f1e4 (gc: replace config\nsubprocesses with API calls, 2022-09-27), when the command started calling\nthe config API instead of git-config(1).\n\nAlso change `unregister` accordingly.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n • Add `unregister` tests\n • Use subshell when exporting an env. variable\n • Style in tests\n • Free variables properly\n\n builtin/gc.c | 27 ++++++++++++-------------\n t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c078751824c..cb80ced6cb5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \n \tif (!found) {\n \t\tint rc;\n-\t\tchar *user_config = NULL, *xdg_config = NULL;\n+\t\tchar *global_config_file = NULL;\n \n \t\tif (!config_file) {\n-\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\t\tconfig_file = user_config;\n-\t\t\tif (!user_config)\n-\t\t\t\tdie(_(\"$HOME not set\"));\n+\t\t\tglobal_config_file = git_global_config();\n+\t\t\tconfig_file = global_config_file;\n \t\t}\n+\t\tif (!config_file)\n+\t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n \t\t\tCONFIG_REGEX_NONE, 0);\n-\t\tfree(user_config);\n-\t\tfree(xdg_config);\n+\t\tfree(global_config_file);\n \n \t\tif (rc)\n \t\t\tdie(_(\"unable to add '%s' value of '%s'\"),\n@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \n \tif (found) {\n \t\tint rc;\n-\t\tchar *user_config = NULL, *xdg_config = NULL;\n+\t\tchar *global_config_file = NULL;\n+\n \t\tif (!config_file) {\n-\t\t\tgit_global_config_paths(&user_config, &xdg_config);\n-\t\t\tconfig_file = user_config;\n-\t\t\tif (!user_config)\n-\t\t\t\tdie(_(\"$HOME not set\"));\n+\t\t\tglobal_config_file = git_global_config();\n+\t\t\tconfig_file = global_config_file;\n \t\t}\n+\t\tif (!config_file)\n+\t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, key, NULL, maintpath,\n \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n-\t\tfree(user_config);\n-\t\tfree(xdg_config);\n+\t\tfree(global_config_file);\n \n \t\tif (rc &&\n \t\t (!force || rc == CONFIG_NOTHING_SET))\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 00d29871e65..0943dfa18a3 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -67,6 +67,51 @@ test_expect_success 'maintenance.auto config option' '\n \ttest_subcommand ! git maintenance run --auto --quiet $XDG_CONFIG_HOME/git/config &&\n+\t\tgit maintenance register &&\n+\t\tgit config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&\n+\t\tpwd >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '\n+\ttest_when_finished git maintenance unregister &&\n+\ttest_path_is_missing $XDG_CONFIG_HOME/git/config &&\n+\tgit maintenance register &&\n+\tgit config --global --get maintenance.repo >actual &&\n+\tpwd >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'unregister uses XDG_CONFIG_HOME config if it exists' '\n+\ttest_when_finished rm -r .config/git/config &&\n+\t(\n+\t\tXDG_CONFIG_HOME=.config &&\n+\t\texport XDG_CONFIG_HOME &&\n+\t\tmkdir -p $XDG_CONFIG_HOME/git &&\n+\t\t>$XDG_CONFIG_HOME/git/config &&\n+\t\tgit maintenance register &&\n+\t\tgit maintenance unregister &&\n+\t\ttest_must_fail git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&\n+\t\ttest_must_be_empty actual\n+\t)\n+'\n+\n+test_expect_success 'unregister does not need XDG_CONFIG_HOME config to exist' '\n+\ttest_path_is_missing $XDG_CONFIG_HOME/git/config &&\n+\tgit maintenance register &&\n+\tgit maintenance unregister &&\n+\ttest_must_fail git config --global --get maintenance.repo >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'maintenance..enabled' '\n \tgit config maintenance.gc.enabled false &&\n \tgit config maintenance.commit-graph.enabled true &&\n-- \n2.43.0\n\n"},{"id":"487046","messageId":"ZaoUOPsze7rhtT2M@tanuki","threadId":"60395","inReplyTo":"32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name","subject":"Re: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-19T06:18:32Z","receivedAt":"2024-01-19T06:18:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 14, 2024 at 10:43:18PM +0100, Kristoffer Haugsbakk wrote:\n> Factor out code that retrieves the global config file so that we can use\n> it in `gc.c` as well.\n> \n> Use the old name from the previous commit since this function acts\n> functionally the same as `git_system_config` but for “global”.\n\nI was briefly wondering whether we also want to give this new function a\nmore descriptive name. For one, calling it `git_system_config()` which\nwe have just removed in the preceding set may easily lead to confusion\nfor any in-flight patch series because the parameters now got dropped\n(or at least it looks like that).\n\nBut second, I think that the new function you introduce here has the\nsame issue as the old function that you refactored in the preceding\npatch: `git_config_global()` isn't very descriptive, and it is also\ninconsistent the new `git_config_global_paths()`. I'd propose to name\nthe new function something like `git_config_global_preferred_path()` or\n`git_config_global_path()`.\n\nSorry for not mentioning this in my first review round. Also, it's only\na minor concern, nothing that needs to block this series if either you\nor others disagree with my opinion.\n\nPatrick\n"},{"id":"487054","messageId":"7f0864ad-c846-42a6-8ddc-85d6be58a4ee@app.fastmail.com","threadId":"60395","inReplyTo":"ZaoUOPsze7rhtT2M@tanuki","subject":"Re: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-19T07:40:51Z","receivedAt":"2024-01-19T07:41:12Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:\n> But second, I think that the new function you introduce here has the\n> same issue as the old function that you refactored in the preceding\n> patch: `git_config_global()` isn't very descriptive, and it is also\n> inconsistent the new `git_config_global_paths()`. I'd propose to name\n> the new function something like `git_config_global_preferred_path()` or\n> `git_config_global_path()`.\n\nThe choice of `git_config_global` is mostly motivated by it working the\nsame way as `git_config_system`:\n\n```\ngiven_config_source.file = git_system_config();\n[…]\ngiven_config_source.file = git_global_config();\n```\n\n(The extra logic imposed by XDG for “global” is implied by `man git\nconfig`. I don’t know what the guidelines are for spelling that out or not\nin the internal functions.)\n\nYour suggestion makes sense. But should `git_system_config` be renamed as\nwell?\n\n-- \nKristoffer Haugsbakk\n"},{"id":"487056","messageId":"Zaor5zNLKE3UXhHM@tanuki","threadId":"60395","inReplyTo":"7f0864ad-c846-42a6-8ddc-85d6be58a4ee@app.fastmail.com","subject":"Re: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-19T07:59:35Z","receivedAt":"2024-01-19T07:59:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 19, 2024 at 08:40:51AM +0100, Kristoffer Haugsbakk wrote:\n> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:\n> > But second, I think that the new function you introduce here has the\n> > same issue as the old function that you refactored in the preceding\n> > patch: `git_config_global()` isn't very descriptive, and it is also\n> > inconsistent the new `git_config_global_paths()`. I'd propose to name\n> > the new function something like `git_config_global_preferred_path()` or\n> > `git_config_global_path()`.\n> \n> The choice of `git_config_global` is mostly motivated by it working the\n> same way as `git_config_system`:\n> \n> ```\n> given_config_source.file = git_system_config();\n> […]\n> given_config_source.file = git_global_config();\n> ```\n> \n> (The extra logic imposed by XDG for “global” is implied by `man git\n> config`. I don’t know what the guidelines are for spelling that out or not\n> in the internal functions.)\n> \n> Your suggestion makes sense. But should `git_system_config` be renamed as\n> well?\n\nYeah, you're right that `git_system_config()` is bad in the same way. In\nfact I think it's worse here because we have both `git_config_system()`\nand `git_system_config()`, which has certainly confused me multiple\ntimes in the past. So I'd be happy to see it renamed, as well, either\nnow or in a follow-up patch series.\n\nBut as I said, I don't think it's a prereq for this patch series to land\nand others may have differing opinions. So please, go ahead as you deem\nfit (or wait for other opinions).\n\nPatrick\n"},{"id":"487083","messageId":"xmqq34utkw6i.fsf@gitster.g","threadId":"60395","inReplyTo":"7f0864ad-c846-42a6-8ddc-85d6be58a4ee@app.fastmail.com","subject":"Re: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-19T18:36:05Z","receivedAt":"2024-01-19T18:36:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:\n>> But second, I think that the new function you introduce here has the\n>> same issue as the old function that you refactored in the preceding\n>> patch: `git_config_global()` isn't very descriptive, and it is also\n>> inconsistent the new `git_config_global_paths()`. I'd propose to name\n>> the new function something like `git_config_global_preferred_path()` or\n>> `git_config_global_path()`.\n>\n> The choice of `git_config_global` is mostly motivated by it working the\n> same way as `git_config_system`:\n>\n> ```\n> given_config_source.file = git_system_config();\n> […]\n> given_config_source.file = git_global_config();\n> ```\n\nI shared the above understanding with you, so I didn't find the name\n\"not very descriptive\" during my review. If only we had two more\nfunctions that can replace our uses of repo_git_path(r, \"config\")\nand repo_git_path(r, \"config.worktree\") [*] in the code, to obtain\nthe path to the repository local and worktree local configuration\nfiles, the convention may have been more obvious.\n\n Side note: the worktree specific one is messier; there are code\n paths that use \"%s/config.worktree\" on gitdir as well---if we\n were to introduce helpers, we should catch and convert them, too.\n\n> Your suggestion makes sense. But should `git_system_config` be renamed as\n> well?\n\nI do not mind including \"path\" in the names of these functions, but\nI do agree that such renaming should be done consistently across the\nfamily of functions (which we currently have only two members, but\nstill).\n\nThanks.\n"},{"id":"487085","messageId":"01e901da4b09$b4548dc0$1cfda940$@nexbridge.com","threadId":"60395","inReplyTo":"xmqq34utkw6i.fsf@gitster.g","subject":"RE: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-01-19T18:59:55Z","receivedAt":"2024-01-19T19:00:16Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Friday, January 19, 2024 1:36 PM, Junio C Hamano wrote:\n>\"Kristoffer Haugsbakk\" writes:\n>\n>> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:\n>>> But second, I think that the new function you introduce here has the\n>>> same issue as the old function that you refactored in the preceding\n>>> patch: `git_config_global()` isn't very descriptive, and it is also\n>>> inconsistent the new `git_config_global_paths()`. I'd propose to name\n>>> the new function something like `git_config_global_preferred_path()`\n>>> or `git_config_global_path()`.\n>>\n>> The choice of `git_config_global` is mostly motivated by it working\n>> the same way as `git_config_system`:\n>>\n>> ```\n>> given_config_source.file = git_system_config(); […]\n>> given_config_source.file = git_global_config(); ```\n>\n>I shared the above understanding with you, so I didn't find the name \"not very\n>descriptive\" during my review. If only we had two more functions that can replace\n>our uses of repo_git_path(r, \"config\") and repo_git_path(r, \"config.worktree\") [*] in\n>the code, to obtain the path to the repository local and worktree local configuration\n>files, the convention may have been more obvious.\n>\n> Side note: the worktree specific one is messier; there are code\n> paths that use \"%s/config.worktree\" on gitdir as well---if we\n> were to introduce helpers, we should catch and convert them, too.\n>\n>> Your suggestion makes sense. But should `git_system_config` be renamed\n>> as well?\n>\n>I do not mind including \"path\" in the names of these functions, but I do agree that\n>such renaming should be done consistently across the family of functions (which\n>we currently have only two members, but still).\n\nIs this going to impact the libification effort? I am just curious what other on the team think.\n--Randall\n\n"},{"id":"487116","messageId":"xmqq8r4lhqmv.fsf@gitster.g","threadId":"60395","inReplyTo":"Zaor5zNLKE3UXhHM@tanuki","subject":"Re: [PATCH v2 3/4] config: factor out global config file retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-19T23:04:08Z","receivedAt":"2024-01-19T23:04:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Yeah, you're right that `git_system_config()` is bad in the same way. In\n> fact I think it's worse here because we have both `git_config_system()`\n> and `git_system_config()`, which has certainly confused me multiple\n> times in the past. So I'd be happy to see it renamed, as well, either\n> now or in a follow-up patch series.\n\nOK, let's make a note #leftoverbits here and merge the topic down to\n'next'. By the time it graduates to 'master', we may have a clean-up\npatch to rename them.\n\nThanks, all.\n"}]}