[PATCH v2 08/12] builtin/maintenance: let tasks do maintenance before and after detach
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 30, 2025, 15:08 UTC
- Message-ID
- <20250530-b4-pks-maintenance-ref-lock-race-v2-8-d04e2f93e51f@pks.im>
- In-Reply-To
- <20250530-b4-pks-maintenance-ref-lock-race-v2-0-d04e2f93e51f@pks.im>
Both git-gc(1) and git-maintenance(1) have logic to daemonize so that the maintenance tasks are performed in the background. git-gc(1) has some special logic though to not perform _all_ housekeeping tasks in the background: both references and reflogs are still handled synchronously in the foreground.
This split exists because otherwise it may easily happen that git-gc(1) keeps the "packed-refs" file locked for an extended amount of time, where the next Git command that wants to modify any reference could now fail. This was especially important in the past, where git-gc(1) was still executed directly as part of our automatic maintenance: git-gc(1) was invoked via `git gc --auto --detach`, so we knew to handle most of the maintenance tasks in the background while doing those parts that may cause locking issues in the foreground.
We have since moved to git-maintenance(1), which is a more flexible replacement for git-gc(1). By default this command runs git-gc(1), only, but it can be configured to run different tasks, as well. This command does not know about the split between maintenance tasks that should run before and after detach though, and this has led to several bug reports about spurious locking errors for the "packed-refs" file.
Prepare for a fix by introducing this split for maintenance tasks. Note that this commit does not yet change any of the tasks, so there should not (yet) be a change in behaviour.
Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/gc.c | 48 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c index 447e5800846..f64bae0a825 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1545,74 +1545,86 @@ typedef int (*maintenance_auto_fn)(struct gc_config *cfg); struct maintenance_task { const char *name; - maintenance_task_fn fn; + maintenance_task_fn before_detach; + maintenance_task_fn after_detach; maintenance_auto_fn auto_condition; }; static const struct maintenance_task tasks[] = { [TASK_PREFETCH] = { .name = "prefetch", - .fn = maintenance_task_prefetch, + .after_detach = maintenance_task_prefetch, }, [TASK_LOOSE_OBJECTS] = { .name = "loose-objects", - .fn = maintenance_task_loose_objects, + .after_detach = maintenance_task_loose_objects, .auto_condition = loose_object_auto_condition, }, [TASK_INCREMENTAL_REPACK] = { .name = "incremental-repack", - .fn = maintenance_task_incremental_repack, + .after_detach = maintenance_task_incremental_repack, .auto_condition = incremental_repack_auto_condition, }, [TASK_GC] = { .name = "gc", - .fn = maintenance_task_gc, + .after_detach = maintenance_task_gc, .auto_condition = need_to_gc, }, [TASK_COMMIT_GRAPH] = { .name = "commit-graph", - .fn = maintenance_task_commit_graph, + .after_detach = maintenance_task_commit_graph, .auto_condition = should_write_commit_graph, }, [TASK_PACK_REFS] = { .name = "pack-refs", - .fn = maintenance_task_pack_refs, + .after_detach = maintenance_task_pack_refs, .auto_condition = pack_refs_condition, }, [TASK_REFLOG_EXPIRE] = { .name = "reflog-expire", - .fn = maintenance_task_reflog_expire, + .after_detach = maintenance_task_reflog_expire, .auto_condition = reflog_expire_condition, }, [TASK_WORKTREE_PRUNE] = { .name = "worktree-prune", - .fn = maintenance_task_worktree_prune, + .after_detach = maintenance_task_worktree_prune, .auto_condition = worktree_prune_condition, }, [TASK_RERERE_GC] = { .name = "rerere-gc", - .fn = maintenance_task_rerere_gc, + .after_detach = maintenance_task_rerere_gc, .auto_condition = rerere_gc_condition, }, }; +enum task_phase { + TASK_PHASE_BEFORE_DETACH, + TASK_PHASE_AFTER_DETACH, +}; + static int maybe_run_task(const struct maintenance_task *task, struct repository *repo, struct maintenance_run_opts *opts, - struct gc_config *cfg) + struct gc_config *cfg, + enum task_phase phase) { + int before = (phase == TASK_PHASE_BEFORE_DETACH); + maintenance_task_fn fn = before ? task->before_detach : task->after_detach; + const char *region = before ? "maintenance before" : "maintenance"; int ret = 0; + if (!fn) + return 0; if (opts->auto_flag && (!task->auto_condition || !task->auto_condition(cfg))) return 0; - trace2_region_enter("maintenance", task->name, repo); - if (task->fn(opts, cfg)) { + trace2_region_enter(region, task->name, repo); + if (fn(opts, cfg)) { error(_("task '%s' failed"), task->name); ret = 1; } - trace2_region_leave("maintenance", task->name, repo); + trace2_region_leave(region, task->name, repo); return ret; } @@ -1641,6 +1653,11 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts, } free(lock_path); + for (size_t i = 0; i < opts->tasks_nr; i++) + if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg, + TASK_PHASE_BEFORE_DETACH)) + result = 1; + /* Failure to daemonize is ok, we'll continue in foreground. */ if (opts->detach > 0) { trace2_region_enter("maintenance", "detach", the_repository); @@ -1649,7 +1666,8 @@ 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)) + if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg, + TASK_PHASE_AFTER_DETACH)) result = 1; rollback_lock_file(&lk);
-- 2.50.0.rc0.604.gd4ff7b7c86.dirty