From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 14 Aug 2023 11:28:47 GMT
Subject: Re: [PATCH v2 8/8] maintenance: update schedule before config
Message-ID: <fee3baa3-f962-45be-9b41-4bb17a5d1475@gmail.com>
In-Reply-To: <f0c0f6eff883c62f6b07223b5f1da3fd8e462507.1691699987.git.gitgitgadget@gmail.com>

Hi Stolee

On 10/08/2023 21:39, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> When running 'git maintenance start', the current pattern is to
> configure global config settings to enable maintenance on the current
> repository and set 'maintenance.auto' to false and _then_ to set up the
> schedule with the system scheduler.
> 
> This has a problematic error condition: if the scheduler fails to
> initialize, the repository still will not use automatic maintenance due
> to the 'maintenance.auto' setting.
> 
> Fix this gap by swapping the order of operations. If Git fails to
> initialize maintenance, then the config changes should never happen.

The commit message and patch look good to me

Thanks

Phillip

> Reported-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>   builtin/gc.c           |  5 ++++-
>   t/t7900-maintenance.sh | 13 +++++++++++++
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 6f8df366fbe..fe5f871c493 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -2728,9 +2728,12 @@ static int maintenance_start(int argc, const char **argv, const char *prefix)
>   	opts.scheduler = resolve_scheduler(opts.scheduler);
>   	validate_scheduler(opts.scheduler);
>   
> +	if (update_background_schedule(&opts, 1))
> +		die(_("failed to set up maintenance schedule"));
> +
>   	if (maintenance_register(ARRAY_SIZE(register_args)-1, register_args, NULL))
>   		warning(_("failed to add repo to global config"));
> -	return update_background_schedule(&opts, 1);
> +	return 0;
>   }
>   
>   static const char *const builtin_maintenance_stop_usage[] = {
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 9ffe76729e6..e56f5980dc4 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -849,4 +849,17 @@ test_expect_success 'register and unregister bare repo' '
>   	)
>   '
>   
> +test_expect_success 'failed schedule prevents config change' '
> +	git init --bare failcase &&
> +
> +	for scheduler in crontab launchctl schtasks systemctl
> +	do
> +		GIT_TEST_MAINT_SCHEDULER="$scheduler:false" &&
> +		export GIT_TEST_MAINT_SCHEDULER &&
> +		test_must_fail \
> +			git -C failcase maintenance start &&
> +		test_must_fail git -C failcase config maintenance.auto || return 1
> +	done
> +'
> +
>   test_done