[PATCH v1 0/4] maintenance: use XDG config if it exists

[PATCH v1 0/4] maintenance: use XDG config if it exists

[Kristoffer Haugsbakk]

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/448cc6ed-c441-85a3-2780-0c07e56f53f8@dunelm.org.uk/

§ 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 v1 1/4] config: format newlines

[Kristoffer Haugsbakk]

Remove unneeded newlines according to `clang-format`.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

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

diff --git a/builtin/config.c b/builtin/config.c
index 11a4d4ef141..87d0dc92d99 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -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
diff --git a/config.c b/config.c
index 3846a37be97..19f832818f1 100644
--- a/config.c
+++ b/config.c
@@ -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,
-- 
2.42.0.2.g879ad04204

[PATCH v1 2/4] config: rename global config function

[Kristoffer Haugsbakk]

Rename this function to a more descriptive name since we want to use the
existing name for a new function.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 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(-)

diff --git a/builtin/config.c b/builtin/config.c
index 87d0dc92d99..6fff2655816 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -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
diff --git a/builtin/gc.c b/builtin/gc.c
index 5c4315f0d81..17fc031f63a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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 *prefi
 		int 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"));
diff --git a/builtin/var.c b/builtin/var.c
index 74161bdf1c6..8e18b50b1e5 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -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);
diff --git a/config.c b/config.c
index 19f832818f1..d2cdda96edd 100644
--- a/config.c
+++ b/config.c
@@ -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,
diff --git a/config.h b/config.h
index 6332d749047..9f04de8ee3e 100644
--- a/config.h
+++ b/config.h
@@ -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);
 
-- 
2.42.0.2.g879ad04204

[PATCH v1 3/4] config: factor out global config file retrieval

[Kristoffer Haugsbakk]

Factor out code that retrieves the global config file so that we can use
it in `gc.c` as well.

Use the old name from the previous commit since this function acts
functionally the same as `git_system_config` but for “global”.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/config.c | 25 ++-----------------------
 config.c         | 24 ++++++++++++++++++++++++
 config.h         |  1 +
 3 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 6fff2655816..df06b766fad 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (use_global_config) {
-		char *user_config, *xdg_config;
-
-		git_global_config_paths(&user_config, &xdg_config);
-		if (!user_config)
-			/*
-			 * It is unknown if HOME/.gitconfig exists, so
-			 * we do not know if we should write to XDG
-			 * location; error out even if XDG_CONFIG_HOME
-			 * is set and points at a sane location.
-			 */
-			die(_("$HOME not set"));
-
+		given_config_source.file = git_global_config();
 		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-		if (access_or_warn(user_config, R_OK, 0) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
-			given_config_source.file = xdg_config;
-			free(user_config);
-		} else {
-			given_config_source.file = user_config;
-			free(xdg_config);
-		}
-	}
-	else if (use_system_config) {
+	} else if (use_system_config) {
 		given_config_source.file = git_system_config();
 		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
 	} else if (use_local_config) {
diff --git a/config.c b/config.c
index d2cdda96edd..2ff766c56ff 100644
--- a/config.c
+++ b/config.c
@@ -2111,6 +2111,30 @@ char *git_system_config(void)
 	return system_config;
 }
 
+char *git_global_config(void)
+{
+	char *user_config, *xdg_config;
+
+	git_global_config_paths(&user_config, &xdg_config);
+	if (!user_config)
+		/*
+		 * It is unknown if HOME/.gitconfig exists, so
+		 * we do not know if we should write to XDG
+		 * location; error out even if XDG_CONFIG_HOME
+		 * is set and points at a sane location.
+		 */
+		die(_("$HOME not set"));
+
+	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+	    !access_or_warn(xdg_config, R_OK, 0)) {
+		free(user_config);
+		return xdg_config;
+	} else {
+		free(xdg_config);
+		return user_config;
+	}
+}
+
 void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
diff --git a/config.h b/config.h
index 9f04de8ee3e..5cf961b548d 100644
--- a/config.h
+++ b/config.h
@@ -394,6 +394,7 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
+char *git_global_config(void);
 void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
-- 
2.42.0.2.g879ad04204

[PATCH v1 4/4] maintenance: use XDG config if it exists

[Kristoffer Haugsbakk]

`git 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.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/gc.c           | 23 +++++------------------
 t/t7900-maintenance.sh | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 17fc031f63a..7b780f2ab38 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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 *prefi
 
 	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, key, NULL, maintpath,
 			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
-		free(user_config);
-		free(xdg_config);
 
 		if (rc &&
 		    (!force || rc == CONFIG_NOTHING_SET))
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 487e326b3fa..a11e6c61520 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -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 &&
-- 
2.42.0.2.g879ad04204

Re: [PATCH v1 3/4] config: factor out global config file retrieval

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3656 bytes --]

On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:
> 
> Factor out code that retrieves the global config file so that we can use
> it in `gc.c` as well.
> 
> Use the old name from the previous commit since this function acts
> functionally the same as `git_system_config` but for “global”.
> 
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  builtin/config.c | 25 ++-----------------------
>  config.c         | 24 ++++++++++++++++++++++++
>  config.h         |  1 +
>  3 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index 6fff2655816..df06b766fad 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (use_global_config) {
> -		char *user_config, *xdg_config;
> -
> -		git_global_config_paths(&user_config, &xdg_config);
> -		if (!user_config)
> -			/*
> -			 * It is unknown if HOME/.gitconfig exists, so
> -			 * we do not know if we should write to XDG
> -			 * location; error out even if XDG_CONFIG_HOME
> -			 * is set and points at a sane location.
> -			 */
> -			die(_("$HOME not set"));
> -
> +		given_config_source.file = git_global_config();
>  		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> -
> -		if (access_or_warn(user_config, R_OK, 0) &&
> -		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> -			given_config_source.file = xdg_config;
> -			free(user_config);
> -		} else {
> -			given_config_source.file = user_config;
> -			free(xdg_config);
> -		}
> -	}
> -	else if (use_system_config) {
> +	} else if (use_system_config) {
>  		given_config_source.file = git_system_config();
>  		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
>  	} else if (use_local_config) {
> diff --git a/config.c b/config.c
> index d2cdda96edd..2ff766c56ff 100644
> --- a/config.c
> +++ b/config.c
> @@ -2111,6 +2111,30 @@ char *git_system_config(void)
>  	return system_config;
>  }
>  
> +char *git_global_config(void)
> +{
> +	char *user_config, *xdg_config;
> +
> +	git_global_config_paths(&user_config, &xdg_config);
> +	if (!user_config)
> +		/*
> +		 * It is unknown if HOME/.gitconfig exists, so
> +		 * we do not know if we should write to XDG

Nit: we don't know about the intent of the caller, so they may not want
to write to the file but only read it.

> +		 * location; error out even if XDG_CONFIG_HOME
> +		 * is set and points at a sane location.
> +		 */
> +		die(_("$HOME not set"));

Is it sensible to `die()` here in this new function that behaves more
like a library function? I imagine it would be more sensible to indicate
the error to the user and let them handle it accordingly.

Patrick

> +
> +	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
> +	    !access_or_warn(xdg_config, R_OK, 0)) {
> +		free(user_config);
> +		return xdg_config;
> +	} else {
> +		free(xdg_config);
> +		return user_config;
> +	}
> +}
> +
>  void git_global_config_paths(char **user_out, char **xdg_out)
>  {
>  	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
> diff --git a/config.h b/config.h
> index 9f04de8ee3e..5cf961b548d 100644
> --- a/config.h
> +++ b/config.h
> @@ -394,6 +394,7 @@ int config_error_nonbool(const char *);
>  #endif
>  
>  char *git_system_config(void);
> +char *git_global_config(void);
>  void git_global_config_paths(char **user, char **xdg);
>  
>  int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
> -- 
> 2.42.0.2.g879ad04204
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 4/4] maintenance: use XDG config if it exists

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4194 bytes --]

On Wed, Oct 18, 2023 at 10:28:41PM +0200, Kristoffer Haugsbakk wrote:
> 
> `git 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.
> 
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  builtin/gc.c           | 23 +++++------------------
>  t/t7900-maintenance.sh | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 17fc031f63a..7b780f2ab38 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -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);

Don't we have to free `config_file` now?

>  		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 *prefi
>  
>  	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, key, NULL, maintpath,
>  			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
> -		free(user_config);
> -		free(xdg_config);

Same here.

>  		if (rc &&
>  		    (!force || rc == CONFIG_NOTHING_SET))
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 487e326b3fa..a11e6c61520 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -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.

> +	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
> +'
> +

As you also change the code of `git maintenance unregister`, should we
test its behaviour, as well?

Patrick

>  test_expect_success 'maintenance.<task>.enabled' '
>  	git config maintenance.gc.enabled false &&
>  	git config maintenance.commit-graph.enabled true &&
> -- 
> 2.42.0.2.g879ad04204
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 4/4] maintenance: use XDG config if it exists

[Eric Sunshine]

On Mon, Oct 23, 2023 at 5:58 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Wed, Oct 18, 2023 at 10:28:41PM +0200, Kristoffer Haugsbakk wrote:
> > `git 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.
> >
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> > +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" &&
>
> 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 &&
            ...
        )
    '

> > +     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`:

    >"$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
> > +'

Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>

[Taylor Blau]

On Mon, Oct 23, 2023 at 11:58:01AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 18, 2023 at 10:28:40PM +0200, Kristoffer Haugsbakk wrote:
> >
> > Factor out code that retrieves the global config file so that we can use
> > it in `gc.c` as well.
> >
> > Use the old name from the previous commit since this function acts
> > functionally the same as `git_system_config` but for “global”.
> >
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> >  builtin/config.c | 25 ++-----------------------
> >  config.c         | 24 ++++++++++++++++++++++++
> >  config.h         |  1 +
> >  3 files changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 6fff2655816..df06b766fad 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> >  	}
> >
> >  	if (use_global_config) {
> > -		char *user_config, *xdg_config;
> > -
> > -		git_global_config_paths(&user_config, &xdg_config);
> > -		if (!user_config)
> > -			/*
> > -			 * It is unknown if HOME/.gitconfig exists, so
> > -			 * we do not know if we should write to XDG
> > -			 * location; error out even if XDG_CONFIG_HOME
> > -			 * is set and points at a sane location.
> > -			 */
> > -			die(_("$HOME not set"));
> > -
> > +		given_config_source.file = git_global_config();
> >  		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
> > -
> > -		if (access_or_warn(user_config, R_OK, 0) &&
> > -		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
> > -			given_config_source.file = xdg_config;
> > -			free(user_config);
> > -		} else {
> > -			given_config_source.file = user_config;
> > -			free(xdg_config);
> > -		}
> > -	}
> > -	else if (use_system_config) {
> > +	} else if (use_system_config) {
> >  		given_config_source.file = git_system_config();
> >  		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
> >  	} else if (use_local_config) {
> > diff --git a/config.c b/config.c
> > index d2cdda96edd..2ff766c56ff 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -2111,6 +2111,30 @@ char *git_system_config(void)
> >  	return system_config;
> >  }
> >
> > +char *git_global_config(void)
> > +{
> > +	char *user_config, *xdg_config;
> > +
> > +	git_global_config_paths(&user_config, &xdg_config);
> > +	if (!user_config)
> > +		/*
> > +		 * It is unknown if HOME/.gitconfig exists, so
> > +		 * we do not know if we should write to XDG
>
> Nit: we don't know about the intent of the caller, so they may not want
> to write to the file but only read it.

I was going to suggest that we allow the caller to pass in the flags
that they wish for git_global_config() to pass down to access(2), but
was surprised to see that we always use R_OK.

But thinking on it for a moment longer, I realized that we don't care
about write-level permissions for the config, since we want to instead
open $GIT_DIR/config.lock for writing, and then rename() it into place,
meaning we only care about whether or not we have write permissions on
$GIT_DIR itself.

I think in the existing location of this code, the "if we should write"
portion of the comment is premature, since we don't know for sure
whether or not we are writing. So I'd be fine with leaving it as-is, but
changing the comment seems easy enough to do...

> > +		 * location; error out even if XDG_CONFIG_HOME
> > +		 * is set and points at a sane location.
> > +		 */
> > +		die(_("$HOME not set"));
>
> Is it sensible to `die()` here in this new function that behaves more
> like a library function? I imagine it would be more sensible to indicate
> the error to the user and let them handle it accordingly.

Agreed.

Thanks,
Taylor

Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>

[Kristoffer Haugsbakk]

Hi Taylor and Patrick

On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:
>> Nit: we don't know about the intent of the caller, so they may not want
>> to write to the file but only read it.
>
> I was going to suggest that we allow the caller to pass in the flags
> that they wish for git_global_config() to pass down to access(2), but
> was surprised to see that we always use R_OK.
>
> But thinking on it for a moment longer, I realized that we don't care
> about write-level permissions for the config, since we want to instead
> open $GIT_DIR/config.lock for writing, and then rename() it into place,
> meaning we only care about whether or not we have write permissions on
> $GIT_DIR itself.
>
> I think in the existing location of this code, the "if we should write"
> portion of the comment is premature, since we don't know for sure
> whether or not we are writing. So I'd be fine with leaving it as-is, but
> changing the comment seems easy enough to do...
>
>> > +		 * location; error out even if XDG_CONFIG_HOME
>> > +		 * is set and points at a sane location.
>> > +		 */
>> > +		die(_("$HOME not set"));
>>
>> Is it sensible to `die()` here in this new function that behaves more
>> like a library function? I imagine it would be more sensible to indicate
>> the error to the user and let them handle it accordingly.
>
> Agreed.
>
> Thanks,
> Taylor

What do you guys think the signature of `git_global_config` should be?

Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

On Tue, Oct 24, 2023 at 03:23:11PM +0200, Kristoffer Haugsbakk wrote:
> Hi Taylor and Patrick
> 
> On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:
> >> Nit: we don't know about the intent of the caller, so they may not want
> >> to write to the file but only read it.
> >
> > I was going to suggest that we allow the caller to pass in the flags
> > that they wish for git_global_config() to pass down to access(2), but
> > was surprised to see that we always use R_OK.
> >
> > But thinking on it for a moment longer, I realized that we don't care
> > about write-level permissions for the config, since we want to instead
> > open $GIT_DIR/config.lock for writing, and then rename() it into place,
> > meaning we only care about whether or not we have write permissions on
> > $GIT_DIR itself.
> >
> > I think in the existing location of this code, the "if we should write"
> > portion of the comment is premature, since we don't know for sure
> > whether or not we are writing. So I'd be fine with leaving it as-is, but
> > changing the comment seems easy enough to do...
> >
> >> > +		 * location; error out even if XDG_CONFIG_HOME
> >> > +		 * is set and points at a sane location.
> >> > +		 */
> >> > +		die(_("$HOME not set"));
> >>
> >> Is it sensible to `die()` here in this new function that behaves more
> >> like a library function? I imagine it would be more sensible to indicate
> >> the error to the user and let them handle it accordingly.
> >
> > Agreed.
> >
> > Thanks,
> > Taylor
> 
> What do you guys think the signature of `git_global_config` should be?

Either of the following:

    - `int git_global_config(char **out_pat)`
    - `char **git_global_config(void)`

In the first case you'd signal error via a non-zero return value,
whereas in the second case you would signal it via a `NULL` return
value.

To decide which one to go with I'd recommend to check whether there is
any similar precedent in "config.h" and what style that precedent uses.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>

[Kristoffer Haugsbakk]

On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:
>> What do you guys think the signature of `git_global_config` should be?
>
> Either of the following:
>
>     - `int git_global_config(char **out_pat)`
>     - `char **git_global_config(void)`
>
> In the first case you'd signal error via a non-zero return value,
> whereas in the second case you would signal it via a `NULL` return
> value.
>
> To decide which one to go with I'd recommend to check whether there is
> any similar precedent in "config.h" and what style that precedent uses.

Okay thanks. So no parameter for determining whether one is writing or
just reading the file.

Cheers

-- 
Kristoffer Haugsbakk

Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

On Wed, Oct 25, 2023 at 09:33:23AM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:
> >> What do you guys think the signature of `git_global_config` should be?
> >
> > Either of the following:
> >
> >     - `int git_global_config(char **out_pat)`
> >     - `char **git_global_config(void)`
> >
> > In the first case you'd signal error via a non-zero return value,
> > whereas in the second case you would signal it via a `NULL` return
> > value.
> >
> > To decide which one to go with I'd recommend to check whether there is
> > any similar precedent in "config.h" and what style that precedent uses.
> 
> Okay thanks. So no parameter for determining whether one is writing or
> just reading the file.

This parameter would only exist for the purpose of the error message,
right? If so, I think that'd be overkill. If we want to have differing
errors depending on how the function is called the best way to handle
that would likely be to generate the error message at the callsite
instead of in the library itself.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> This parameter would only exist for the purpose of the error message,
> right? If so, I think that'd be overkill. If we want to have differing
> errors depending on how the function is called the best way to handle
> that would likely be to generate the error message at the callsite
> instead of in the library itself.

We would need to make sure the lower-level helpers need to be able
to tell what kind of failure they saw (in other words, why they are
failing) to the callers, which may require a bit of designing the
error return convention and plumbing through necessary pieces of
information, but the longer term payoff would be great.

I do not think this is such a case, but if the lower-level needs to
fail differently (e.g., the thing not existing is acceptable when
writing as we will create a new one, but is a fail-worthy error when
reading), then the caller needs to give that down the callchain,
though.

[PATCH v2 0/4] maintenance: use XDG config if it exists

[Kristoffer Haugsbakk]

I 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 <code@khaugsbakk.name>

    +
    + ## 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

[Kristoffer Haugsbakk]

Remove unneeded newlines according to `clang-format`.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

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

diff --git a/builtin/config.c b/builtin/config.c
index 11a4d4ef141..87d0dc92d99 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -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
diff --git a/config.c b/config.c
index 9ff6ae1cb90..d26e16e3ce3 100644
--- a/config.c
+++ b/config.c
@@ -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,
-- 
2.43.0

[PATCH v2 2/4] config: rename global config function

[Kristoffer Haugsbakk]

Rename this function to a more descriptive name since we want to use the
existing name for a new function.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 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(-)

diff --git a/builtin/config.c b/builtin/config.c
index 87d0dc92d99..6fff2655816 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -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
diff --git a/builtin/gc.c b/builtin/gc.c
index 7c11d5ebef0..c078751824c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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 *prefi
 		int 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"));
diff --git a/builtin/var.c b/builtin/var.c
index 8cf7dd9e2e5..cf5567208a2 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -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);
diff --git a/config.c b/config.c
index d26e16e3ce3..ebc6a57e1c3 100644
--- a/config.c
+++ b/config.c
@@ -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,
diff --git a/config.h b/config.h
index 14f881ecfaf..e5e523553cc 100644
--- a/config.h
+++ b/config.h
@@ -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);
 
-- 
2.43.0

[PATCH v2 3/4] config: factor out global config file retrieval

[Kristoffer Haugsbakk]

Factor out code that retrieves the global config file so that we can use
it in `gc.c` as well.

Use the old name from the previous commit since this function acts
functionally the same as `git_system_config` but for “global”.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    • Don’t die; return `NULL`

 builtin/config.c | 25 +++----------------------
 config.c         | 20 ++++++++++++++++++++
 config.h         |  4 ++++
 3 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 6fff2655816..08fe36d4997 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (use_global_config) {
-		char *user_config, *xdg_config;
-
-		git_global_config_paths(&user_config, &xdg_config);
-		if (!user_config)
-			/*
-			 * It is unknown if HOME/.gitconfig exists, so
-			 * we do not know if we should write to XDG
-			 * location; error out even if XDG_CONFIG_HOME
-			 * is set and points at a sane location.
-			 */
+		given_config_source.file = git_global_config();
+		if (!given_config_source.file)
 			die(_("$HOME not set"));
-
 		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-		if (access_or_warn(user_config, R_OK, 0) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
-			given_config_source.file = xdg_config;
-			free(user_config);
-		} else {
-			given_config_source.file = user_config;
-			free(xdg_config);
-		}
-	}
-	else if (use_system_config) {
+	} else if (use_system_config) {
 		given_config_source.file = git_system_config();
 		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
 	} else if (use_local_config) {
diff --git a/config.c b/config.c
index ebc6a57e1c3..3cfeb3d8bd9 100644
--- a/config.c
+++ b/config.c
@@ -1987,6 +1987,26 @@ char *git_system_config(void)
 	return system_config;
 }
 
+char *git_global_config(void)
+{
+	char *user_config, *xdg_config;
+
+	git_global_config_paths(&user_config, &xdg_config);
+	if (!user_config) {
+		free(xdg_config);
+		return NULL;
+	}
+
+	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+	    !access_or_warn(xdg_config, R_OK, 0)) {
+		free(user_config);
+		return xdg_config;
+	} else {
+		free(xdg_config);
+		return user_config;
+	}
+}
+
 void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
diff --git a/config.h b/config.h
index e5e523553cc..625e932b993 100644
--- a/config.h
+++ b/config.h
@@ -382,6 +382,10 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
+/**
+ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
+ */
+char *git_global_config(void);
 void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
-- 
2.43.0

[PATCH v2 4/4] maintenance: use XDG config if it exists

[Kristoffer Haugsbakk]

`git 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.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

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

diff --git a/builtin/gc.c b/builtin/gc.c
index c078751824c..cb80ced6cb5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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 *prefi
 
 	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, 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))
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 00d29871e65..0943dfa18a3 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -67,6 +67,51 @@ 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' '
+	test_when_finished rm -r .config/git/config &&
+	(
+		XDG_CONFIG_HOME=.config &&
+		export XDG_CONFIG_HOME &&
+		mkdir -p $XDG_CONFIG_HOME/git &&
+		>$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 'unregister uses XDG_CONFIG_HOME config if it exists' '
+	test_when_finished rm -r .config/git/config &&
+	(
+		XDG_CONFIG_HOME=.config &&
+		export XDG_CONFIG_HOME &&
+		mkdir -p $XDG_CONFIG_HOME/git &&
+		>$XDG_CONFIG_HOME/git/config &&
+		git maintenance register &&
+		git maintenance unregister &&
+		test_must_fail git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&
+		test_must_be_empty actual
+	)
+'
+
+test_expect_success 'unregister does not need XDG_CONFIG_HOME config to exist' '
+	test_path_is_missing $XDG_CONFIG_HOME/git/config &&
+	git maintenance register &&
+	git maintenance unregister &&
+	test_must_fail git config --global --get maintenance.repo >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'maintenance.<task>.enabled' '
 	git config maintenance.gc.enabled false &&
 	git config maintenance.commit-graph.enabled true &&
-- 
2.43.0

Re: [PATCH v2 3/4] config: factor out global config file retrieval

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

>  	if (use_global_config) {
> -		char *user_config, *xdg_config;
> ...
> -	else if (use_system_config) {
> +	} else if (use_system_config) {
>  		given_config_source.file = git_system_config();
>  		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
>  	} else if (use_local_config) {
> diff --git a/config.c b/config.c
> index ebc6a57e1c3..3cfeb3d8bd9 100644
> --- a/config.c
> +++ b/config.c
> @@ -1987,6 +1987,26 @@ char *git_system_config(void)
>  	return system_config;
>  }
>  
> +char *git_global_config(void)
> +{
> +...
> +}
> +
>  void git_global_config_paths(char **user_out, char **xdg_out)
>  {
>  	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));

The conversion above

> diff --git a/config.h b/config.h
> index e5e523553cc..625e932b993 100644
> --- a/config.h
> +++ b/config.h
> @@ -382,6 +382,10 @@ int config_error_nonbool(const char *);
>  #endif
>  
>  char *git_system_config(void);
> +/**
> + * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
> + */

Sorry, but I am not sure what this comment wants to say.

When $HOME is not set, we do get NULL out of this function.  But
interpolate_path() that makes git_global_config_paths() to return
NULL in user_config does not do any existence check with stat() or
access(), so even when we return a string that is "~/.gitconfig"
expanded to '/home/user/.gitconfig", we are not certain if the file
exists.  So,... it is unclear what "uncertain"ty we are talking
about in this case.

> +char *git_global_config(void);

Re: [PATCH v2 3/4] config: factor out global config file retrieval

[Kristoffer Haugsbakk]

On Tue, Jan 16, 2024, at 22:39, Junio C Hamano wrote:
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>>  char *git_system_config(void);
>> +/**
>> + * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
>> + */
>
> Sorry, but I am not sure what this comment wants to say.
>
> When $HOME is not set, we do get NULL out of this function.  But
> interpolate_path() that makes git_global_config_paths() to return
> NULL in user_config does not do any existence check with stat() or
> access(), so even when we return a string that is "~/.gitconfig"
> expanded to '/home/user/.gitconfig", we are not certain if the file
> exists.  So,... it is unclear what "uncertain"ty we are talking
> about in this case.

I'll delete it. It was an attempt to refer to the comments about
"It is unknown if HOME/.gitconfig exists".

Cheers

-- 
Kristoffer Haugsbakk

Re: [PATCH v2 4/4] maintenance: use XDG config if it exists

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> diff --git a/builtin/gc.c b/builtin/gc.c
> index c078751824c..cb80ced6cb5 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -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);

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.

> @@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  
>  	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, key, NULL, maintpath,
>  			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);

Ditto.

[PATCH v3 0/4] maintenance: use XDG config if it exists

[Kristoffer Haugsbakk]

I 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/c87b3d93-74db-4377-a57c-80f766d46e7f@app.fastmail.com/

§ 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/c87b3d93-74db-4377-a57c-80f766d46e7f@app.fastmail.com/
         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

[Kristoffer Haugsbakk]

Remove unneeded newlines according to `clang-format`.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

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

diff --git a/builtin/config.c b/builtin/config.c
index 11a4d4ef141..87d0dc92d99 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -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
diff --git a/config.c b/config.c
index 9ff6ae1cb90..d26e16e3ce3 100644
--- a/config.c
+++ b/config.c
@@ -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,
-- 
2.43.0

[PATCH v3 2/4] config: rename global config function

[Kristoffer Haugsbakk]

Rename this function to a more descriptive name since we want to use the
existing name for a new function.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 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(-)

diff --git a/builtin/config.c b/builtin/config.c
index 87d0dc92d99..6fff2655816 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -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
diff --git a/builtin/gc.c b/builtin/gc.c
index 7c11d5ebef0..c078751824c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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 *prefi
 		int 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"));
diff --git a/builtin/var.c b/builtin/var.c
index 8cf7dd9e2e5..cf5567208a2 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -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);
diff --git a/config.c b/config.c
index d26e16e3ce3..ebc6a57e1c3 100644
--- a/config.c
+++ b/config.c
@@ -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,
diff --git a/config.h b/config.h
index 14f881ecfaf..e5e523553cc 100644
--- a/config.h
+++ b/config.h
@@ -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);
 
-- 
2.43.0

[PATCH v3 3/4] config: factor out global config file retrieval

[Kristoffer Haugsbakk]

Factor out code that retrieves the global config file so that we can use
it in `gc.c` as well.

Use the old name from the previous commit since this function acts
functionally the same as `git_system_config` but for “global”.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v3:
    • Remove doc on `git_global_config`
    • https://lore.kernel.org/git/c87b3d93-74db-4377-a57c-80f766d46e7f@app.fastmail.com/
    v2:
    • Don’t die; return `NULL`

 builtin/config.c | 25 +++----------------------
 config.c         | 20 ++++++++++++++++++++
 config.h         |  1 +
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 6fff2655816..08fe36d4997 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (use_global_config) {
-		char *user_config, *xdg_config;
-
-		git_global_config_paths(&user_config, &xdg_config);
-		if (!user_config)
-			/*
-			 * It is unknown if HOME/.gitconfig exists, so
-			 * we do not know if we should write to XDG
-			 * location; error out even if XDG_CONFIG_HOME
-			 * is set and points at a sane location.
-			 */
+		given_config_source.file = git_global_config();
+		if (!given_config_source.file)
 			die(_("$HOME not set"));
-
 		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-		if (access_or_warn(user_config, R_OK, 0) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
-			given_config_source.file = xdg_config;
-			free(user_config);
-		} else {
-			given_config_source.file = user_config;
-			free(xdg_config);
-		}
-	}
-	else if (use_system_config) {
+	} else if (use_system_config) {
 		given_config_source.file = git_system_config();
 		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
 	} else if (use_local_config) {
diff --git a/config.c b/config.c
index ebc6a57e1c3..3cfeb3d8bd9 100644
--- a/config.c
+++ b/config.c
@@ -1987,6 +1987,26 @@ char *git_system_config(void)
 	return system_config;
 }
 
+char *git_global_config(void)
+{
+	char *user_config, *xdg_config;
+
+	git_global_config_paths(&user_config, &xdg_config);
+	if (!user_config) {
+		free(xdg_config);
+		return NULL;
+	}
+
+	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+	    !access_or_warn(xdg_config, R_OK, 0)) {
+		free(user_config);
+		return xdg_config;
+	} else {
+		free(xdg_config);
+		return user_config;
+	}
+}
+
 void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
diff --git a/config.h b/config.h
index e5e523553cc..5dba984f770 100644
--- a/config.h
+++ b/config.h
@@ -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);
-- 
2.43.0

[PATCH v3 4/4] maintenance: use XDG config if it exists

[Kristoffer Haugsbakk]

`git 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.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

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

diff --git a/builtin/gc.c b/builtin/gc.c
index c078751824c..cb80ced6cb5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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 *prefi
 
 	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, 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))
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 00d29871e65..0943dfa18a3 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -67,6 +67,51 @@ 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' '
+	test_when_finished rm -r .config/git/config &&
+	(
+		XDG_CONFIG_HOME=.config &&
+		export XDG_CONFIG_HOME &&
+		mkdir -p $XDG_CONFIG_HOME/git &&
+		>$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 'unregister uses XDG_CONFIG_HOME config if it exists' '
+	test_when_finished rm -r .config/git/config &&
+	(
+		XDG_CONFIG_HOME=.config &&
+		export XDG_CONFIG_HOME &&
+		mkdir -p $XDG_CONFIG_HOME/git &&
+		>$XDG_CONFIG_HOME/git/config &&
+		git maintenance register &&
+		git maintenance unregister &&
+		test_must_fail git config --file=$XDG_CONFIG_HOME/git/config --get maintenance.repo >actual &&
+		test_must_be_empty actual
+	)
+'
+
+test_expect_success 'unregister does not need XDG_CONFIG_HOME config to exist' '
+	test_path_is_missing $XDG_CONFIG_HOME/git/config &&
+	git maintenance register &&
+	git maintenance unregister &&
+	test_must_fail git config --global --get maintenance.repo >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'maintenance.<task>.enabled' '
 	git config maintenance.gc.enabled false &&
 	git config maintenance.commit-graph.enabled true &&
-- 
2.43.0

Re: [PATCH v2 3/4] config: factor out global config file retrieval

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

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

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

But second, I think that the new function you introduce here has the
same issue as the old function that you refactored in the preceding
patch: `git_config_global()` isn't very descriptive, and it is also
inconsistent the new `git_config_global_paths()`. I'd propose to name
the new function something like `git_config_global_preferred_path()` or
`git_config_global_path()`.

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

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] config: factor out global config file retrieval

[Kristoffer Haugsbakk]

On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:
> But second, I think that the new function you introduce here has the
> same issue as the old function that you refactored in the preceding
> patch: `git_config_global()` isn't very descriptive, and it is also
> inconsistent the new `git_config_global_paths()`. I'd propose to name
> the new function something like `git_config_global_preferred_path()` or
> `git_config_global_path()`.

The choice of `git_config_global` is mostly motivated by it working the
same way as `git_config_system`:

```
given_config_source.file = git_system_config();
[…]
given_config_source.file = git_global_config();
```

(The extra logic imposed by XDG for “global” is implied by `man git
config`. I don’t know what the guidelines are for spelling that out or not
in the internal functions.)

Your suggestion makes sense. But should `git_system_config` be renamed as
well?

-- 
Kristoffer Haugsbakk

Re: [PATCH v2 3/4] config: factor out global config file retrieval

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

On Fri, Jan 19, 2024 at 08:40:51AM +0100, Kristoffer Haugsbakk wrote:
> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:
> > But second, I think that the new function you introduce here has the
> > same issue as the old function that you refactored in the preceding
> > patch: `git_config_global()` isn't very descriptive, and it is also
> > inconsistent the new `git_config_global_paths()`. I'd propose to name
> > the new function something like `git_config_global_preferred_path()` or
> > `git_config_global_path()`.
> 
> The choice of `git_config_global` is mostly motivated by it working the
> same way as `git_config_system`:
> 
> ```
> given_config_source.file = git_system_config();
> […]
> given_config_source.file = git_global_config();
> ```
> 
> (The extra logic imposed by XDG for “global” is implied by `man git
> config`. I don’t know what the guidelines are for spelling that out or not
> in the internal functions.)
> 
> Your suggestion makes sense. But should `git_system_config` be renamed as
> well?

Yeah, you're right that `git_system_config()` is bad in the same way. In
fact I think it's worse here because we have both `git_config_system()`
and `git_system_config()`, which has certainly confused me multiple
times in the past. So I'd be happy to see it renamed, as well, either
now or in a follow-up patch series.

But as I said, I don't think it's a prereq for this patch series to land
and others may have differing opinions. So please, go ahead as you deem
fit (or wait for other opinions).

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] config: factor out global config file retrieval

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:
>> But second, I think that the new function you introduce here has the
>> same issue as the old function that you refactored in the preceding
>> patch: `git_config_global()` isn't very descriptive, and it is also
>> inconsistent the new `git_config_global_paths()`. I'd propose to name
>> the new function something like `git_config_global_preferred_path()` or
>> `git_config_global_path()`.
>
> The choice of `git_config_global` is mostly motivated by it working the
> same way as `git_config_system`:
>
> ```
> given_config_source.file = git_system_config();
> […]
> given_config_source.file = git_global_config();
> ```

I shared the above understanding with you, so I didn't find the name
"not very descriptive" during my review.  If only we had two more
functions that can replace our uses of repo_git_path(r, "config")
and repo_git_path(r, "config.worktree") [*] in the code, to obtain
the path to the repository local and worktree local configuration
files, the convention may have been more obvious.

    Side note: the worktree specific one is messier; there are code
    paths that use "%s/config.worktree" on gitdir as well---if we
    were to introduce helpers, we should catch and convert them, too.

> Your suggestion makes sense. But should `git_system_config` be renamed as
> well?

I do not mind including "path" in the names of these functions, but
I do agree that such renaming should be done consistently across the
family of functions (which we currently have only two members, but
still).

Thanks.

RE: [PATCH v2 3/4] config: factor out global config file retrieval

[rsbecker]

On Friday, January 19, 2024 1:36 PM, Junio C Hamano wrote:
>"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>
>> On Fri, Jan 19, 2024, at 07:18, Patrick Steinhardt wrote:
>>> But second, I think that the new function you introduce here has the
>>> same issue as the old function that you refactored in the preceding
>>> patch: `git_config_global()` isn't very descriptive, and it is also
>>> inconsistent the new `git_config_global_paths()`. I'd propose to name
>>> the new function something like `git_config_global_preferred_path()`
>>> or `git_config_global_path()`.
>>
>> The choice of `git_config_global` is mostly motivated by it working
>> the same way as `git_config_system`:
>>
>> ```
>> given_config_source.file = git_system_config(); […]
>> given_config_source.file = git_global_config(); ```
>
>I shared the above understanding with you, so I didn't find the name "not very
>descriptive" during my review.  If only we had two more functions that can replace
>our uses of repo_git_path(r, "config") and repo_git_path(r, "config.worktree") [*] in
>the code, to obtain the path to the repository local and worktree local configuration
>files, the convention may have been more obvious.
>
>    Side note: the worktree specific one is messier; there are code
>    paths that use "%s/config.worktree" on gitdir as well---if we
>    were to introduce helpers, we should catch and convert them, too.
>
>> Your suggestion makes sense. But should `git_system_config` be renamed
>> as well?
>
>I do not mind including "path" in the names of these functions, but I do agree that
>such renaming should be done consistently across the family of functions (which
>we currently have only two members, but still).

Is this going to impact the libification effort? I am just curious what other on the team think.
--Randall

Re: [PATCH v2 3/4] config: factor out global config file retrieval

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Yeah, you're right that `git_system_config()` is bad in the same way. In
> fact I think it's worse here because we have both `git_config_system()`
> and `git_system_config()`, which has certainly confused me multiple
> times in the past. So I'd be happy to see it renamed, as well, either
> now or in a follow-up patch series.

OK, let's make a note #leftoverbits here and merge the topic down to
'next'.  By the time it graduates to 'master', we may have a clean-up
patch to rename them.

Thanks, all.