Re: [PATCH v2 8/8] maintenance: update schedule before config
From: Phillip Wood <hidden>
Date: 2023-08-14 11:29:51
Hi Stolee On 10/08/2023 21:39, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee <redacted> 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
quoted hunk ↗ jump to hunk
Reported-by: Phillip Wood <redacted> Signed-off-by: Derrick Stolee <redacted> --- 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