From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 08 Nov 2022 20:38:51 GMT
Subject: Re: [PATCH v2 2/2] maintenance: add option to register in a specific config
Message-ID: <448cc6ed-c441-85a3-2780-0c07e56f53f8@dunelm.org.uk>
In-Reply-To: <20221108194930.25805-3-ronan@rjp.ie>

Hi Ronan

I think both these patches are useful improvements, thanks for working 
on them. I've left one comment below

On 08/11/2022 19:49, Ronan Pigott wrote:
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 24ea85c7af..1709355bce 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1497,12 +1499,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
>   
>   	if (!found) {
>   		int rc;
> -		char *user_config, *xdg_config;
> -		git_global_config(&user_config, &xdg_config);
> -		if (!user_config)
> -			die(_("$HOME not set"));
> +		char *user_config = NULL, *xdg_config = NULL;
> +
> +		if (!config_file) {
> +			git_global_config(&user_config, &xdg_config);
> +			config_file = user_config;

Here we need to decide whether to use user_config or xdg_config as the 
config file. In builtin/config.c we do this with

		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);
		}

We need something similar here (maybe we should create a helper function 
to find the appropriate file)

> +			if (!user_config)
> +				die(_("$HOME not set"));

This check needs to come before deciding which config file to use

Best Wishes

Phillip

> +		}
>   		rc = git_config_set_multivar_in_file_gently(
> -			user_config, "maintenance.repo", maintpath,
> +			config_file, "maintenance.repo", maintpath,
>   			CONFIG_REGEX_NONE, 0);
>   		free(user_config);
>   		free(xdg_config);
> @@ -1517,14 +1523,16 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
>   }
>   
>   static char const * const builtin_maintenance_unregister_usage[] = {
> -	"git maintenance unregister [--force]",
> +	"git maintenance unregister [--config-file <path>] [--force]",
>   	NULL
>   };
>   
>   static int maintenance_unregister(int argc, const char **argv, const char *prefix)
>   {
>   	int force = 0;
> +	char *config_file = NULL;
>   	struct option options[] = {
> +		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),
>   		OPT__FORCE(&force,
>   			   N_("return success even if repository was not registered"),
>   			   PARSE_OPT_NOCOMPLETE),
> @@ -1542,24 +1550,29 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>   		usage_with_options(builtin_maintenance_unregister_usage,
>   				   options);
>   
> -	list = git_config_get_value_multi(key);
> -	if (list) {
> -		for_each_string_list_item(item, list) {
> -			if (!strcmp(maintpath, item->string)) {
> -				found = 1;
> -				break;
> +	if (!config_file) {
> +		list = git_config_get_value_multi(key);
> +		if (list) {
> +			for_each_string_list_item(item, list) {
> +				if (!strcmp(maintpath, item->string)) {
> +					found = 1;
> +					break;
> +				}
>   			}
>   		}
>   	}
>   
> -	if (found) {
> +	if (found || config_file) {
>   		int rc;
> -		char *user_config, *xdg_config;
> -		git_global_config(&user_config, &xdg_config);
> -		if (!user_config)
> -			die(_("$HOME not set"));
> +		char *user_config = NULL, *xdg_config = NULL;
> +		if (!config_file) {
> +			git_global_config(&user_config, &xdg_config);
> +			config_file = user_config;
> +			if (!user_config)
> +				die(_("$HOME not set"));
> +		}
>   		rc = git_config_set_multivar_in_file_gently(
> -			user_config, key, NULL, maintpath,
> +			config_file, key, NULL, maintpath,
>   			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
>   		free(user_config);
>   		free(xdg_config);
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 96bdd42045..091da683a8 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -500,6 +500,21 @@ test_expect_success 'register and unregister' '
>   	git config --global --get-all maintenance.repo >actual &&
>   	test_cmp before actual &&
>   
> +	git config --file ./other --add maintenance.repo /existing1 &&
> +	git config --file ./other --add maintenance.repo /existing2 &&
> +	git config --file ./other --get-all maintenance.repo >before &&
> +
> +	git maintenance register --config-file ./other &&
> +	test_cmp_config false maintenance.auto &&
> +	git config --file ./other --get-all maintenance.repo >between &&
> +	cp before expect &&
> +	pwd >>expect &&
> +	test_cmp expect between &&
> +
> +	git maintenance unregister --config-file ./other &&
> +	git config --file ./other --get-all maintenance.repo >actual &&
> +	test_cmp before actual &&
> +
>   	test_must_fail git maintenance unregister 2>err &&
>   	grep "is not registered" err &&
>   	git maintenance unregister --force