Re: [PATCH v2 2/2] maintenance: add option to register in a specific config

[Derrick Stolee <derrickstolee@github.com>]

On 11/8/2022 2:49 PM, Ronan Pigott wrote:
> maintenance register currently records the maintenance repo exclusively
> within the user's global configuration, but other configuration files
> may be relevant when running maintenance if they are included from the
> global config. This option allows the user to choose where maintenance
> repos are recorded.

> +	Initialize Git config values so any scheduled maintenance will start
> +	running on this repository. This adds the repository to the
> +	`maintenance.repo` config variable in the current user's global config,
> +	or the config specified by --config option, and enables some

This still says --config...

> +	recommended configuration values for `maintenance.<task>.schedule`. The
> +	tasks that are enabled are safe for running in the background without
> +	disrupting foreground processes.

> +	"git maintenance register [--config-file <path>]",

> +	char *config_file = NULL;
>  	struct option options[] = {
> +		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),

>  static char const * const builtin_maintenance_unregister_usage[] = {
> -	"git maintenance unregister [--force]",
> +	"git maintenance unregister [--config-file <path>] [--force]",

> +	char *config_file = NULL;
>  	struct option options[] = {
> +		OPT_STRING(0, "config-file", &config_file, N_("file"), N_("use given config file")),

...but these have been updated. Easy to miss that doc update.

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

This seems like it will attempt to update the given config file
regardless of its contents. This might result in failures when the
file does not have that value, which is different than the typical
case.

I think the right thing to do would be to update the first
"if (!config_file)" block to have an else.


	if (!config_file)
		list = git_config_get_value_multi(key);
	else
		list = <get multi-value from the file>

	if (list) {
		for_each_string_list_item(item, list) {
			...

Then the "if (found)" case can be the same.

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

at least until this part that handles the config write.

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

Please add a case of `git maintenance unregister --config-file` where
the given file does not already have that repo listed to make sure we
cover that case.

Thanks,
-Stolee