> Le 27 mai 2025 à 10:05, Patrick Steinhardt <ps@xxxxxx> a écrit :
>
> 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
> ni the foreground.
>
> This split exists because otherwise it may easily happen that git-gc(1)
> keeps for 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@xxxxxx>
> ---
> builtin/gc.c | 40 +++++++++++++++++++++++++---------------
> 1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 447e5800846..57f3bbf5344 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1545,53 +1545,54 @@ 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,
> },
> };
> @@ -1599,20 +1600,25 @@ static const struct maintenance_task tasks[] = {
> 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,
> + int before)
Perhaps we can use a small enum…
> {
> + 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 +1647,10 @@ 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, 1))
So that is clear what « 1 » (« BEFORE ») does here…
> + 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 +1659,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))
> + if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg, 0))
And « 0 » (« AFTER ») does here?
> result = 1;
>
> rollback_lock_file(&lk);
>
> --
> 2.49.0.1266.g31b7d2e469.dirty
