odb: run "pre-auto-gc" hook for all maintenance tasks

The "pre-auto-gc" hook is supposed to run before auto-maintenance
starts. The intent of this is to give users the ability to intercept
running maintenance in case there's for example an event that is not
supposed to run in parallel with repository maintenance.

This hook runs via `need_to_gc()`, which is invoked via two paths:

  - It is called directly by git-gc(1).

  - It is called indirectly by git-maintenance(1) via the "gc" task.

While the former makes sense, the latter is somewhat off. While the hook
is indeed strongly tied to gc'ing a repository, the original intent of
the hook is rather to inhibit any kind of automated garbage collection.
That noticeably also includes all the other maintenance tasks that our
new infrastructure may run, but those aren't getting intercepted at all.
The move towards our new maintenance strategy has thus somewhat neutered
the effectiveness of the hook.

Fix this issue by running the hook before the first auto-maintenance
task that would run as determined by the tasks's auto condition. Note
that this requires us to lift the call to `run_hooks()` out of
`needs_to_gc()`, as the hook would otherwise potentially run multiple
times.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/gc.c b/builtin/gc.c
index d32af422af5e58b395fdd5d030b12cc3c581bbe5..77d0a5c9484375823e38c87deb225760d03d4840 100644 (file)
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -709,8 +709,6 @@ static int need_to_gc(struct gc_config *cfg, struct strvec *repack_args)
        else
                return 0;
 
-       if (run_hooks(the_repository, "pre-auto-gc"))
-               return 0;
        return 1;
 }
 
@@ -933,7 +931,8 @@ int cmd_gc(int argc,
                /*
                 * Auto-gc should be least intrusive as possible.
                 */
-               if (!need_to_gc(&cfg, &repack_args)) {
+               if (!need_to_gc(&cfg, &repack_args) ||
+                   run_hooks(the_repository, "pre-auto-gc")) {
                        ret = 0;
                        goto out;
                }
@@ -1755,11 +1754,18 @@ enum task_phase {
        TASK_PHASE_BACKGROUND,
 };
 
+enum auto_gc_hook_result {
+       AUTO_GC_HOOK_UNDECIDED = 0,
+       AUTO_GC_HOOK_RUN = 1,
+       AUTO_GC_HOOK_SKIP = 2,
+};
+
 static int maybe_run_task(const struct maintenance_task *task,
                          struct repository *repo,
                          struct maintenance_run_opts *opts,
                          struct gc_config *cfg,
-                         enum task_phase phase)
+                         enum task_phase phase,
+                         enum auto_gc_hook_result *auto_gc_hook_result)
 {
        int foreground = (phase == TASK_PHASE_FOREGROUND);
        maintenance_task_fn fn = foreground ? task->foreground : task->background;
@@ -1768,9 +1774,19 @@ static int maybe_run_task(const struct maintenance_task *task,
 
        if (!fn)
                return 0;
-       if (opts->auto_flag &&
-           (!task->auto_condition || !task->auto_condition(cfg)))
-               return 0;
+       if (opts->auto_flag) {
+               if (*auto_gc_hook_result == AUTO_GC_HOOK_SKIP)
+                       return 0;
+
+               if (!task->auto_condition || !task->auto_condition(cfg))
+                       return 0;
+
+               if (*auto_gc_hook_result == AUTO_GC_HOOK_UNDECIDED)
+                       *auto_gc_hook_result = run_hooks(repo, "pre-auto-gc") ?
+                               AUTO_GC_HOOK_SKIP : AUTO_GC_HOOK_RUN;
+               if (*auto_gc_hook_result == AUTO_GC_HOOK_SKIP)
+                       return 0;
+       }
 
        trace2_region_enter(region, task->name, repo);
        if (fn(opts, cfg)) {
@@ -1789,6 +1805,7 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
        struct lock_file lk;
        struct repository *r = the_repository;
        char *lock_path = xstrfmt("%s/maintenance", r->objects->sources->path);
+       enum auto_gc_hook_result auto_gc_hook_result = AUTO_GC_HOOK_UNDECIDED;
 
        if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
                /*
@@ -1808,7 +1825,7 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 
        for (size_t i = 0; i < opts->tasks_nr; i++)
                if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg,
-                                  TASK_PHASE_FOREGROUND))
+                                  TASK_PHASE_FOREGROUND, &auto_gc_hook_result))
                        result = 1;
 
        /* Failure to daemonize is ok, we'll continue in foreground. */
@@ -1820,7 +1837,7 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 
        for (size_t i = 0; i < opts->tasks_nr; i++)
                if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg,
-                                  TASK_PHASE_BACKGROUND))
+                                  TASK_PHASE_BACKGROUND, &auto_gc_hook_result))
                        result = 1;
 
        rollback_lock_file(&lk);

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 129829f1f42f34b9b7b2d3dff06c281223362ab9..2d52e7918a33ff6cce01ecd211a01fcabea87800 100755 (executable)
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -758,6 +758,132 @@ test_expect_success 'geometric repacking honors configured split factor' '
        )
 '
 
+test_expect_success 'pre-auto-gc hook runs exactly once' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+               write_script .git/hooks/pre-auto-gc <<-\EOF &&
+               echo hook >>hook.log
+               EOF
+
+               # Satisfy the auto condition for multiple tasks, both in the
+               # foreground and in the background phase.
+               git config set maintenance.reflog-expire.auto -1 &&
+               git config set maintenance.geometric-repack.auto -1 &&
+               git config set maintenance.rerere-gc.auto -1 &&
+
+               GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+                       git maintenance run --auto 2>/dev/null &&
+
+               # The successful hook does not inhibit any of the tasks...
+               test_maintenance_tasks trace2.txt <<-\EOF &&
+               reflog-expire foreground
+               geometric-repack
+               rerere-gc
+               EOF
+               # ... but it must only have been executed a single time.
+               test_line_count = 1 hook.log
+       )
+'
+
+test_expect_success 'pre-auto-gc hook can inhibit geometric strategy' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+               write_script .git/hooks/pre-auto-gc <<-\EOF &&
+               echo hook >>hook.log
+               exit 1
+               EOF
+
+               git config set maintenance.reflog-expire.auto -1 &&
+               git config set maintenance.geometric-repack.auto -1 &&
+               git config set maintenance.rerere-gc.auto -1 &&
+
+               # Maintenance would be required...
+               git maintenance is-needed --auto &&
+
+               GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+                       git maintenance run --auto 2>/dev/null &&
+
+               # ... but the failing hook inhibits all tasks. The hook itself
+               # is expected to be the only child process being spawned, and
+               # it must only run a single time.
+               test_grep "child_start.*pre-auto-gc" trace2.txt &&
+               test_maintenance_tasks trace2.txt <<-\EOF &&
+               EOF
+               test_line_count = 1 hook.log
+       )
+'
+
+test_expect_success 'pre-auto-gc hook can inhibit gc strategy' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+               write_script .git/hooks/pre-auto-gc <<-\EOF &&
+               echo hook >>hook.log
+               exit 1
+               EOF
+
+               git config set maintenance.strategy gc &&
+               git config set maintenance.auto false &&
+               git config set gc.auto 3 &&
+
+               test_oid_init &&
+
+               # We need to create two objects whose hashes start with 17
+               # since this is what the gc task counts.
+               test_commit "$(test_oid blob17_1)" &&
+               test_commit "$(test_oid blob17_2)" &&
+
+               # Maintenance would be required...
+               git maintenance is-needed --auto &&
+
+               GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+                       git maintenance run --auto 2>/dev/null &&
+
+               # ... but the failing hook inhibits all tasks. The hook itself
+               # is expected to be the only child process being spawned, and
+               # it must only run a single time.
+               test_grep "child_start.*pre-auto-gc" trace2.txt &&
+               test_maintenance_tasks trace2.txt <<-\EOF &&
+               EOF
+               test_subcommand_flex ! git trace2 &&
+               test_line_count = 1 hook.log
+       )
+'
+
+test_expect_success 'pre-auto-gc hook does not run when no maintenance is needed' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+               write_script .git/hooks/pre-auto-gc <<-\EOF &&
+               echo hook >>hook.log
+               EOF
+               test_must_fail git maintenance is-needed --auto &&
+               git maintenance run --auto 2>/dev/null &&
+               test_path_is_missing hook.log
+       )
+'
+
+test_expect_success 'pre-auto-gc hook does not run without --auto' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       test_hook -C repo pre-auto-gc <<-\EOF &&
+       echo hook >>hook.log
+       EOF
+       (
+               cd repo &&
+               GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+                       git maintenance run 2>/dev/null &&
+               test_grep "\[\"git\",\"repack\"," trace2.txt &&
+               test_path_is_missing hook.log
+       )
+'
+
 test_expect_success 'pack-refs task' '
        for n in $(test_seq 1 5)
        do