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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]