{"thread":{"id":"63548","subject":"Re: [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach","startedAt":"2025-05-30T13:36:54Z","lastAt":"2025-05-30T14:05:29Z","messageCount":2,"participants":["Ben Knoble","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"519236","messageId":"6E33013D-8336-4BE1-9DBB-CAFF835A49C3@gmail.com","threadId":"63548","inReplyTo":null,"subject":"Re: [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-05-30T12:56:38Z","receivedAt":"2025-05-30T13:36:54Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 27 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> a écrit :\n> \n> Both git-gc(1) and git-maintenance(1) have logic to daemonize so that\n> the maintenance tasks are performed in the background. git-gc(1) has\n> some special logic though to not perform _all_ housekeeping tasks in the\n> background: both references and reflogs are still handled synchronously\n> ni the foreground.\n> \n> This split exists because otherwise it may easily happen that git-gc(1)\n> keeps for the \"packed-refs\" file locked for an extended amount of time,\n> where the next Git command that wants to modify any reference could now\n> fail. This was especially important in the past, where git-gc(1) was\n> still executed directly as part of our automatic maintenance: git-gc(1)\n> was invoked via `git gc --auto --detach`, so we knew to handle most of\n> the maintenance tasks in the background while doing those parts that may\n> cause locking issues in the foreground.\n> \n> We have since moved to git-maintenance(1), which is a more flexible\n> replacement for git-gc(1). By default this command runs git-gc(1), only,\n> but it can be configured to run different tasks, as well. This command\n> does not know about the split between maintenance tasks that should run\n> before and after detach though, and this has led to several bug reports\n> about spurious locking errors for the \"packed-refs\" file.\n> \n> Prepare for a fix by introducing this split for maintenance tasks. Note\n> that this commit does not yet change any of the tasks, so there should\n> not (yet) be a change in behaviour.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> builtin/gc.c | 40 +++++++++++++++++++++++++---------------\n> 1 file changed, 25 insertions(+), 15 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 447e5800846..57f3bbf5344 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1545,53 +1545,54 @@ typedef int (*maintenance_auto_fn)(struct gc_config *cfg);\n> \n> struct maintenance_task {\n>   const char *name;\n> -    maintenance_task_fn fn;\n> +    maintenance_task_fn before_detach;\n> +    maintenance_task_fn after_detach;\n>   maintenance_auto_fn auto_condition;\n> };\n> \n> static const struct maintenance_task tasks[] = {\n>   [TASK_PREFETCH] = {\n>       .name = \"prefetch\",\n> -        .fn = maintenance_task_prefetch,\n> +        .after_detach = maintenance_task_prefetch,\n>   },\n>   [TASK_LOOSE_OBJECTS] = {\n>       .name = \"loose-objects\",\n> -        .fn = maintenance_task_loose_objects,\n> +        .after_detach = maintenance_task_loose_objects,\n>       .auto_condition = loose_object_auto_condition,\n>   },\n>   [TASK_INCREMENTAL_REPACK] = {\n>       .name = \"incremental-repack\",\n> -        .fn = maintenance_task_incremental_repack,\n> +        .after_detach = maintenance_task_incremental_repack,\n>       .auto_condition = incremental_repack_auto_condition,\n>   },\n>   [TASK_GC] = {\n>       .name = \"gc\",\n> -        .fn = maintenance_task_gc,\n> +        .after_detach = maintenance_task_gc,\n>       .auto_condition = need_to_gc,\n>   },\n>   [TASK_COMMIT_GRAPH] = {\n>       .name = \"commit-graph\",\n> -        .fn = maintenance_task_commit_graph,\n> +        .after_detach = maintenance_task_commit_graph,\n>       .auto_condition = should_write_commit_graph,\n>   },\n>   [TASK_PACK_REFS] = {\n>       .name = \"pack-refs\",\n> -        .fn = maintenance_task_pack_refs,\n> +        .after_detach = maintenance_task_pack_refs,\n>       .auto_condition = pack_refs_condition,\n>   },\n>   [TASK_REFLOG_EXPIRE] = {\n>       .name = \"reflog-expire\",\n> -        .fn = maintenance_task_reflog_expire,\n> +        .after_detach = maintenance_task_reflog_expire,\n>       .auto_condition = reflog_expire_condition,\n>   },\n>   [TASK_WORKTREE_PRUNE] = {\n>       .name = \"worktree-prune\",\n> -        .fn = maintenance_task_worktree_prune,\n> +        .after_detach = maintenance_task_worktree_prune,\n>       .auto_condition = worktree_prune_condition,\n>   },\n>   [TASK_RERERE_GC] = {\n>       .name = \"rerere-gc\",\n> -        .fn = maintenance_task_rerere_gc,\n> +        .after_detach = maintenance_task_rerere_gc,\n>       .auto_condition = rerere_gc_condition,\n>   },\n> };\n> @@ -1599,20 +1600,25 @@ static const struct maintenance_task tasks[] = {\n> static int maybe_run_task(const struct maintenance_task *task,\n>             struct repository *repo,\n>             struct maintenance_run_opts *opts,\n> -              struct gc_config *cfg)\n> +              struct gc_config *cfg,\n> +              int before)\n\nPerhaps we can use a small enum…\n\n> {\n> +    maintenance_task_fn fn = before ? task->before_detach : task->after_detach;\n> +    const char *region = before ? \"maintenance before\" : \"maintenance\";\n>   int ret = 0;\n> \n> +    if (!fn)\n> +        return 0;\n>   if (opts->auto_flag &&\n>       (!task->auto_condition || !task->auto_condition(cfg)))\n>       return 0;\n> \n> -    trace2_region_enter(\"maintenance\", task->name, repo);\n> -    if (task->fn(opts, cfg)) {\n> +    trace2_region_enter(region, task->name, repo);\n> +    if (fn(opts, cfg)) {\n>       error(_(\"task '%s' failed\"), task->name);\n>       ret = 1;\n>   }\n> -    trace2_region_leave(\"maintenance\", task->name, repo);\n> +    trace2_region_leave(region, task->name, repo);\n> \n>   return ret;\n> }\n> @@ -1641,6 +1647,10 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n>   }\n>   free(lock_path);\n> \n> +    for (size_t i = 0; i < opts->tasks_nr; i++)\n> +        if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg, 1))\n\nSo that is clear what « 1 » (« BEFORE ») does here…\n\n> +            result = 1;\n> +\n>   /* Failure to daemonize is ok, we'll continue in foreground. */\n>   if (opts->detach > 0) {\n>       trace2_region_enter(\"maintenance\", \"detach\", the_repository);\n> @@ -1649,7 +1659,7 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n>   }\n> \n>   for (size_t i = 0; i < opts->tasks_nr; i++)\n> -        if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg))\n> +        if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg, 0))\n\nAnd « 0 » (« AFTER ») does here?\n\n>           result = 1;\n> \n>   rollback_lock_file(&lk);\n> \n> --\n> 2.49.0.1266.g31b7d2e469.dirty\n"},{"id":"519241","messageId":"aDm7JeVg9vqsq-09@pks.im","threadId":"63548","inReplyTo":"6E33013D-8336-4BE1-9DBB-CAFF835A49C3@gmail.com","subject":"Re: [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-30T14:05:25Z","receivedAt":"2025-05-30T14:05:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 30, 2025 at 08:56:38AM -0400, Ben Knoble wrote:\n> > @@ -1599,20 +1600,25 @@ static const struct maintenance_task tasks[] = {\n> > static int maybe_run_task(const struct maintenance_task *task,\n> >             struct repository *repo,\n> >             struct maintenance_run_opts *opts,\n> > -              struct gc_config *cfg)\n> > +              struct gc_config *cfg,\n> > +              int before)\n> \n> Perhaps we can use a small enum…\n\n> > @@ -1641,6 +1647,10 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n> >   }\n> >   free(lock_path);\n> > \n> > +    for (size_t i = 0; i < opts->tasks_nr; i++)\n> > +        if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg, 1))\n> \n> So that is clear what « 1 » (« BEFORE ») does here…\n> \n> > +            result = 1;\n> > +\n> >   /* Failure to daemonize is ok, we'll continue in foreground. */\n> >   if (opts->detach > 0) {\n> >       trace2_region_enter(\"maintenance\", \"detach\", the_repository);\n> > @@ -1649,7 +1659,7 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n> >   }\n> > \n> >   for (size_t i = 0; i < opts->tasks_nr; i++)\n> > -        if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg))\n> > +        if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg, 0))\n> \n> And « 0 » (« AFTER ») does here?\n\nYeah, I was very close to adding one, but then decided against it to not\nproduce additional churn. But agreed, it would be way easier to read\nthis way, so let me add one.\n\nPatrick\n"}]}