[PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach

[PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach

[Patrick Steinhardt]

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@pks.im>
---
 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)
 {
+	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))
+			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))
 			result = 1;
 
 	rollback_lock_file(&lk);

-- 
2.49.0.1266.g31b7d2e469.dirty

Re: [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach

[Ramsay Jones]

On 27/05/2025 15:04, Patrick Steinhardt wrote:
> 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.

s/ni/in/

> 
> 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,

s/keeps for the/keeps the/

> 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>
[snip]

ATB,
Ramsay Jones

Re: [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach

[Patrick Steinhardt]

On Tue, May 27, 2025 at 06:01:21PM +0100, Ramsay Jones wrote:
> On 27/05/2025 15:04, Patrick Steinhardt wrote:
> > 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.
> 
> s/ni/in/
> 
> > 
> > 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,
> 
> s/keeps for the/keeps the/

And these I've got fixed now, as well. I'll wait a bit for more feedback
though before sending out these fixes.

Thanks!

Patrick

Re: [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach

[Ben Knoble]


> Le 27 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> 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@pks.im>
> ---
> 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

Re: [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach

[Patrick Steinhardt]

On Fri, May 30, 2025 at 08:56:38AM -0400, Ben Knoble wrote:
> > @@ -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…

> > @@ -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?

Yeah, I was very close to adding one, but then decided against it to not
produce additional churn. But agreed, it would be way easier to read
this way, so let me add one.

Patrick