Skip to contentSkip to conversation

maintenance: use XDG config if it exists

PATCH Superseded Kristoffer Haugsbakk 6 participants 33 messages Oct 18, 2023 – Jan 19, 2024

lore
Share & tools
lore

Kristoffer Haugsbakk

author
#0 lore raw

cover maintenance: use XDG config if it exists

maintenance: use XDG config if it exists

I use the conventional XDG config path for the global configuration. This path is always used except for git maintenance register.

§ Other discussions

While working on this series I found that Phillip Wood[1] had pointed out that xdg_config is never used. However that was on a series where this was the existing behavior (not new), so this wasn't acted upon.

🔗 1: https://lore.kernel.org/git/[email protected]/

§ Patches

• 1–3: Preparatory
• 4: The desired change

§ CC

• Patrick Steinhardt: config changes
• Derrick Stolee: maintenance changes

Kristoffer Haugsbakk (4):
config: format newlines
config: rename global config function
config: factor out global config file retrieval
maintenance: use XDG config if it exists

 builtin/config.c       | 26 ++------------------------
 builtin/gc.c           | 23 +++++------------------
 builtin/var.c          |  2 +-
 config.c               | 30 ++++++++++++++++++++++++++----
 config.h               |  3 ++-
 t/t7900-maintenance.sh | 21 +++++++++++++++++++++
 6 files changed, 57 insertions(+), 48 deletions(-)

--
2.42.0.2.g879ad04204

author
#1 lore raw

PATCH 1/4 config: format newlines

Remove unneeded newlines according to clang-format.

Signed-off-by Kristoffer Haugsbakk <[email protected]>

---

Notes (series):
Honestly the formatter changing these lines over and over again was just annoying. And we're visiting the file anyway.

 builtin/config.c | 1 -
 config.c         | 2 --
 2 files changed, 3 deletions(-)
builtin/config.c +0 -1
index 11a4d4ef141..87d0dc92d99 100644
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		given_config_source.scope = CONFIG_SCOPE_COMMAND;
 	}
 
-
 	if (respect_includes_opt == -1)
 		config_options.respect_includes = !given_config_source.file;
 	else
config.c +0 -2
index 3846a37be97..19f832818f1 100644
@@ -96,7 +96,6 @@ static long config_file_ftell(struct config_source *conf)
 	return ftell(conf->u.file);
 }
 
-
 static int config_buf_fgetc(struct config_source *conf)
 {
 	if (conf->u.buf.pos < conf->u.buf.len)

@@ -3564,7 +3563,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 write_err_out:
 	ret = write_error(get_lock_file_path(&lock));
 	goto out_free;
-
 }
 
 void git_config_set_multivar_in_file(const char *config_filename,
Signature · 1 line
2.42.0.2.g879ad04204
author
#2 lore raw

PATCH 2/4 config: rename global config function

In reply to 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 <[email protected]>
---
 builtin/config.c | 2 +-
 builtin/gc.c     | 4 ++--
 builtin/var.c    | 2 +-
 config.c         | 4 ++--
 config.h         | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)
builtin/config.c +1 -1
index 87d0dc92d99..6fff2655816 100644
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	if (use_global_config) {
 		char *user_config, *xdg_config;
 
-		git_global_config(&user_config, &xdg_config);
+		git_global_config_paths(&user_config, &xdg_config);
 		if (!user_config)
 			/*
 			 * It is unknown if HOME/.gitconfig exists, so
builtin/gc.c +2 -2
index 5c4315f0d81..17fc031f63a 100644
@@ -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"));
builtin/var.c +1 -1
index 74161bdf1c6..8e18b50b1e5 100644
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)
 	char *user, *xdg;
 	size_t unused;
 
-	git_global_config(&user, &xdg);
+	git_global_config_paths(&user, &xdg);
 	if (xdg && *xdg) {
 		normalize_path_copy(xdg, xdg);
 		strbuf_addf(&buf, "%s\n", xdg);
config.c +2 -2
index 19f832818f1..d2cdda96edd 100644
@@ -2111,7 +2111,7 @@ char *git_system_config(void)
 	return system_config;
 }
 
-void git_global_config(char **user_out, char **xdg_out)
+void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
 	char *xdg_config = NULL;

@@ -2186,7 +2186,7 @@ static int do_git_config_sequence(const struct config_options *opts,
 							 data, CONFIG_SCOPE_SYSTEM,
 							 NULL);
 
-	git_global_config(&user_config, &xdg_config);
+	git_global_config_paths(&user_config, &xdg_config);
 
 	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
 		ret += git_config_from_file_with_options(fn, xdg_config, data,
config.h +1 -1
index 6332d749047..9f04de8ee3e 100644
@@ -394,7 +394,7 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
-void git_global_config(char **user, char **xdg);
+void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
Signature · 1 line
2.42.0.2.g879ad04204
author
#3 lore raw

PATCH 3/4 config: factor out global config file retrieval

In reply to 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 <[email protected]>
---
 builtin/config.c | 25 ++-----------------------
 config.c         | 24 ++++++++++++++++++++++++
 config.h         |  1 +
 3 files changed, 27 insertions(+), 23 deletions(-)
builtin/config.c +2 -23
index 6fff2655816..df06b766fad 100644
@@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (use_global_config) {
-		char *user_config, *xdg_config;
-
-		git_global_config_paths(&user_config, &xdg_config);
-		if (!user_config)
-			/*
-			 * It is unknown if HOME/.gitconfig exists, so
-			 * we do not know if we should write to XDG
-			 * location; error out even if XDG_CONFIG_HOME
-			 * is set and points at a sane location.
-			 */
-			die(_("$HOME not set"));
-
+		given_config_source.file = git_global_config();
 		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-		if (access_or_warn(user_config, R_OK, 0) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
-			given_config_source.file = xdg_config;
-			free(user_config);
-		} else {
-			given_config_source.file = user_config;
-			free(xdg_config);
-		}
-	}
-	else if (use_system_config) {
+	} else if (use_system_config) {
 		given_config_source.file = git_system_config();
 		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
 	} else if (use_local_config) {
config.c +24 -0
index d2cdda96edd..2ff766c56ff 100644
@@ -2111,6 +2111,30 @@ char *git_system_config(void)
 	return system_config;
 }
 
+char *git_global_config(void)
+{
+	char *user_config, *xdg_config;
+
+	git_global_config_paths(&user_config, &xdg_config);
+	if (!user_config)
+		/*
+		 * It is unknown if HOME/.gitconfig exists, so
+		 * we do not know if we should write to XDG
+		 * location; error out even if XDG_CONFIG_HOME
+		 * is set and points at a sane location.
+		 */
+		die(_("$HOME not set"));
+
+	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+	    !access_or_warn(xdg_config, R_OK, 0)) {
+		free(user_config);
+		return xdg_config;
+	} else {
+		free(xdg_config);
+		return user_config;
+	}
+}
+
 void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
config.h +1 -0
index 9f04de8ee3e..5cf961b548d 100644
@@ -394,6 +394,7 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
+char *git_global_config(void);
 void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
Signature · 1 line
2.42.0.2.g879ad04204
#4 lore raw

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

Quoted 58 lines · 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 <[email protected]>
---
 builtin/config.c | 25 ++-----------------------
 config.c         | 24 ++++++++++++++++++++++++
 config.h         |  1 +
 3 files changed, 27 insertions(+), 23 deletions(-)
builtin/config.c +2 -23
index 6fff2655816..df06b766fad 100644
@@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (use_global_config) {
-		char *user_config, *xdg_config;
-
-		git_global_config_paths(&user_config, &xdg_config);
-		if (!user_config)
-			/*
-			 * It is unknown if HOME/.gitconfig exists, so
-			 * we do not know if we should write to XDG
-			 * location; error out even if XDG_CONFIG_HOME
-			 * is set and points at a sane location.
-			 */
-			die(_("$HOME not set"));
-
+		given_config_source.file = git_global_config();
 		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-		if (access_or_warn(user_config, R_OK, 0) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
-			given_config_source.file = xdg_config;
-			free(user_config);
-		} else {
-			given_config_source.file = user_config;
-			free(xdg_config);
-		}
-	}
-	else if (use_system_config) {
+	} else if (use_system_config) {
 		given_config_source.file = git_system_config();
 		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
 	} else if (use_local_config) {
config.c +0 -0
index d2cdda96edd..2ff766c56ff 100644
@@ -2111,6 +2111,30 @@ char *git_system_config(void)
 	return system_config;
 }

config.c char *git_system_config(void) · line 2122

Quoted 10 lines
config.c +9 -0
 
+char *git_global_config(void)
+{
+	char *user_config, *xdg_config;
+
+	git_global_config_paths(&user_config, &xdg_config);
+	if (!user_config)
+		/*
+		 * It is unknown if HOME/.gitconfig exists, so
+		 * we do not know if we should write to XDG

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

config.c char *git_system_config(void) · line 2126

Quoted 4 lines
config.c +4 -0
+		 * location; error out even if XDG_CONFIG_HOME
+		 * is set and points at a sane location.
+		 */
+		die(_("$HOME not set"));

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

Patrick

Quoted 29 lines
config.c +11 -0
+
+	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+	    !access_or_warn(xdg_config, R_OK, 0)) {
+		free(user_config);
+		return xdg_config;
+	} else {
+		free(xdg_config);
+		return user_config;
+	}
+}
+
 void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
config.h +1 -0
index 9f04de8ee3e..5cf961b548d 100644
@@ -394,6 +394,7 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
+char *git_global_config(void);
 void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
Signature · 1 line
2.42.0.2.g879ad04204
#5 lore raw

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

Quoted 60 lines · Patrick Steinhardt
Quoted 59 lines · Kristoffer Haugsbakk

Factor out code that retrieves the global config file so that we can use it in gc.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 <[email protected]>
---
 builtin/config.c | 25 ++-----------------------
 config.c         | 24 ++++++++++++++++++++++++
 config.h         |  1 +
 3 files changed, 27 insertions(+), 23 deletions(-)
builtin/config.c +2 -23
index 6fff2655816..df06b766fad 100644
@@ -708,30 +708,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (use_global_config) {
-		char *user_config, *xdg_config;
-
-		git_global_config_paths(&user_config, &xdg_config);
-		if (!user_config)
-			/*
-			 * It is unknown if HOME/.gitconfig exists, so
-			 * we do not know if we should write to XDG
-			 * location; error out even if XDG_CONFIG_HOME
-			 * is set and points at a sane location.
-			 */
-			die(_("$HOME not set"));
-
+		given_config_source.file = git_global_config();
 		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-		if (access_or_warn(user_config, R_OK, 0) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
-			given_config_source.file = xdg_config;
-			free(user_config);
-		} else {
-			given_config_source.file = user_config;
-			free(xdg_config);
-		}
-	}
-	else if (use_system_config) {
+	} else if (use_system_config) {
 		given_config_source.file = git_system_config();
 		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
 	} else if (use_local_config) {
config.c +0 -0
index d2cdda96edd..2ff766c56ff 100644
@@ -2111,6 +2111,30 @@ char *git_system_config(void)
 	return system_config;
 }
Quoted 12 lines

config.c char *git_system_config(void) · line 2122

Quoted 9 lines
config.c +9 -0
+char *git_global_config(void)
+{
+	char *user_config, *xdg_config;
+
+	git_global_config_paths(&user_config, &xdg_config);
+	if (!user_config)
+		/*
+		 * It is unknown if HOME/.gitconfig exists, so
+		 * we do not know if we should write to XDG

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

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

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

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

Quoted 8 lines

config.c char *git_system_config(void) · line 2126

Quoted 4 lines
config.c +4 -0
+		 * location; error out even if XDG_CONFIG_HOME
+		 * is set and points at a sane location.
+		 */
+		die(_("$HOME not set"));

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

Agreed.

Thanks,
Taylor

author
#6 lore raw

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

Hi Taylor and Patrick

Quoted 27 lines · Taylor Blau
Quoted 2 lines

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

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

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

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

Quoted 8 lines

config.c char *git_system_config(void) · line 2126

Quoted 4 lines
config.c +4 -0
+		 * location; error out even if XDG_CONFIG_HOME
+		 * is set and points at a sane location.
+		 */
+		die(_("$HOME not set"));

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

Quoted 4 lines

Agreed.

Thanks,
Taylor

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

#7 lore raw

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

Quoted 30 lines · Kristoffer Haugsbakk

Hi Taylor and Patrick

Quoted 27 lines · Taylor Blau
Quoted 2 lines

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

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

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

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

Quoted 8 lines

config.c char *git_system_config(void) · line 2126

Quoted 4 lines
config.c +4 -0
+		 * location; error out even if XDG_CONFIG_HOME
+		 * is set and points at a sane location.
+		 */
+		die(_("$HOME not set"));

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

Quoted 6 lines
Quoted 4 lines

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

author
#8 lore raw

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

Quoted 7 lines · Patrick Steinhardt
Quoted 1 line

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

Either of the following:

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

Quoted 6 lines

In the first case you'd signal error via a non-zero return value, whereas in the second case you would signal it via a 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

Signature · 1 line
Kristoffer Haugsbakk
#9 lore raw

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

Quoted 12 lines · Kristoffer Haugsbakk
Quoted 11 lines · Patrick Steinhardt
Quoted 1 line

What do you guys think the signature of git_global_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.

Quoted 5 lines
Quoted 2 lines

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

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

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

Patrick

#10 lore raw

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

Quoted 5 lines · Patrick Steinhardt

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

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

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

author
#11 lore raw

PATCH 4/4 maintenance: use XDG config if it exists

In reply to 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 <[email protected]>
---
 builtin/gc.c           | 23 +++++------------------
 t/t7900-maintenance.sh | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 18 deletions(-)
builtin/gc.c +5 -18
index 17fc031f63a..7b780f2ab38 100644
@@ -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))
t/t7900-maintenance.sh +21 -0
index 487e326b3fa..a11e6c61520 100755
@@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' '
 	test_subcommand ! git maintenance run --auto --quiet  <false
 '
 
+test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
+	XDG_CONFIG_HOME=.config &&
+	test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config &&
+	export "XDG_CONFIG_HOME" &&
+	mkdir -p "$XDG_CONFIG_HOME"/git &&
+	touch "$XDG_CONFIG_HOME"/git/config &&
+	git maintenance register &&
+	git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual &&
+	pwd >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '
+	test_when_finished git maintenance unregister &&
+	test_path_is_missing "$XDG_CONFIG_HOME"/git/config &&
+	git maintenance register &&
+	git config --global --get maintenance.repo >actual &&
+	pwd >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'maintenance.<task>.enabled' '
 	git config maintenance.gc.enabled false &&
 	git config maintenance.commit-graph.enabled true &&
Signature · 1 line
2.42.0.2.g879ad04204
#12 lore raw
Quoted 32 lines · 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 <[email protected]>
---
 builtin/gc.c           | 23 +++++------------------
 t/t7900-maintenance.sh | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 18 deletions(-)
builtin/gc.c +0 -4
index 17fc031f63a..7b780f2ab38 100644
@@ -1526,19 +1526,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 
 	if (!found) {
 		int rc;
-		char *user_config = NULL, *xdg_config = NULL;
 
-		if (!config_file) {
-			git_global_config_paths(&user_config, &xdg_config);
-			config_file = user_config;

builtin/gc.c static int maintenance_register(int argc, const char **argv, const char *prefix) · line 1541

Quoted 10 lines
builtin/gc.c +2 -5
-			if (!user_config)
-				die(_("$HOME not set"));
-		}
+		if (!config_file)
+			config_file = git_global_config();
 		rc = git_config_set_multivar_in_file_gently(
 			config_file, "maintenance.repo", maintpath,
 			CONFIG_REGEX_NONE, 0);
-		free(user_config);
-		free(xdg_config);

Don't we have to free config_file now?

Quoted 11 lines
builtin/gc.c +0 -5
 		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)

builtin/gc.c static int maintenance_unregister(int argc, const char **argv, const char *prefi · line 1609

Quoted 10 lines
builtin/gc.c +3 -4
-				die(_("$HOME not set"));
-		}
+
+		if (!config_file)
+			config_file = git_global_config();
 		rc = git_config_set_multivar_in_file_gently(
 			config_file, key, NULL, maintpath,
 			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
-		free(user_config);
-		free(xdg_config);

Same here.

Quoted 7 lines
builtin/gc.c +0 -0
 		if (rc &&
 		    (!force || rc == CONFIG_NOTHING_SET))
t/t7900-maintenance.sh +0 -0
index 487e326b3fa..a11e6c61520 100755

t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 73

Quoted 7 lines
t/t7900-maintenance.sh +4 -0
@@ -67,6 +67,27 @@ test_expect_success 'maintenance.auto config option' '
 	test_subcommand ! git maintenance run --auto --quiet  <false
 '
 
+test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
+	XDG_CONFIG_HOME=.config &&
+	test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config &&
+	export "XDG_CONFIG_HOME" &&

Style: there is no need to quote here.

Also, I think we need to unset this variable at the end of this test as tests don't run in a subshell. In theory, we should also be able to leave this variable unset completely as it will be derived from HOME automatically anyway.

Quoted 7 lines
t/t7900-maintenance.sh +7 -0
+	mkdir -p "$XDG_CONFIG_HOME"/git &&
+	touch "$XDG_CONFIG_HOME"/git/config &&
+	git maintenance register &&
+	git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual &&
+	pwd >expect &&
+	test_cmp expect actual
+'

t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 90

Quoted 10 lines
t/t7900-maintenance.sh +10 -0
+
+test_expect_success 'register does not need XDG_CONFIG_HOME config to exist' '
+	test_when_finished git maintenance unregister &&
+	test_path_is_missing "$XDG_CONFIG_HOME"/git/config &&
+	git maintenance register &&
+	git config --global --get maintenance.repo >actual &&
+	pwd >expect &&
+	test_cmp expect actual
+'
+

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

Patrick

Quoted 6 lines
t/t7900-maintenance.sh +0 -0
 test_expect_success 'maintenance.<task>.enabled' '
 	git config maintenance.gc.enabled false &&
 	git config maintenance.commit-graph.enabled true &&
Signature · 1 line
2.42.0.2.g879ad04204
#13 lore raw
Quoted 20 lines · Patrick Steinhardt
Quoted 14 lines · 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 <[email protected]>

---

t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 73

Quoted 4 lines
t/t7900-maintenance.sh +4 -0
+test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
+     XDG_CONFIG_HOME=.config &&
+     test_when_finished rm -r "$XDG_CONFIG_HOME"/git/config &&
+     export "XDG_CONFIG_HOME" &&
Quoted 2 lines

Also, I think we need to unset this variable at the end of this test as tests don't run in a subshell. [...]

Yup, well spotted. Almost the entire body of this test should be in a subshell to ensure that the environment variable does not live beyond the end of this test. But test_when_finished() can't be used in a subshell, so a little care is needed:

    test_expect_success 'register uses XDG_CONFIG_HOME config if it exists' '
        test_when_finished rm -r .config/git/config &&
        (
            XDG_CONFIG_HOME=.config &&
            ...
        )
    '
Quoted 2 lines

t/t7900-maintenance.sh test_expect_success 'maintenance.auto config option' ' · line 75

Quoted 2 lines
t/t7900-maintenance.sh +2 -0
+     mkdir -p "$XDG_CONFIG_HOME"/git &&
+     touch "$XDG_CONFIG_HOME"/git/config &&

If the timestamp of the file is not significant, then we use > to create it rather than touch:

Quoted 7 lines

"$XDG_CONFIG_HOME"/git/config &&

Quoted 5 lines
t/t7900-maintenance.sh +5 -0
+     git maintenance register &&
+     git config --file="$XDG_CONFIG_HOME"/git/config --get maintenance.repo >actual &&
+     pwd >expect &&
+     test_cmp expect actual
+'
author
#14 lore raw

v2 cover maintenance: use XDG config if it exists

In reply to 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 <[email protected]>

    +
    + ## Notes (series) ##
    +    v2:
    +    • Don’t die; return `NULL`
    +
      ## builtin/config.c ##
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
      	}
    @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
     -			 * location; error out even if XDG_CONFIG_HOME
     -			 * is set and points at a sane location.
     -			 */
    --			die(_("$HOME not set"));
    --
     +		given_config_source.file = git_global_config();
    ++		if (!given_config_source.file)
    + 			die(_("$HOME not set"));
    +-
      		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
     -
     -		if (access_or_warn(user_config, R_OK, 0) &&
    @@ config.c: char *git_system_config(void)
     +	char *user_config, *xdg_config;
     +
     +	git_global_config_paths(&user_config, &xdg_config);
    -+	if (!user_config)
    -+		/*
    -+		 * It is unknown if HOME/.gitconfig exists, so
    -+		 * we do not know if we should write to XDG
    -+		 * location; error out even if XDG_CONFIG_HOME
    -+		 * is set and points at a sane location.
    -+		 */
    -+		die(_("$HOME not set"));
    ++	if (!user_config) {
    ++		free(xdg_config);
    ++		return NULL;
    ++	}
     +
     +	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
     +	    !access_or_warn(xdg_config, R_OK, 0)) {
    @@ config.h: int config_error_nonbool(const char *);
      #endif

      char *git_system_config(void);
    ++/**
    ++ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
    ++ */
     +char *git_global_config(void);
      void git_global_config_paths(char **user, char **xdg);
4:  1e2376a4b99 < -:  ----------- maintenance: use XDG config if it exists
-:  ----------- > 4:  8bd67c5bf01 maintenance: use XDG config if it exists

--
2.43.0

author
#15 lore raw

PATCH v2 1/4 config: format newlines

Remove unneeded newlines according to clang-format.

Signed-off-by Kristoffer Haugsbakk <[email protected]>

---

Notes (series):
Honestly the formatter changing these lines over and over again was just annoying. And we're visiting the file anyway.

 builtin/config.c | 1 -
 config.c         | 2 --
 2 files changed, 3 deletions(-)
builtin/config.c +0 -1
index 11a4d4ef141..87d0dc92d99 100644
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		given_config_source.scope = CONFIG_SCOPE_COMMAND;
 	}
 
-
 	if (respect_includes_opt == -1)
 		config_options.respect_includes = !given_config_source.file;
 	else
config.c +0 -2
index 9ff6ae1cb90..d26e16e3ce3 100644
@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)
 	return ftell(conf->u.file);
 }
 
-
 static int config_buf_fgetc(struct config_source *conf)
 {
 	if (conf->u.buf.pos < conf->u.buf.len)

@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 write_err_out:
 	ret = write_error(get_lock_file_path(&lock));
 	goto out_free;
-
 }
 
 void git_config_set_multivar_in_file(const char *config_filename,
Signature · 1 line
2.43.0
author
#16 lore raw

PATCH v2 2/4 config: rename global config function

In reply to 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 <[email protected]>
---
 builtin/config.c | 2 +-
 builtin/gc.c     | 4 ++--
 builtin/var.c    | 2 +-
 config.c         | 4 ++--
 config.h         | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)
builtin/config.c +1 -1
index 87d0dc92d99..6fff2655816 100644
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	if (use_global_config) {
 		char *user_config, *xdg_config;
 
-		git_global_config(&user_config, &xdg_config);
+		git_global_config_paths(&user_config, &xdg_config);
 		if (!user_config)
 			/*
 			 * It is unknown if HOME/.gitconfig exists, so
builtin/gc.c +2 -2
index 7c11d5ebef0..c078751824c 100644
@@ -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"));
builtin/var.c +1 -1
index 8cf7dd9e2e5..cf5567208a2 100644
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)
 	char *user, *xdg;
 	size_t unused;
 
-	git_global_config(&user, &xdg);
+	git_global_config_paths(&user, &xdg);
 	if (xdg && *xdg) {
 		normalize_path_copy(xdg, xdg);
 		strbuf_addf(&buf, "%s\n", xdg);
config.c +2 -2
index d26e16e3ce3..ebc6a57e1c3 100644
@@ -1987,7 +1987,7 @@ char *git_system_config(void)
 	return system_config;
 }
 
-void git_global_config(char **user_out, char **xdg_out)
+void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
 	char *xdg_config = NULL;

@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts,
 							 data, CONFIG_SCOPE_SYSTEM,
 							 NULL);
 
-	git_global_config(&user_config, &xdg_config);
+	git_global_config_paths(&user_config, &xdg_config);
 
 	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
 		ret += git_config_from_file_with_options(fn, xdg_config, data,
config.h +1 -1
index 14f881ecfaf..e5e523553cc 100644
@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
-void git_global_config(char **user, char **xdg);
+void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
Signature · 1 line
2.43.0
author
#17 lore raw

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

In reply to 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 <[email protected]>

---

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

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

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

Quoted 14 lines · Kristoffer Haugsbakk
builtin/config.c +1 -2
 	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) {
config.c +0 -0
index ebc6a57e1c3..3cfeb3d8bd9 100644
@@ -1987,6 +1987,26 @@ char *git_system_config(void)
 	return system_config;

config.c char *git_system_config(void) · line 1997

Quoted 10 lines
config.c +5 -0
 }
 
+char *git_global_config(void)
+{
+...
+}
+
 void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));

The conversion above

config.h int config_error_nonbool(const char *); · line 387

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

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

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

Quoted 1 line
config.h +1 -0
+char *git_global_config(void);
author
#19 lore raw

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

Quoted 8 lines · Junio C Hamano
Quoted 4 lines · Kristoffer Haugsbakk

char *git_system_config(void);
+/**
+ * Returns NULL if is uncertain whether or not HOME/.gitconfig exists.
+ */

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

Quoted 7 lines

When $HOME is not set, we do get NULL out of this function. But interpolate_path() that 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

Signature · 1 line
Kristoffer Haugsbakk
#20 lore raw

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

In reply to Kristoffer Haugsbakk
Quoted 5 lines · 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”.

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

author
#21 lore raw

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

Quoted 6 lines · Patrick Steinhardt

But second, I think that the new function you introduce here has the same issue as the old function that you refactored in the preceding
patch: git_config_global() isn't very descriptive, and it is also inconsistent the 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?

Signature · 1 line
Kristoffer Haugsbakk
#22 lore raw

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

Quoted 17 lines · Kristoffer Haugsbakk
Quoted 6 lines · Patrick Steinhardt

But second, I think that the new function you introduce here has the same issue as the old function that you refactored in the preceding
patch: git_config_global() isn't very descriptive, and it is also inconsistent the 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();
```
Quoted 6 lines

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

Your suggestion makes sense. But should git_system_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

#23 lore raw

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

Quoted 5 lines · Patrick Steinhardt

Yeah, you're right that git_system_config() is bad in the same way. In fact I think it's worse here because we have 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.

#24 lore raw

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

In reply to Kristoffer Haugsbakk
Quoted 8 lines · Kristoffer Haugsbakk
Quoted 6 lines · Patrick Steinhardt

But second, I think that the new function you introduce here has the same issue as the old function that you refactored in the preceding
patch: git_config_global() isn't very descriptive, and it is also inconsistent the 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().

Quoted 8 lines

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.

Quoted 2 lines

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.

#25 lore raw

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

Quoted 27 lines · Junio C Hamano
Quoted 8 lines · Kristoffer Haugsbakk
Quoted 6 lines · Patrick Steinhardt

But second, I think that the new function you introduce here has the same issue as the old function that you refactored in the preceding
patch: git_config_global() isn't very descriptive, and it is also inconsistent the 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().

Quoted 6 lines

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.

Quoted 6 lines
Quoted 2 lines

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

author
#26 lore raw

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

In reply to 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 <[email protected]>

---

Notes (series):
v2:
• Add unregister tests
• Use subshell when exporting an env. variable
• Style in tests
• Free variables properly

 builtin/gc.c           | 27 ++++++++++++-------------
 t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 14 deletions(-)
builtin/gc.c +13 -14
index c078751824c..cb80ced6cb5 100644
@@ -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))
t/t7900-maintenance.sh +45 -0

51 lines not on this page · raw message

Signature · 1 line
2.43.0
#27 lore raw
Quoted 14 lines · Kristoffer Haugsbakk
builtin/gc.c +1 -3
index c078751824c..cb80ced6cb5 100644
@@ -1543,19 +1543,18 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 
 	if (!found) {
 		int rc;
-		char *user_config = NULL, *xdg_config = NULL;
+		char *global_config_file = NULL;
 
 		if (!config_file) {
-			git_global_config_paths(&user_config, &xdg_config);
-			config_file = user_config;

builtin/gc.c static int maintenance_register(int argc, const char **argv, const char *prefix) · line 1556

Quoted 10 lines
builtin/gc.c +4 -2
-			if (!user_config)
-				die(_("$HOME not set"));
+			global_config_file = git_global_config();
+			config_file = global_config_file;
 		}
+		if (!config_file)
+			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
 			config_file, "maintenance.repo", maintpath,
 			CONFIG_REGEX_NONE, 0);

OK. We used to ask for both user and xdg and without using xdg at all, as long as $HOME is set, we used $HOME/.gitconfig even if it did not exist.

What we want to happen is we pick XDG is XDG exists *and* $HOME/.gitconfig does not. And that is exactly what git_global_config() gives us.

Nicely done.

Quoted 10 lines
builtin/gc.c +2 -3
@@ -1612,18 +1611,18 @@ static int maintenance_unregister(int argc, const char **argv, const char *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;

builtin/gc.c static int maintenance_unregister(int argc, const char **argv, const char *prefi · line 1624

Quoted 10 lines
builtin/gc.c +4 -2
-			if (!user_config)
-				die(_("$HOME not set"));
+			global_config_file = git_global_config();
+			config_file = global_config_file;
 		}
+		if (!config_file)
+			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
 			config_file, key, NULL, maintpath,
 			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);

Ditto.

author
#28 lore raw

v3 cover maintenance: use XDG config if it exists

In reply to 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/[email protected]/

§ Patches

• 1–3: Preparatory
• 4: The desired change

§ CC

• Patrick Steinhardt: config changes; v1 feedback
• Derrick Stolee: maintenance changes
• Eric Sunshine: v1 feedback
• Taylor Blau: v1 feedback
• Junio C Hamano: v2 feedback

§ CI

https://github.com/LemmingAvalanche/git/actions/runs/7521230119

Kristoffer Haugsbakk (4):
config: format newlines
config: rename global config function
config: factor out global config file retrieval
maintenance: use XDG config if it exists

 builtin/config.c       | 26 +++---------------------
 builtin/gc.c           | 27 ++++++++++++-------------
 builtin/var.c          |  2 +-
 config.c               | 26 ++++++++++++++++++++----
 config.h               |  3 ++-
 t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 86 insertions(+), 43 deletions(-)

Range-diff against v2:

1:  d5f6c8d62ec = 1:  1c92b772ef4 config: format newlines
2:  cbc5fde0094 = 2:  269490794bc config: rename global config function
3:  32e5ec7d866 ! 3:  0643a85892c config: factor out global config file retrieval
    @@ Commit message


      ## Notes (series) ##
    +    v3:
    +    • Remove doc on `git_global_config`
    +    • https://lore.kernel.org/git/[email protected]/
         v2:
         • Don’t die; return `NULL`

    @@ config.h: int config_error_nonbool(const char *);
      #endif

      char *git_system_config(void);
    -+/**
    -+ * Returns `NULL` if is uncertain whether or not `HOME/.gitconfig` exists.
    -+ */
     +char *git_global_config(void);
      void git_global_config_paths(char **user, char **xdg);

4: 8bd67c5bf01 = 4: e0880af0a31 maintenance: use XDG config if it exists --
2.43.0

author
#29 lore raw

PATCH v3 1/4 config: format newlines

Remove unneeded newlines according to clang-format.

Signed-off-by Kristoffer Haugsbakk <[email protected]>

---

Notes (series):
Honestly the formatter changing these lines over and over again was just annoying. And we're visiting the file anyway.

 builtin/config.c | 1 -
 config.c         | 2 --
 2 files changed, 3 deletions(-)
builtin/config.c +0 -1
index 11a4d4ef141..87d0dc92d99 100644
@@ -760,7 +760,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		given_config_source.scope = CONFIG_SCOPE_COMMAND;
 	}
 
-
 	if (respect_includes_opt == -1)
 		config_options.respect_includes = !given_config_source.file;
 	else
config.c +0 -2
index 9ff6ae1cb90..d26e16e3ce3 100644
@@ -95,7 +95,6 @@ static long config_file_ftell(struct config_source *conf)
 	return ftell(conf->u.file);
 }
 
-
 static int config_buf_fgetc(struct config_source *conf)
 {
 	if (conf->u.buf.pos < conf->u.buf.len)

@@ -3418,7 +3417,6 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 write_err_out:
 	ret = write_error(get_lock_file_path(&lock));
 	goto out_free;
-
 }
 
 void git_config_set_multivar_in_file(const char *config_filename,
Signature · 1 line
2.43.0
author
#30 lore raw

PATCH v3 2/4 config: rename global config function

In reply to 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 <[email protected]>
---
 builtin/config.c | 2 +-
 builtin/gc.c     | 4 ++--
 builtin/var.c    | 2 +-
 config.c         | 4 ++--
 config.h         | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)
builtin/config.c +1 -1
index 87d0dc92d99..6fff2655816 100644
@@ -710,7 +710,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	if (use_global_config) {
 		char *user_config, *xdg_config;
 
-		git_global_config(&user_config, &xdg_config);
+		git_global_config_paths(&user_config, &xdg_config);
 		if (!user_config)
 			/*
 			 * It is unknown if HOME/.gitconfig exists, so
builtin/gc.c +2 -2
index 7c11d5ebef0..c078751824c 100644
@@ -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"));
builtin/var.c +1 -1
index 8cf7dd9e2e5..cf5567208a2 100644
@@ -90,7 +90,7 @@ static char *git_config_val_global(int ident_flag UNUSED)
 	char *user, *xdg;
 	size_t unused;
 
-	git_global_config(&user, &xdg);
+	git_global_config_paths(&user, &xdg);
 	if (xdg && *xdg) {
 		normalize_path_copy(xdg, xdg);
 		strbuf_addf(&buf, "%s\n", xdg);
config.c +2 -2
index d26e16e3ce3..ebc6a57e1c3 100644
@@ -1987,7 +1987,7 @@ char *git_system_config(void)
 	return system_config;
 }
 
-void git_global_config(char **user_out, char **xdg_out)
+void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
 	char *xdg_config = NULL;

@@ -2040,7 +2040,7 @@ static int do_git_config_sequence(const struct config_options *opts,
 							 data, CONFIG_SCOPE_SYSTEM,
 							 NULL);
 
-	git_global_config(&user_config, &xdg_config);
+	git_global_config_paths(&user_config, &xdg_config);
 
 	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))
 		ret += git_config_from_file_with_options(fn, xdg_config, data,
config.h +1 -1
index 14f881ecfaf..e5e523553cc 100644
@@ -382,7 +382,7 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
-void git_global_config(char **user, char **xdg);
+void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
Signature · 1 line
2.43.0
author
#31 lore raw

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

In reply to 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 <[email protected]>

---

Notes (series):
v3:
• Remove doc on git_global_config
• https://lore.kernel.org/git/[email protected]/
v2:
• Don’t die; return NULL

 builtin/config.c | 25 +++----------------------
 config.c         | 20 ++++++++++++++++++++
 config.h         |  1 +
 3 files changed, 24 insertions(+), 22 deletions(-)
builtin/config.c +3 -22
index 6fff2655816..08fe36d4997 100644
@@ -708,30 +708,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (use_global_config) {
-		char *user_config, *xdg_config;
-
-		git_global_config_paths(&user_config, &xdg_config);
-		if (!user_config)
-			/*
-			 * It is unknown if HOME/.gitconfig exists, so
-			 * we do not know if we should write to XDG
-			 * location; error out even if XDG_CONFIG_HOME
-			 * is set and points at a sane location.
-			 */
+		given_config_source.file = git_global_config();
+		if (!given_config_source.file)
 			die(_("$HOME not set"));
-
 		given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
-		if (access_or_warn(user_config, R_OK, 0) &&
-		    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
-			given_config_source.file = xdg_config;
-			free(user_config);
-		} else {
-			given_config_source.file = user_config;
-			free(xdg_config);
-		}
-	}
-	else if (use_system_config) {
+	} else if (use_system_config) {
 		given_config_source.file = git_system_config();
 		given_config_source.scope = CONFIG_SCOPE_SYSTEM;
 	} else if (use_local_config) {
config.c +20 -0
index ebc6a57e1c3..3cfeb3d8bd9 100644
@@ -1987,6 +1987,26 @@ char *git_system_config(void)
 	return system_config;
 }
 
+char *git_global_config(void)
+{
+	char *user_config, *xdg_config;
+
+	git_global_config_paths(&user_config, &xdg_config);
+	if (!user_config) {
+		free(xdg_config);
+		return NULL;
+	}
+
+	if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+	    !access_or_warn(xdg_config, R_OK, 0)) {
+		free(user_config);
+		return xdg_config;
+	} else {
+		free(xdg_config);
+		return user_config;
+	}
+}
+
 void git_global_config_paths(char **user_out, char **xdg_out)
 {
 	char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
config.h +1 -0
index e5e523553cc..5dba984f770 100644
@@ -382,6 +382,7 @@ int config_error_nonbool(const char *);
 #endif
 
 char *git_system_config(void);
+char *git_global_config(void);
 void git_global_config_paths(char **user, char **xdg);
 
 int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
Signature · 1 line
2.43.0
author
#32 lore raw

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

In reply to 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 <[email protected]>

---

Notes (series):
v2:
• Add unregister tests
• Use subshell when exporting an env. variable
• Style in tests
• Free variables properly

 builtin/gc.c           | 27 ++++++++++++-------------
 t/t7900-maintenance.sh | 45 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 14 deletions(-)
builtin/gc.c +13 -14
index c078751824c..cb80ced6cb5 100644
@@ -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))
t/t7900-maintenance.sh +45 -0

51 lines not on this page · raw message

Signature · 1 line
2.43.0