On Fri, May 30, 2025 at 08:56:38AM -0400, Ben Knoble wrote:

quoted

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

quoted

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

quoted

+            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