[PATCH 00/11] builtin/maintenance: fix ref lock races when detaching

[PATCH 00/11] builtin/maintenance: fix ref lock races when detaching

[Patrick Steinhardt]

Hi,

this patch series fixes races around locking the "packed-refs" file when
auto-maintenance decides to repack it. This issue has been reported e.g.
via [1] and [2].

The root cause is that git-gc(1) used to know to detach _after_ having
repacked references. As such, callers wouldn't continue with their thing
until we have already packed refs, and thus the race does not exist
there. git-maintenance(1) didn't have the same split though, so this
patch series retrofits that logic.

The series is structured as follows:

  - Patches 1 and 2 do some light refactorings.

  - Patches 3 to 5 refactor how we set up the list of tasks to not rely
    on globals anymore. Instead, we now have a single source of truth
    for which tasks exactly will be run.

  - The remaining patches introduce the split of before/after-detach
    tasks and wire them up for "pack-refs", "reflog-expire" and "gc"
    tasks.

Thanks!

Patrick

[1]: <CAJR-fbZ4X1+gN75m2dUvocR6NkowLOZ9F26cjBy8w1qd181OoQ@mail.gmail.com>
[2]: <CANi7bVAkNc+gY1NoXfJuDRjxjZLTgL8Lfn8_ZmWsvLAoiLPkNg@mail.gmail.com>

---
Patrick Steinhardt (11):
      builtin/gc: use designated field initializers for maintenance tasks
      builtin/gc: drop redundant local variable
      builtin/maintenance: centralize configuration of explicit tasks
      builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
      builtin/maintenance: stop modifying global array of tasks
      builtin/maintenance: extract function to run tasks
      builtin/maintenance: fix typedef for function pointers
      builtin/maintenance: let tasks do maintenance before and after detach
      builtin/maintenance: fix locking race when packing refs and reflogs
      builtin/gc: avoid global state in `gc_before_repack()`
      builtin/maintenance: fix locking race when handling "gc" task

 builtin/gc.c           | 386 +++++++++++++++++++++++++++----------------------
 t/t7900-maintenance.sh |  19 ++-
 2 files changed, 229 insertions(+), 176 deletions(-)


---
base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
change-id: 20250527-b4-pks-maintenance-ref-lock-race-11ae5d68e06f

[PATCH 01/11] builtin/gc: use designated field initializers for maintenance tasks

[Patrick Steinhardt]

Convert the array of maintenance tasks to use designated field
initializers. This makes it easier to add more fields to the struct
without having to modify all tasks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 54 +++++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e33ba946e43..54fc7f299a9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1550,49 +1550,49 @@ enum maintenance_task_label {
 
 static struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
-		"prefetch",
-		maintenance_task_prefetch,
+		.name = "prefetch",
+		.fn = maintenance_task_prefetch,
 	},
 	[TASK_LOOSE_OBJECTS] = {
-		"loose-objects",
-		maintenance_task_loose_objects,
-		loose_object_auto_condition,
+		.name = "loose-objects",
+		.fn = maintenance_task_loose_objects,
+		.auto_condition = loose_object_auto_condition,
 	},
 	[TASK_INCREMENTAL_REPACK] = {
-		"incremental-repack",
-		maintenance_task_incremental_repack,
-		incremental_repack_auto_condition,
+		.name = "incremental-repack",
+		.fn = maintenance_task_incremental_repack,
+		.auto_condition = incremental_repack_auto_condition,
 	},
 	[TASK_GC] = {
-		"gc",
-		maintenance_task_gc,
-		need_to_gc,
-		1,
+		.name = "gc",
+		.fn = maintenance_task_gc,
+		.auto_condition = need_to_gc,
+		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
-		"commit-graph",
-		maintenance_task_commit_graph,
-		should_write_commit_graph,
+		.name = "commit-graph",
+		.fn = maintenance_task_commit_graph,
+		.auto_condition = should_write_commit_graph,
 	},
 	[TASK_PACK_REFS] = {
-		"pack-refs",
-		maintenance_task_pack_refs,
-		pack_refs_condition,
+		.name = "pack-refs",
+		.fn = maintenance_task_pack_refs,
+		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
-		"reflog-expire",
-		maintenance_task_reflog_expire,
-		reflog_expire_condition,
+		.name = "reflog-expire",
+		.fn = maintenance_task_reflog_expire,
+		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {
-		"worktree-prune",
-		maintenance_task_worktree_prune,
-		worktree_prune_condition,
+		.name = "worktree-prune",
+		.fn = maintenance_task_worktree_prune,
+		.auto_condition = worktree_prune_condition,
 	},
 	[TASK_RERERE_GC] = {
-		"rerere-gc",
-		maintenance_task_rerere_gc,
-		rerere_gc_condition,
+		.name = "rerere-gc",
+		.fn = maintenance_task_rerere_gc,
+		.auto_condition = rerere_gc_condition,
 	},
 };
 

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 02/11] builtin/gc: drop redundant local variable

[Patrick Steinhardt]

We have two different variables that track the quietness for git-gc(1):

  - The local variable `quiet`, which we wire up.

  - The `quiet` field of `struct maintenance_run_opts`.

This leads to confusion which of these variables should be used and what
the respective effect is.

Simplify this logic by dropping the local variable in favor of the
options field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 54fc7f299a9..7adda8d2d0d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -818,7 +818,6 @@ int cmd_gc(int argc,
 	   struct repository *repo UNUSED)
 {
 	int aggressive = 0;
-	int quiet = 0;
 	int force = 0;
 	const char *name;
 	pid_t pid;
@@ -831,7 +830,7 @@ int cmd_gc(int argc,
 	const char *prune_expire_arg = prune_expire_sentinel;
 	int ret;
 	struct option builtin_gc_options[] = {
-		OPT__QUIET(&quiet, N_("suppress progress reporting")),
+		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		{
 			.type = OPTION_STRING,
 			.long_name = "prune",
@@ -891,7 +890,7 @@ int cmd_gc(int argc,
 		if (cfg.aggressive_window > 0)
 			strvec_pushf(&repack, "--window=%d", cfg.aggressive_window);
 	}
-	if (quiet)
+	if (opts.quiet)
 		strvec_push(&repack, "-q");
 
 	if (opts.auto_flag) {
@@ -906,7 +905,7 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (!quiet) {
+		if (!opts.quiet) {
 			if (opts.detach > 0)
 				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
 			else
@@ -991,7 +990,7 @@ int cmd_gc(int argc,
 			strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL);
 			/* run `git prune` even if using cruft packs */
 			strvec_push(&prune_cmd.args, cfg.prune_expire);
-			if (quiet)
+			if (opts.quiet)
 				strvec_push(&prune_cmd.args, "--no-progress");
 			if (repo_has_promisor_remote(the_repository))
 				strvec_push(&prune_cmd.args,
@@ -1019,7 +1018,7 @@ int cmd_gc(int argc,
 
 	if (the_repository->settings.gc_write_commit_graph == 1)
 		write_commit_graph_reachable(the_repository->objects->odb,
-					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
+					     !opts.quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
 					     NULL);
 
 	if (opts.auto_flag && too_many_loose_objects(&cfg))

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 03/11] builtin/maintenance: centralize configuration of explicit tasks

[Patrick Steinhardt]

Users of git-maintenance(1) can explicitly ask it to run specific tasks
by passing the `--task=` command line option. This option can be passed
multiple times, which causes us to execute tasks in the same order as
the tasks have been provided by the user.

The order in which tasks are run is computed in `task_option_parse()`:
every time we parse such a command line argument, we modify the global
array of tasks by seting the selected index for that specific task.
This has two downsides:

  - We modify global state, which makes it hard to follow the logic.

  - The configuration of tasks is split across multiple different
    functions, so it is not easy to figure out the different factors
    that play a role in selecting tasks.

Refactor the logic so that `task_option_parse()` does not modify global
state anymore. Instead, this function now only collects the list of
configured tasks. The logic to configure ordering of the respective
tasks is then deferred to `initialize_task_config()`.

This refactoring solves the second problem, that the configuration of
tasks is spread across multiple different locations. The first problem,
that we modify global state, will be fixed in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7adda8d2d0d..c4af9b11287 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1690,15 +1690,22 @@ static void initialize_maintenance_strategy(void)
 	}
 }
 
-static void initialize_task_config(int schedule)
+static void initialize_task_config(const struct string_list *selected_tasks,
+				   int schedule)
 {
-	int i;
 	struct strbuf config_name = STRBUF_INIT;
 
+	for (size_t i = 0; i < TASK__COUNT; i++)
+		tasks[i].selected_order = -1;
+	for (size_t i = 0; i < selected_tasks->nr; i++) {
+		struct maintenance_task *task = selected_tasks->items[i].util;
+		task->selected_order = i;
+	}
+
 	if (schedule)
 		initialize_maintenance_strategy();
 
-	for (i = 0; i < TASK__COUNT; i++) {
+	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
 		char *config_str;
 
@@ -1722,33 +1729,28 @@ static void initialize_task_config(int schedule)
 	strbuf_release(&config_name);
 }
 
-static int task_option_parse(const struct option *opt UNUSED,
+static int task_option_parse(const struct option *opt,
 			     const char *arg, int unset)
 {
-	int i, num_selected = 0;
-	struct maintenance_task *task = NULL;
+	struct string_list *selected_tasks = opt->value;
+	size_t i;
 
 	BUG_ON_OPT_NEG(unset);
 
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (tasks[i].selected_order >= 0)
-			num_selected++;
-		if (!strcasecmp(tasks[i].name, arg)) {
-			task = &tasks[i];
-		}
-	}
-
-	if (!task) {
+	for (i = 0; i < TASK__COUNT; i++)
+		if (!strcasecmp(tasks[i].name, arg))
+			break;
+	if (i >= TASK__COUNT) {
 		error(_("'%s' is not a valid task"), arg);
 		return 1;
 	}
 
-	if (task->selected_order >= 0) {
+	if (unsorted_string_list_has_string(selected_tasks, arg)) {
 		error(_("task '%s' cannot be selected multiple times"), arg);
 		return 1;
 	}
 
-	task->selected_order = num_selected + 1;
+	string_list_append(selected_tasks, arg)->util = &tasks[i];
 
 	return 0;
 }
@@ -1756,8 +1758,8 @@ static int task_option_parse(const struct option *opt UNUSED,
 static int maintenance_run(int argc, const char **argv, const char *prefix,
 			   struct repository *repo UNUSED)
 {
-	int i;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
+	struct string_list selected_tasks = STRING_LIST_INIT_DUP;
 	struct gc_config cfg = GC_CONFIG_INIT;
 	struct option builtin_maintenance_run_options[] = {
 		OPT_BOOL(0, "auto", &opts.auto_flag,
@@ -1769,7 +1771,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     maintenance_opt_schedule),
 		OPT_BOOL(0, "quiet", &opts.quiet,
 			 N_("do not report progress or other information over stderr")),
-		OPT_CALLBACK_F(0, "task", NULL, N_("task"),
+		OPT_CALLBACK_F(0, "task", &selected_tasks, N_("task"),
 			N_("run a specific task"),
 			PARSE_OPT_NONEG, task_option_parse),
 		OPT_END()
@@ -1778,9 +1780,6 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 
 	opts.quiet = !isatty(2);
 
-	for (i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_run_options,
 			     builtin_maintenance_run_usage,
@@ -1790,13 +1789,15 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 		die(_("use at most one of --auto and --schedule=<frequency>"));
 
 	gc_config(&cfg);
-	initialize_task_config(opts.schedule);
+	initialize_task_config(&selected_tasks, opts.schedule);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
 				   builtin_maintenance_run_options);
 
 	ret = maintenance_run_tasks(&opts, &cfg);
+
+	string_list_clear(&selected_tasks, 0);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 04/11] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible

[Patrick Steinhardt]

The "--task=" option explicitly allows the user to say which maintenance
tasks should be run, whereas "--schedule=" only respects the maintenance
strategy configured for a specific repository. As such, it is sensible
to accept both options at the same time.

Mark them as incompatible with one another. While at it, also convert
the existing logic that marks "--auto" and "--schedule=" as incompatible
to use `die_for_incompatible_opt2()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 6 ++++--
 t/t7900-maintenance.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c4af9b11287..57d7602596a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1785,8 +1785,10 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     builtin_maintenance_run_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (opts.auto_flag && opts.schedule)
-		die(_("use at most one of --auto and --schedule=<frequency>"));
+	die_for_incompatible_opt2(opts.auto_flag, "--auto",
+				  opts.schedule, "--schedule=");
+	die_for_incompatible_opt2(selected_tasks.nr, "--task=",
+				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
 	initialize_task_config(&selected_tasks, opts.schedule);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 8cf89e285f4..1ada5246606 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -610,7 +610,12 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
 
 test_expect_success '--auto and --schedule incompatible' '
 	test_must_fail git maintenance run --auto --schedule=daily 2>err &&
-	test_grep "at most one" err
+	test_grep "cannot be used together" err
+'
+
+test_expect_success '--task and --schedule incompatible' '
+	test_must_fail git maintenance run --task=pack-refs --schedule=daily 2>err &&
+	test_grep "cannot be used together" err
 '
 
 test_expect_success 'invalid --schedule value' '

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 05/11] builtin/maintenance: stop modifying global array of tasks

[Patrick Steinhardt]

When configuring maintenance tasks run by git-maintenance(1) we do so by
modifying the global array of tasks directly. This is already quite bad
on its own, as global state makes for logic that is hard to follow.

Even more importantly though we use multiple different fields to track
whether or not a task should be run:

  - "enabled" tracks the "maintenance.*.enabled" config key. This field
    disables execution of a task, unless the user has explicitly asked
    for the task.

  - "selected_order" tracks the order in which jobs have been asked for
    by the user via the "--task=" command line option. It overrides
    everything else, but only has an effect if at least one job has been
    selected.

  - "schedule" tracks the schedule priority for a job, that is how often
    it should run. This field only plays a role when the user has passed
    the "--schedule=" command line option.

All of this makes it non-trivial to figure out which job really should
be running right now. The logic to configure these fields and the logic
that interprets them is distributed across multiple functions, making it
even harder to follow it.

Refactor the logic so that we stop modifying global state. Instead, we
now compute which jobs should be run in `initialize_task_config()`,
represented as an array of jobs to run that is stored in the options
structure. Like this, all logic becomes self-contained and any users of
this array only need to iterate through the tasks and execute them one
by one.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 206 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 112 insertions(+), 94 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 57d7602596a..4d636237cac 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -251,7 +251,24 @@ static enum schedule_priority parse_schedule(const char *value)
 	return SCHEDULE_NONE;
 }
 
+enum maintenance_task_label {
+	TASK_PREFETCH,
+	TASK_LOOSE_OBJECTS,
+	TASK_INCREMENTAL_REPACK,
+	TASK_GC,
+	TASK_COMMIT_GRAPH,
+	TASK_PACK_REFS,
+	TASK_REFLOG_EXPIRE,
+	TASK_WORKTREE_PRUNE,
+	TASK_RERERE_GC,
+
+	/* Leave as final value */
+	TASK__COUNT
+};
+
 struct maintenance_run_opts {
+	enum maintenance_task_label *tasks;
+	size_t tasks_nr, tasks_alloc;
 	int auto_flag;
 	int detach;
 	int quiet;
@@ -261,6 +278,11 @@ struct maintenance_run_opts {
 	.detach = -1, \
 }
 
+static void maintenance_run_opts_release(struct maintenance_run_opts *opts)
+{
+	free(opts->tasks);
+}
+
 static int pack_refs_condition(UNUSED struct gc_config *cfg)
 {
 	/*
@@ -1032,6 +1054,7 @@ int cmd_gc(int argc,
 	}
 
 out:
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return 0;
 }
@@ -1524,30 +1547,9 @@ struct maintenance_task {
 	const char *name;
 	maintenance_task_fn *fn;
 	maintenance_auto_fn *auto_condition;
-	unsigned enabled:1;
-
-	enum schedule_priority schedule;
-
-	/* -1 if not selected. */
-	int selected_order;
-};
-
-enum maintenance_task_label {
-	TASK_PREFETCH,
-	TASK_LOOSE_OBJECTS,
-	TASK_INCREMENTAL_REPACK,
-	TASK_GC,
-	TASK_COMMIT_GRAPH,
-	TASK_PACK_REFS,
-	TASK_REFLOG_EXPIRE,
-	TASK_WORKTREE_PRUNE,
-	TASK_RERERE_GC,
-
-	/* Leave as final value */
-	TASK__COUNT
 };
 
-static struct maintenance_task tasks[] = {
+static const struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
 		.name = "prefetch",
 		.fn = maintenance_task_prefetch,
@@ -1566,7 +1568,6 @@ static struct maintenance_task tasks[] = {
 		.name = "gc",
 		.fn = maintenance_task_gc,
 		.auto_condition = need_to_gc,
-		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
 		.name = "commit-graph",
@@ -1595,18 +1596,9 @@ static struct maintenance_task tasks[] = {
 	},
 };
 
-static int compare_tasks_by_selection(const void *a_, const void *b_)
-{
-	const struct maintenance_task *a = a_;
-	const struct maintenance_task *b = b_;
-
-	return b->selected_order - a->selected_order;
-}
-
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
-	int i, found_selected = 0;
 	int result = 0;
 	struct lock_file lk;
 	struct repository *r = the_repository;
@@ -1635,95 +1627,120 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (i = 0; !found_selected && i < TASK__COUNT; i++)
-		found_selected = tasks[i].selected_order >= 0;
-
-	if (found_selected)
-		QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);
-
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (found_selected && tasks[i].selected_order < 0)
-			continue;
-
-		if (!found_selected && !tasks[i].enabled)
-			continue;
-
+	for (size_t i = 0; i < opts->tasks_nr; i++) {
 		if (opts->auto_flag &&
-		    (!tasks[i].auto_condition ||
-		     !tasks[i].auto_condition(cfg)))
-			continue;
-
-		if (opts->schedule && tasks[i].schedule < opts->schedule)
+		    (!tasks[opts->tasks[i]].auto_condition ||
+		     !tasks[opts->tasks[i]].auto_condition(cfg)))
 			continue;
 
-		trace2_region_enter("maintenance", tasks[i].name, r);
-		if (tasks[i].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[i].name);
+		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
+		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
+			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
 			result = 1;
 		}
-		trace2_region_leave("maintenance", tasks[i].name, r);
+		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
 	}
 
 	rollback_lock_file(&lk);
 	return result;
 }
 
-static void initialize_maintenance_strategy(void)
+struct maintenance_strategy {
+	struct {
+		int enabled;
+		enum schedule_priority schedule;
+	} tasks[TASK__COUNT];
+};
+
+static const struct maintenance_strategy none_strategy = { 0 };
+static const struct maintenance_strategy default_strategy = {
+	.tasks = {
+		[TASK_GC].enabled = 1,
+	},
+};
+static const struct maintenance_strategy incremental_strategy = {
+	.tasks = {
+		[TASK_COMMIT_GRAPH].enabled = 1,
+		[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY,
+		[TASK_PREFETCH].enabled = 1,
+		[TASK_PREFETCH].schedule = SCHEDULE_HOURLY,
+		[TASK_INCREMENTAL_REPACK].enabled = 1,
+		[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY,
+		[TASK_LOOSE_OBJECTS].enabled = 1,
+		[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY,
+		[TASK_PACK_REFS].enabled = 1,
+		[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY,
+	},
+};
+
+static void initialize_task_config(struct maintenance_run_opts *opts,
+				   const struct string_list *selected_tasks)
 {
+	struct strbuf config_name = STRBUF_INIT;
+	struct maintenance_strategy strategy;
 	const char *config_str;
 
-	if (git_config_get_string_tmp("maintenance.strategy", &config_str))
-		return;
+	/*
+	 * In case the user has asked us to run tasks explicitly we only use
+	 * those specified tasks. Specifically, we do _not_ want to consult the
+	 * config or maintenance strategy.
+	 */
+	if (selected_tasks->nr) {
+		for (size_t i = 0; i < selected_tasks->nr; i++) {
+			enum maintenance_task_label label = (intptr_t)selected_tasks->items[i].util;;
+			ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+			opts->tasks[opts->tasks_nr++] = label;
+		}
 
-	if (!strcasecmp(config_str, "incremental")) {
-		tasks[TASK_GC].schedule = SCHEDULE_NONE;
-		tasks[TASK_COMMIT_GRAPH].enabled = 1;
-		tasks[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_PREFETCH].enabled = 1;
-		tasks[TASK_PREFETCH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_INCREMENTAL_REPACK].enabled = 1;
-		tasks[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY;
-		tasks[TASK_LOOSE_OBJECTS].enabled = 1;
-		tasks[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY;
-		tasks[TASK_PACK_REFS].enabled = 1;
-		tasks[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY;
+		return;
 	}
-}
 
-static void initialize_task_config(const struct string_list *selected_tasks,
-				   int schedule)
-{
-	struct strbuf config_name = STRBUF_INIT;
+	/*
+	 * Otherwise, the strategy depends on whether we run as part of a
+	 * scheduled job or not:
+	 *
+	 *   - Scheduled maintenance does not perform any housekeeping by
+	 *     default, but requires the user to pick a maintenance strategy.
+	 *
+	 *   - Unscheduled maintenance uses our default strategy.
+	 *
+	 * Both of these are affected by the gitconfig though, which may
+	 * override specific aspects of our strategy.
+	 */
+	if (opts->schedule) {
+		strategy = none_strategy;
 
-	for (size_t i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-	for (size_t i = 0; i < selected_tasks->nr; i++) {
-		struct maintenance_task *task = selected_tasks->items[i].util;
-		task->selected_order = i;
+		if (!git_config_get_string_tmp("maintenance.strategy", &config_str)) {
+			if (!strcasecmp(config_str, "incremental"))
+				strategy = incremental_strategy;
+		}
+	} else {
+		strategy = default_strategy;
 	}
 
-	if (schedule)
-		initialize_maintenance_strategy();
-
 	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
-		char *config_str;
 
 		strbuf_reset(&config_name);
 		strbuf_addf(&config_name, "maintenance.%s.enabled",
 			    tasks[i].name);
-
 		if (!git_config_get_bool(config_name.buf, &config_value))
-			tasks[i].enabled = config_value;
-
-		strbuf_reset(&config_name);
-		strbuf_addf(&config_name, "maintenance.%s.schedule",
-			    tasks[i].name);
+			strategy.tasks[i].enabled = config_value;
+		if (!strategy.tasks[i].enabled)
+			continue;
 
-		if (!git_config_get_string(config_name.buf, &config_str)) {
-			tasks[i].schedule = parse_schedule(config_str);
-			free(config_str);
+		if (opts->schedule) {
+			strbuf_reset(&config_name);
+			strbuf_addf(&config_name, "maintenance.%s.schedule",
+				    tasks[i].name);
+			if (!git_config_get_string_tmp(config_name.buf, &config_str))
+				strategy.tasks[i].schedule = parse_schedule(config_str);
+			if (strategy.tasks[i].schedule < opts->schedule)
+				continue;
 		}
+
+		ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+		opts->tasks[opts->tasks_nr++] = i;
 	}
 
 	strbuf_release(&config_name);
@@ -1750,7 +1767,7 @@ static int task_option_parse(const struct option *opt,
 		return 1;
 	}
 
-	string_list_append(selected_tasks, arg)->util = &tasks[i];
+	string_list_append(selected_tasks, arg)->util = (void *)(intptr_t)i;
 
 	return 0;
 }
@@ -1791,7 +1808,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
-	initialize_task_config(&selected_tasks, opts.schedule);
+	initialize_task_config(&opts, &selected_tasks);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
@@ -1800,6 +1817,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 	ret = maintenance_run_tasks(&opts, &cfg);
 
 	string_list_clear(&selected_tasks, 0);
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 06/11] builtin/maintenance: extract function to run tasks

[Patrick Steinhardt]

Extract the function to run maintenance tasks. This function will be
reused in a subsequent commit where we introduce a split between
maintenance tasks that run before and after daemonizing the process.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4d636237cac..cfbf9d8a2b9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1596,6 +1596,27 @@ 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)
+{
+	int ret = 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)) {
+		error(_("task '%s' failed"), task->name);
+		ret = 1;
+	}
+	trace2_region_leave("maintenance", task->name, repo);
+
+	return ret;
+}
+
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
@@ -1627,19 +1648,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (size_t i = 0; i < opts->tasks_nr; i++) {
-		if (opts->auto_flag &&
-		    (!tasks[opts->tasks[i]].auto_condition ||
-		     !tasks[opts->tasks[i]].auto_condition(cfg)))
-			continue;
-
-		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
-		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
+	for (size_t i = 0; i < opts->tasks_nr; i++)
+		if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg))
 			result = 1;
-		}
-		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
-	}
 
 	rollback_lock_file(&lk);
 	return result;

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 07/11] builtin/maintenance: fix typedef for function pointers

[Patrick Steinhardt]

The typedefs for `maintenance_task_fn` and `maintenance_auto_fn` are
somewhat confusingly not true function pointers. As such, any user of
those typedefs needs to manually add the pointer to make use of them.

Fix this by making these true function pointers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cfbf9d8a2b9..447e5800846 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1533,20 +1533,20 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
 	return 0;
 }
 
-typedef int maintenance_task_fn(struct maintenance_run_opts *opts,
-				struct gc_config *cfg);
+typedef int (*maintenance_task_fn)(struct maintenance_run_opts *opts,
+				   struct gc_config *cfg);
 
 /*
  * An auto condition function returns 1 if the task should run
  * and 0 if the task should NOT run. See needs_to_gc() for an
  * example.
  */
-typedef int maintenance_auto_fn(struct gc_config *cfg);
+typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
 
 struct maintenance_task {
 	const char *name;
-	maintenance_task_fn *fn;
-	maintenance_auto_fn *auto_condition;
+	maintenance_task_fn fn;
+	maintenance_auto_fn auto_condition;
 };
 
 static const struct maintenance_task tasks[] = {

-- 
2.49.0.1266.g31b7d2e469.dirty

[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

[PATCH 09/11] builtin/maintenance: fix locking race when packing refs and reflogs

[Patrick Steinhardt]

As explained in the preceding commit, git-gc(1) knows to detach only
after it has already packed references and reflogs. This is done to
avoid racing around their respective lockfiles.

Adapt git-maintenance(1) accordingly and run the "pack-refs" and
"reflog-expire" tasks before detaching. Note that the "gc" task has the
same issue, but the fix is a bit more involved there and will thus be
done in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 57f3bbf5344..e5d1114bd2d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1577,12 +1577,12 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_PACK_REFS] = {
 		.name = "pack-refs",
-		.after_detach = maintenance_task_pack_refs,
+		.before_detach = maintenance_task_pack_refs,
 		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
 		.name = "reflog-expire",
-		.after_detach = maintenance_task_reflog_expire,
+		.before_detach = maintenance_task_reflog_expire,
 		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`

[Patrick Steinhardt]

The `gc_before_repack()` should only ever run once in git-gc(1), but we
may end up calling it twice when the "--detach" flag is passed. The
duplicated call is avoided though via a static flag in this function.

This pattern is somewhat unintuitive though. Refactor it to drop the
static flag and instead guard the second call of `gc_before_repack()`
via `opts.detach`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e5d1114bd2d..174357b9c25 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,22 +816,14 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static void gc_before_repack(struct maintenance_run_opts *opts,
-			     struct gc_config *cfg)
+static int gc_before_repack(struct maintenance_run_opts *opts,
+			    struct gc_config *cfg)
 {
-	/*
-	 * We may be called twice, as both the pre- and
-	 * post-daemonized phases will call us, but running these
-	 * commands more than once is pointless and wasteful.
-	 */
-	static int done = 0;
-	if (done++)
-		return;
-
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
-		die(FAILED_RUN, "pack-refs");
+		return error(FAILED_RUN, "pack-refs");
 	if (cfg->prune_reflogs && maintenance_task_reflog_expire(opts, cfg))
-		die(FAILED_RUN, "reflog");
+		return error(FAILED_RUN, "reflog");
+	return 0;
 }
 
 int cmd_gc(int argc,
@@ -965,7 +957,8 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		gc_before_repack(&opts, &cfg); /* dies on failure */
+		if (gc_before_repack(&opts, &cfg) < 0)
+			exit(127);
 		delete_tempfile(&pidfile);
 
 		/*
@@ -995,7 +988,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0)
+		gc_before_repack(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;

-- 
2.49.0.1266.g31b7d2e469.dirty

[PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task

[Patrick Steinhardt]

The "gc" task has a similar locking race as the one that we have fixed
for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix
this by splitting up the logic of the "gc" task:

  - Before detaching we execute `gc_before_repack()`, which contains the
    logic that git-gc(1) itself would execute before detaching.

  - After detaching we spawn git-gc(1), but with a new hidden flag that
    suppresses calling `gc_before_repack()`.

Like this we have roughly the same logic as git-gc(1) itself and know to
repack refs and reflogs before detaching, thus fixing the race.

Note that `gc_before_repack()` is renamed to `gc_before_detach()` to
better reflect what this function does.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 39 ++++++++++++++++++++++++++-------------
 t/t7900-maintenance.sh | 12 ++++++------
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 174357b9c25..2cf61efcee9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,7 +816,7 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static int gc_before_repack(struct maintenance_run_opts *opts,
+static int gc_before_detach(struct maintenance_run_opts *opts,
 			    struct gc_config *cfg)
 {
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
@@ -837,6 +837,7 @@ int cmd_gc(int argc,
 	pid_t pid;
 	int daemonized = 0;
 	int keep_largest_pack = -1;
+	int skip_maintenance_before_detach = 0;
 	timestamp_t dummy;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
 	struct gc_config cfg = GC_CONFIG_INIT;
@@ -869,6 +870,8 @@ int cmd_gc(int argc,
 			 N_("repack all other packs except the largest pack")),
 		OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"),
 			   N_("pack prefix to store a pack containing pruned objects")),
+		OPT_HIDDEN_BOOL(0, "skip-maintenance-before-detach", &skip_maintenance_before_detach,
+			   N_("skip maintenance steps typically done before detaching")),
 		OPT_END()
 	};
 
@@ -952,14 +955,16 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (lock_repo_for_gc(force, &pid)) {
-			ret = 0;
-			goto out;
-		}
+		if (!skip_maintenance_before_detach) {
+			if (lock_repo_for_gc(force, &pid)) {
+				ret = 0;
+				goto out;
+			}
 
-		if (gc_before_repack(&opts, &cfg) < 0)
-			exit(127);
-		delete_tempfile(&pidfile);
+			if (gc_before_detach(&opts, &cfg) < 0)
+				exit(127);
+			delete_tempfile(&pidfile);
+		}
 
 		/*
 		 * failure to daemonize is ok, we'll continue
@@ -988,8 +993,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	if (opts.detach <= 0)
-		gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0 && !skip_maintenance_before_detach)
+		gc_before_detach(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;
@@ -1225,8 +1230,14 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,
 	return 0;
 }
 
-static int maintenance_task_gc(struct maintenance_run_opts *opts,
-			       struct gc_config *cfg UNUSED)
+static int maintenance_task_gc_before_detach(struct maintenance_run_opts *opts,
+					     struct gc_config *cfg)
+{
+	return gc_before_detach(opts, cfg);
+}
+
+static int maintenance_task_gc_after_detach(struct maintenance_run_opts *opts,
+					    struct gc_config *cfg UNUSED)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
@@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts,
 	else
 		strvec_push(&child.args, "--no-quiet");
 	strvec_push(&child.args, "--no-detach");
+	strvec_push(&child.args, "--skip-maintenance-before-detach");
 
 	return run_command(&child);
 }
@@ -1561,7 +1573,8 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_GC] = {
 		.name = "gc",
-		.after_detach = maintenance_task_gc,
+		.before_detach = maintenance_task_gc_before_detach,
+		.after_detach = maintenance_task_gc_after_detach,
 		.auto_condition = need_to_gc,
 	},
 	[TASK_COMMIT_GRAPH] = {
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 1ada5246606..e09a36ab021 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -49,9 +49,9 @@ test_expect_success 'run [--auto|--quiet]' '
 		git maintenance run --auto 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \
 		git maintenance run --no-quiet 2>/dev/null &&
-	test_subcommand git gc --quiet --no-detach <run-no-auto.txt &&
-	test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
-	test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-no-auto.txt &&
+	test_subcommand ! git gc --auto --quiet --no-detach --skip-maintenance-before-detach <run-auto.txt &&
+	test_subcommand git gc --no-quiet --no-detach --skip-maintenance-before-detach <run-no-quiet.txt
 '
 
 test_expect_success 'maintenance.auto config option' '
@@ -154,9 +154,9 @@ test_expect_success 'run --task=<task>' '
 		git maintenance run --task=commit-graph 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \
 		git maintenance run --task=commit-graph --task=gc 2>/dev/null &&
-	test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt &&
-	test_subcommand git gc --quiet --no-detach <run-gc.txt &&
-	test_subcommand git gc --quiet --no-detach <run-both.txt &&
+	test_subcommand ! git gc --quiet --no-detach --skip-maintenance-before-detach <run-commit-graph.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-gc.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-both.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
 	test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt

-- 
2.49.0.1266.g31b7d2e469.dirty

Re: [PATCH 04/11] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible

[Ramsay Jones]

On 27/05/2025 15:04, Patrick Steinhardt wrote:
> The "--task=" option explicitly allows the user to say which maintenance
> tasks should be run, whereas "--schedule=" only respects the maintenance
> strategy configured for a specific repository. As such, it is sensible

s/is sensible/is not sensible/ ?

> to accept both options at the same time.
> 
> Mark them as incompatible with one another. While at it, also convert
> the existing logic that marks "--auto" and "--schedule=" as incompatible
> to use `die_for_incompatible_opt2()`.
> 
> 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

[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 04/11] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible

[Patrick Steinhardt]

On Tue, May 27, 2025 at 05:43:23PM +0100, Ramsay Jones wrote:
> On 27/05/2025 15:04, Patrick Steinhardt wrote:
> > The "--task=" option explicitly allows the user to say which maintenance
> > tasks should be run, whereas "--schedule=" only respects the maintenance
> > strategy configured for a specific repository. As such, it is sensible
> 
> s/is sensible/is not sensible/ ?

Oh, obviously, yeah. Thanks!

Patrick

[PATCH v2 00/12] builtin/maintenance: fix ref lock races when detaching

[Patrick Steinhardt]

Hi,

this patch series fixes races around locking the "packed-refs" file when
auto-maintenance decides to repack it. This issue has been reported e.g.
via [1] and [2].

The root cause is that git-gc(1) used to know to detach _after_ having
repacked references. As such, callers wouldn't continue with their thing
until we have already packed refs, and thus the race does not exist
there. git-maintenance(1) didn't have the same split though, so this
patch series retrofits that logic.

The series is structured as follows:

  - Patches 1 and 2 do some light refactorings.

  - Patches 3 to 5 refactor how we set up the list of tasks to not rely
    on globals anymore. Instead, we now have a single source of truth
    for which tasks exactly will be run.

  - The remaining patches introduce the split of before/after-detach
    tasks and wire them up for "pack-refs", "reflog-expire" and "gc"
    tasks.

Changes in v2:
  - A couple of commit message improvements.
  - Introduce `die(NULL)` to die with the correct exit code but no error
    message. This gets rid of some magic numbers.
  - Introduce an enum to discern the phases before and after detach.
  - Link to v1: https://lore.kernel.org/r/20250527-b4-pks-maintenance-ref-lock-race-v1-0-e1ceb2dea66e@pks.im

Thanks!

Patrick

[1]: <CAJR-fbZ4X1+gN75m2dUvocR6NkowLOZ9F26cjBy8w1qd181OoQ@mail.gmail.com>
[2]: <CANi7bVAkNc+gY1NoXfJuDRjxjZLTgL8Lfn8_ZmWsvLAoiLPkNg@mail.gmail.com>

---
Patrick Steinhardt (12):
      builtin/gc: use designated field initializers for maintenance tasks
      builtin/gc: drop redundant local variable
      builtin/maintenance: centralize configuration of explicit tasks
      builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
      builtin/maintenance: stop modifying global array of tasks
      builtin/maintenance: extract function to run tasks
      builtin/maintenance: fix typedef for function pointers
      builtin/maintenance: let tasks do maintenance before and after detach
      builtin/maintenance: fix locking race when packing refs and reflogs
      usage: allow dying without writing an error message
      builtin/gc: avoid global state in `gc_before_repack()`
      builtin/maintenance: fix locking race when handling "gc" task

 builtin/am.c                |   4 +-
 builtin/checkout.c          |   4 +-
 builtin/fetch.c             |   2 +-
 builtin/gc.c                | 394 +++++++++++++++++++++++++-------------------
 builtin/submodule--helper.c |  12 +-
 t/t7900-maintenance.sh      |  19 ++-
 usage.c                     |   2 +
 7 files changed, 250 insertions(+), 187 deletions(-)

Range-diff versus v1:

 1:  74fcc4e2251 =  1:  87df070a9e7 builtin/gc: use designated field initializers for maintenance tasks
 2:  1cc513a7b0f =  2:  e2acea10f7e builtin/gc: drop redundant local variable
 3:  be8c8a98892 =  3:  48a5e25c8bc builtin/maintenance: centralize configuration of explicit tasks
 4:  b19fa152c81 !  4:  680b36e2fa6 builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
    @@ Commit message
     
         The "--task=" option explicitly allows the user to say which maintenance
         tasks should be run, whereas "--schedule=" only respects the maintenance
    -    strategy configured for a specific repository. As such, it is sensible
    -    to accept both options at the same time.
    +    strategy configured for a specific repository. As such, it is not
    +    sensible to accept both options at the same time.
     
         Mark them as incompatible with one another. While at it, also convert
         the existing logic that marks "--auto" and "--schedule=" as incompatible
 5:  8f692f30829 =  5:  9eaabb93edd builtin/maintenance: stop modifying global array of tasks
 6:  fc0ea110c01 =  6:  ffee3ca3c6c builtin/maintenance: extract function to run tasks
 7:  b5821ef6cfe =  7:  66e1bb2111b builtin/maintenance: fix typedef for function pointers
 8:  42a9210e445 !  8:  4eed6a8dc9c builtin/maintenance: let tasks do maintenance before and after detach
    @@ Commit message
         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.
    +    in 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,
    +    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)
    @@ builtin/gc.c: typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
      		.auto_condition = rerere_gc_condition,
      	},
      };
    -@@ builtin/gc.c: static const struct maintenance_task tasks[] = {
    + 
    ++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,
    -+			  int before)
    ++			  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;
    @@ builtin/gc.c: 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))
    ++		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. */
    @@ builtin/gc.c: 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))
    ++		if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg,
    ++				   TASK_PHASE_AFTER_DETACH))
      			result = 1;
      
      	rollback_lock_file(&lk);
 9:  7859d3b9b4f =  9:  41ae51294e6 builtin/maintenance: fix locking race when packing refs and reflogs
 -:  ----------- > 10:  2e1b4deb668 usage: allow dying without writing an error message
10:  18bce954787 ! 11:  7d9be688eb4 builtin/gc: avoid global state in `gc_before_repack()`
    @@ builtin/gc.c: int cmd_gc(int argc,
      
     -		gc_before_repack(&opts, &cfg); /* dies on failure */
     +		if (gc_before_repack(&opts, &cfg) < 0)
    -+			exit(127);
    ++			die(NULL);
      		delete_tempfile(&pidfile);
      
      		/*
11:  ff92709bf6c ! 12:  291498849b8 builtin/maintenance: fix locking race when handling "gc" task
    @@ builtin/gc.c: int cmd_gc(int argc,
     +			}
      
     -		if (gc_before_repack(&opts, &cfg) < 0)
    --			exit(127);
    +-			die(NULL);
     -		delete_tempfile(&pidfile);
     +			if (gc_before_detach(&opts, &cfg) < 0)
    -+				exit(127);
    ++				die(NULL);
     +			delete_tempfile(&pidfile);
     +		}
      

---
base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
change-id: 20250527-b4-pks-maintenance-ref-lock-race-11ae5d68e06f

[PATCH v2 01/12] builtin/gc: use designated field initializers for maintenance tasks

[Patrick Steinhardt]

Convert the array of maintenance tasks to use designated field
initializers. This makes it easier to add more fields to the struct
without having to modify all tasks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 54 +++++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e33ba946e43..54fc7f299a9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1550,49 +1550,49 @@ enum maintenance_task_label {
 
 static struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
-		"prefetch",
-		maintenance_task_prefetch,
+		.name = "prefetch",
+		.fn = maintenance_task_prefetch,
 	},
 	[TASK_LOOSE_OBJECTS] = {
-		"loose-objects",
-		maintenance_task_loose_objects,
-		loose_object_auto_condition,
+		.name = "loose-objects",
+		.fn = maintenance_task_loose_objects,
+		.auto_condition = loose_object_auto_condition,
 	},
 	[TASK_INCREMENTAL_REPACK] = {
-		"incremental-repack",
-		maintenance_task_incremental_repack,
-		incremental_repack_auto_condition,
+		.name = "incremental-repack",
+		.fn = maintenance_task_incremental_repack,
+		.auto_condition = incremental_repack_auto_condition,
 	},
 	[TASK_GC] = {
-		"gc",
-		maintenance_task_gc,
-		need_to_gc,
-		1,
+		.name = "gc",
+		.fn = maintenance_task_gc,
+		.auto_condition = need_to_gc,
+		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
-		"commit-graph",
-		maintenance_task_commit_graph,
-		should_write_commit_graph,
+		.name = "commit-graph",
+		.fn = maintenance_task_commit_graph,
+		.auto_condition = should_write_commit_graph,
 	},
 	[TASK_PACK_REFS] = {
-		"pack-refs",
-		maintenance_task_pack_refs,
-		pack_refs_condition,
+		.name = "pack-refs",
+		.fn = maintenance_task_pack_refs,
+		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
-		"reflog-expire",
-		maintenance_task_reflog_expire,
-		reflog_expire_condition,
+		.name = "reflog-expire",
+		.fn = maintenance_task_reflog_expire,
+		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {
-		"worktree-prune",
-		maintenance_task_worktree_prune,
-		worktree_prune_condition,
+		.name = "worktree-prune",
+		.fn = maintenance_task_worktree_prune,
+		.auto_condition = worktree_prune_condition,
 	},
 	[TASK_RERERE_GC] = {
-		"rerere-gc",
-		maintenance_task_rerere_gc,
-		rerere_gc_condition,
+		.name = "rerere-gc",
+		.fn = maintenance_task_rerere_gc,
+		.auto_condition = rerere_gc_condition,
 	},
 };
 

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 02/12] builtin/gc: drop redundant local variable

[Patrick Steinhardt]

We have two different variables that track the quietness for git-gc(1):

  - The local variable `quiet`, which we wire up.

  - The `quiet` field of `struct maintenance_run_opts`.

This leads to confusion which of these variables should be used and what
the respective effect is.

Simplify this logic by dropping the local variable in favor of the
options field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 54fc7f299a9..7adda8d2d0d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -818,7 +818,6 @@ int cmd_gc(int argc,
 	   struct repository *repo UNUSED)
 {
 	int aggressive = 0;
-	int quiet = 0;
 	int force = 0;
 	const char *name;
 	pid_t pid;
@@ -831,7 +830,7 @@ int cmd_gc(int argc,
 	const char *prune_expire_arg = prune_expire_sentinel;
 	int ret;
 	struct option builtin_gc_options[] = {
-		OPT__QUIET(&quiet, N_("suppress progress reporting")),
+		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		{
 			.type = OPTION_STRING,
 			.long_name = "prune",
@@ -891,7 +890,7 @@ int cmd_gc(int argc,
 		if (cfg.aggressive_window > 0)
 			strvec_pushf(&repack, "--window=%d", cfg.aggressive_window);
 	}
-	if (quiet)
+	if (opts.quiet)
 		strvec_push(&repack, "-q");
 
 	if (opts.auto_flag) {
@@ -906,7 +905,7 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (!quiet) {
+		if (!opts.quiet) {
 			if (opts.detach > 0)
 				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
 			else
@@ -991,7 +990,7 @@ int cmd_gc(int argc,
 			strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL);
 			/* run `git prune` even if using cruft packs */
 			strvec_push(&prune_cmd.args, cfg.prune_expire);
-			if (quiet)
+			if (opts.quiet)
 				strvec_push(&prune_cmd.args, "--no-progress");
 			if (repo_has_promisor_remote(the_repository))
 				strvec_push(&prune_cmd.args,
@@ -1019,7 +1018,7 @@ int cmd_gc(int argc,
 
 	if (the_repository->settings.gc_write_commit_graph == 1)
 		write_commit_graph_reachable(the_repository->objects->odb,
-					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
+					     !opts.quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
 					     NULL);
 
 	if (opts.auto_flag && too_many_loose_objects(&cfg))

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 03/12] builtin/maintenance: centralize configuration of explicit tasks

[Patrick Steinhardt]

Users of git-maintenance(1) can explicitly ask it to run specific tasks
by passing the `--task=` command line option. This option can be passed
multiple times, which causes us to execute tasks in the same order as
the tasks have been provided by the user.

The order in which tasks are run is computed in `task_option_parse()`:
every time we parse such a command line argument, we modify the global
array of tasks by seting the selected index for that specific task.
This has two downsides:

  - We modify global state, which makes it hard to follow the logic.

  - The configuration of tasks is split across multiple different
    functions, so it is not easy to figure out the different factors
    that play a role in selecting tasks.

Refactor the logic so that `task_option_parse()` does not modify global
state anymore. Instead, this function now only collects the list of
configured tasks. The logic to configure ordering of the respective
tasks is then deferred to `initialize_task_config()`.

This refactoring solves the second problem, that the configuration of
tasks is spread across multiple different locations. The first problem,
that we modify global state, will be fixed in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7adda8d2d0d..c4af9b11287 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1690,15 +1690,22 @@ static void initialize_maintenance_strategy(void)
 	}
 }
 
-static void initialize_task_config(int schedule)
+static void initialize_task_config(const struct string_list *selected_tasks,
+				   int schedule)
 {
-	int i;
 	struct strbuf config_name = STRBUF_INIT;
 
+	for (size_t i = 0; i < TASK__COUNT; i++)
+		tasks[i].selected_order = -1;
+	for (size_t i = 0; i < selected_tasks->nr; i++) {
+		struct maintenance_task *task = selected_tasks->items[i].util;
+		task->selected_order = i;
+	}
+
 	if (schedule)
 		initialize_maintenance_strategy();
 
-	for (i = 0; i < TASK__COUNT; i++) {
+	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
 		char *config_str;
 
@@ -1722,33 +1729,28 @@ static void initialize_task_config(int schedule)
 	strbuf_release(&config_name);
 }
 
-static int task_option_parse(const struct option *opt UNUSED,
+static int task_option_parse(const struct option *opt,
 			     const char *arg, int unset)
 {
-	int i, num_selected = 0;
-	struct maintenance_task *task = NULL;
+	struct string_list *selected_tasks = opt->value;
+	size_t i;
 
 	BUG_ON_OPT_NEG(unset);
 
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (tasks[i].selected_order >= 0)
-			num_selected++;
-		if (!strcasecmp(tasks[i].name, arg)) {
-			task = &tasks[i];
-		}
-	}
-
-	if (!task) {
+	for (i = 0; i < TASK__COUNT; i++)
+		if (!strcasecmp(tasks[i].name, arg))
+			break;
+	if (i >= TASK__COUNT) {
 		error(_("'%s' is not a valid task"), arg);
 		return 1;
 	}
 
-	if (task->selected_order >= 0) {
+	if (unsorted_string_list_has_string(selected_tasks, arg)) {
 		error(_("task '%s' cannot be selected multiple times"), arg);
 		return 1;
 	}
 
-	task->selected_order = num_selected + 1;
+	string_list_append(selected_tasks, arg)->util = &tasks[i];
 
 	return 0;
 }
@@ -1756,8 +1758,8 @@ static int task_option_parse(const struct option *opt UNUSED,
 static int maintenance_run(int argc, const char **argv, const char *prefix,
 			   struct repository *repo UNUSED)
 {
-	int i;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
+	struct string_list selected_tasks = STRING_LIST_INIT_DUP;
 	struct gc_config cfg = GC_CONFIG_INIT;
 	struct option builtin_maintenance_run_options[] = {
 		OPT_BOOL(0, "auto", &opts.auto_flag,
@@ -1769,7 +1771,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     maintenance_opt_schedule),
 		OPT_BOOL(0, "quiet", &opts.quiet,
 			 N_("do not report progress or other information over stderr")),
-		OPT_CALLBACK_F(0, "task", NULL, N_("task"),
+		OPT_CALLBACK_F(0, "task", &selected_tasks, N_("task"),
 			N_("run a specific task"),
 			PARSE_OPT_NONEG, task_option_parse),
 		OPT_END()
@@ -1778,9 +1780,6 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 
 	opts.quiet = !isatty(2);
 
-	for (i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_run_options,
 			     builtin_maintenance_run_usage,
@@ -1790,13 +1789,15 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 		die(_("use at most one of --auto and --schedule=<frequency>"));
 
 	gc_config(&cfg);
-	initialize_task_config(opts.schedule);
+	initialize_task_config(&selected_tasks, opts.schedule);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
 				   builtin_maintenance_run_options);
 
 	ret = maintenance_run_tasks(&opts, &cfg);
+
+	string_list_clear(&selected_tasks, 0);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 04/12] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible

[Patrick Steinhardt]

The "--task=" option explicitly allows the user to say which maintenance
tasks should be run, whereas "--schedule=" only respects the maintenance
strategy configured for a specific repository. As such, it is not
sensible to accept both options at the same time.

Mark them as incompatible with one another. While at it, also convert
the existing logic that marks "--auto" and "--schedule=" as incompatible
to use `die_for_incompatible_opt2()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 6 ++++--
 t/t7900-maintenance.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c4af9b11287..57d7602596a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1785,8 +1785,10 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     builtin_maintenance_run_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (opts.auto_flag && opts.schedule)
-		die(_("use at most one of --auto and --schedule=<frequency>"));
+	die_for_incompatible_opt2(opts.auto_flag, "--auto",
+				  opts.schedule, "--schedule=");
+	die_for_incompatible_opt2(selected_tasks.nr, "--task=",
+				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
 	initialize_task_config(&selected_tasks, opts.schedule);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 8cf89e285f4..1ada5246606 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -610,7 +610,12 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
 
 test_expect_success '--auto and --schedule incompatible' '
 	test_must_fail git maintenance run --auto --schedule=daily 2>err &&
-	test_grep "at most one" err
+	test_grep "cannot be used together" err
+'
+
+test_expect_success '--task and --schedule incompatible' '
+	test_must_fail git maintenance run --task=pack-refs --schedule=daily 2>err &&
+	test_grep "cannot be used together" err
 '
 
 test_expect_success 'invalid --schedule value' '

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 05/12] builtin/maintenance: stop modifying global array of tasks

[Patrick Steinhardt]

When configuring maintenance tasks run by git-maintenance(1) we do so by
modifying the global array of tasks directly. This is already quite bad
on its own, as global state makes for logic that is hard to follow.

Even more importantly though we use multiple different fields to track
whether or not a task should be run:

  - "enabled" tracks the "maintenance.*.enabled" config key. This field
    disables execution of a task, unless the user has explicitly asked
    for the task.

  - "selected_order" tracks the order in which jobs have been asked for
    by the user via the "--task=" command line option. It overrides
    everything else, but only has an effect if at least one job has been
    selected.

  - "schedule" tracks the schedule priority for a job, that is how often
    it should run. This field only plays a role when the user has passed
    the "--schedule=" command line option.

All of this makes it non-trivial to figure out which job really should
be running right now. The logic to configure these fields and the logic
that interprets them is distributed across multiple functions, making it
even harder to follow it.

Refactor the logic so that we stop modifying global state. Instead, we
now compute which jobs should be run in `initialize_task_config()`,
represented as an array of jobs to run that is stored in the options
structure. Like this, all logic becomes self-contained and any users of
this array only need to iterate through the tasks and execute them one
by one.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 206 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 112 insertions(+), 94 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 57d7602596a..4d636237cac 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -251,7 +251,24 @@ static enum schedule_priority parse_schedule(const char *value)
 	return SCHEDULE_NONE;
 }
 
+enum maintenance_task_label {
+	TASK_PREFETCH,
+	TASK_LOOSE_OBJECTS,
+	TASK_INCREMENTAL_REPACK,
+	TASK_GC,
+	TASK_COMMIT_GRAPH,
+	TASK_PACK_REFS,
+	TASK_REFLOG_EXPIRE,
+	TASK_WORKTREE_PRUNE,
+	TASK_RERERE_GC,
+
+	/* Leave as final value */
+	TASK__COUNT
+};
+
 struct maintenance_run_opts {
+	enum maintenance_task_label *tasks;
+	size_t tasks_nr, tasks_alloc;
 	int auto_flag;
 	int detach;
 	int quiet;
@@ -261,6 +278,11 @@ struct maintenance_run_opts {
 	.detach = -1, \
 }
 
+static void maintenance_run_opts_release(struct maintenance_run_opts *opts)
+{
+	free(opts->tasks);
+}
+
 static int pack_refs_condition(UNUSED struct gc_config *cfg)
 {
 	/*
@@ -1032,6 +1054,7 @@ int cmd_gc(int argc,
 	}
 
 out:
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return 0;
 }
@@ -1524,30 +1547,9 @@ struct maintenance_task {
 	const char *name;
 	maintenance_task_fn *fn;
 	maintenance_auto_fn *auto_condition;
-	unsigned enabled:1;
-
-	enum schedule_priority schedule;
-
-	/* -1 if not selected. */
-	int selected_order;
-};
-
-enum maintenance_task_label {
-	TASK_PREFETCH,
-	TASK_LOOSE_OBJECTS,
-	TASK_INCREMENTAL_REPACK,
-	TASK_GC,
-	TASK_COMMIT_GRAPH,
-	TASK_PACK_REFS,
-	TASK_REFLOG_EXPIRE,
-	TASK_WORKTREE_PRUNE,
-	TASK_RERERE_GC,
-
-	/* Leave as final value */
-	TASK__COUNT
 };
 
-static struct maintenance_task tasks[] = {
+static const struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
 		.name = "prefetch",
 		.fn = maintenance_task_prefetch,
@@ -1566,7 +1568,6 @@ static struct maintenance_task tasks[] = {
 		.name = "gc",
 		.fn = maintenance_task_gc,
 		.auto_condition = need_to_gc,
-		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
 		.name = "commit-graph",
@@ -1595,18 +1596,9 @@ static struct maintenance_task tasks[] = {
 	},
 };
 
-static int compare_tasks_by_selection(const void *a_, const void *b_)
-{
-	const struct maintenance_task *a = a_;
-	const struct maintenance_task *b = b_;
-
-	return b->selected_order - a->selected_order;
-}
-
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
-	int i, found_selected = 0;
 	int result = 0;
 	struct lock_file lk;
 	struct repository *r = the_repository;
@@ -1635,95 +1627,120 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (i = 0; !found_selected && i < TASK__COUNT; i++)
-		found_selected = tasks[i].selected_order >= 0;
-
-	if (found_selected)
-		QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);
-
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (found_selected && tasks[i].selected_order < 0)
-			continue;
-
-		if (!found_selected && !tasks[i].enabled)
-			continue;
-
+	for (size_t i = 0; i < opts->tasks_nr; i++) {
 		if (opts->auto_flag &&
-		    (!tasks[i].auto_condition ||
-		     !tasks[i].auto_condition(cfg)))
-			continue;
-
-		if (opts->schedule && tasks[i].schedule < opts->schedule)
+		    (!tasks[opts->tasks[i]].auto_condition ||
+		     !tasks[opts->tasks[i]].auto_condition(cfg)))
 			continue;
 
-		trace2_region_enter("maintenance", tasks[i].name, r);
-		if (tasks[i].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[i].name);
+		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
+		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
+			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
 			result = 1;
 		}
-		trace2_region_leave("maintenance", tasks[i].name, r);
+		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
 	}
 
 	rollback_lock_file(&lk);
 	return result;
 }
 
-static void initialize_maintenance_strategy(void)
+struct maintenance_strategy {
+	struct {
+		int enabled;
+		enum schedule_priority schedule;
+	} tasks[TASK__COUNT];
+};
+
+static const struct maintenance_strategy none_strategy = { 0 };
+static const struct maintenance_strategy default_strategy = {
+	.tasks = {
+		[TASK_GC].enabled = 1,
+	},
+};
+static const struct maintenance_strategy incremental_strategy = {
+	.tasks = {
+		[TASK_COMMIT_GRAPH].enabled = 1,
+		[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY,
+		[TASK_PREFETCH].enabled = 1,
+		[TASK_PREFETCH].schedule = SCHEDULE_HOURLY,
+		[TASK_INCREMENTAL_REPACK].enabled = 1,
+		[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY,
+		[TASK_LOOSE_OBJECTS].enabled = 1,
+		[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY,
+		[TASK_PACK_REFS].enabled = 1,
+		[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY,
+	},
+};
+
+static void initialize_task_config(struct maintenance_run_opts *opts,
+				   const struct string_list *selected_tasks)
 {
+	struct strbuf config_name = STRBUF_INIT;
+	struct maintenance_strategy strategy;
 	const char *config_str;
 
-	if (git_config_get_string_tmp("maintenance.strategy", &config_str))
-		return;
+	/*
+	 * In case the user has asked us to run tasks explicitly we only use
+	 * those specified tasks. Specifically, we do _not_ want to consult the
+	 * config or maintenance strategy.
+	 */
+	if (selected_tasks->nr) {
+		for (size_t i = 0; i < selected_tasks->nr; i++) {
+			enum maintenance_task_label label = (intptr_t)selected_tasks->items[i].util;;
+			ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+			opts->tasks[opts->tasks_nr++] = label;
+		}
 
-	if (!strcasecmp(config_str, "incremental")) {
-		tasks[TASK_GC].schedule = SCHEDULE_NONE;
-		tasks[TASK_COMMIT_GRAPH].enabled = 1;
-		tasks[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_PREFETCH].enabled = 1;
-		tasks[TASK_PREFETCH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_INCREMENTAL_REPACK].enabled = 1;
-		tasks[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY;
-		tasks[TASK_LOOSE_OBJECTS].enabled = 1;
-		tasks[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY;
-		tasks[TASK_PACK_REFS].enabled = 1;
-		tasks[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY;
+		return;
 	}
-}
 
-static void initialize_task_config(const struct string_list *selected_tasks,
-				   int schedule)
-{
-	struct strbuf config_name = STRBUF_INIT;
+	/*
+	 * Otherwise, the strategy depends on whether we run as part of a
+	 * scheduled job or not:
+	 *
+	 *   - Scheduled maintenance does not perform any housekeeping by
+	 *     default, but requires the user to pick a maintenance strategy.
+	 *
+	 *   - Unscheduled maintenance uses our default strategy.
+	 *
+	 * Both of these are affected by the gitconfig though, which may
+	 * override specific aspects of our strategy.
+	 */
+	if (opts->schedule) {
+		strategy = none_strategy;
 
-	for (size_t i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-	for (size_t i = 0; i < selected_tasks->nr; i++) {
-		struct maintenance_task *task = selected_tasks->items[i].util;
-		task->selected_order = i;
+		if (!git_config_get_string_tmp("maintenance.strategy", &config_str)) {
+			if (!strcasecmp(config_str, "incremental"))
+				strategy = incremental_strategy;
+		}
+	} else {
+		strategy = default_strategy;
 	}
 
-	if (schedule)
-		initialize_maintenance_strategy();
-
 	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
-		char *config_str;
 
 		strbuf_reset(&config_name);
 		strbuf_addf(&config_name, "maintenance.%s.enabled",
 			    tasks[i].name);
-
 		if (!git_config_get_bool(config_name.buf, &config_value))
-			tasks[i].enabled = config_value;
-
-		strbuf_reset(&config_name);
-		strbuf_addf(&config_name, "maintenance.%s.schedule",
-			    tasks[i].name);
+			strategy.tasks[i].enabled = config_value;
+		if (!strategy.tasks[i].enabled)
+			continue;
 
-		if (!git_config_get_string(config_name.buf, &config_str)) {
-			tasks[i].schedule = parse_schedule(config_str);
-			free(config_str);
+		if (opts->schedule) {
+			strbuf_reset(&config_name);
+			strbuf_addf(&config_name, "maintenance.%s.schedule",
+				    tasks[i].name);
+			if (!git_config_get_string_tmp(config_name.buf, &config_str))
+				strategy.tasks[i].schedule = parse_schedule(config_str);
+			if (strategy.tasks[i].schedule < opts->schedule)
+				continue;
 		}
+
+		ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+		opts->tasks[opts->tasks_nr++] = i;
 	}
 
 	strbuf_release(&config_name);
@@ -1750,7 +1767,7 @@ static int task_option_parse(const struct option *opt,
 		return 1;
 	}
 
-	string_list_append(selected_tasks, arg)->util = &tasks[i];
+	string_list_append(selected_tasks, arg)->util = (void *)(intptr_t)i;
 
 	return 0;
 }
@@ -1791,7 +1808,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
-	initialize_task_config(&selected_tasks, opts.schedule);
+	initialize_task_config(&opts, &selected_tasks);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
@@ -1800,6 +1817,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 	ret = maintenance_run_tasks(&opts, &cfg);
 
 	string_list_clear(&selected_tasks, 0);
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 06/12] builtin/maintenance: extract function to run tasks

[Patrick Steinhardt]

Extract the function to run maintenance tasks. This function will be
reused in a subsequent commit where we introduce a split between
maintenance tasks that run before and after daemonizing the process.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4d636237cac..cfbf9d8a2b9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1596,6 +1596,27 @@ 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)
+{
+	int ret = 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)) {
+		error(_("task '%s' failed"), task->name);
+		ret = 1;
+	}
+	trace2_region_leave("maintenance", task->name, repo);
+
+	return ret;
+}
+
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
@@ -1627,19 +1648,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (size_t i = 0; i < opts->tasks_nr; i++) {
-		if (opts->auto_flag &&
-		    (!tasks[opts->tasks[i]].auto_condition ||
-		     !tasks[opts->tasks[i]].auto_condition(cfg)))
-			continue;
-
-		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
-		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
+	for (size_t i = 0; i < opts->tasks_nr; i++)
+		if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg))
 			result = 1;
-		}
-		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
-	}
 
 	rollback_lock_file(&lk);
 	return result;

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 07/12] builtin/maintenance: fix typedef for function pointers

[Patrick Steinhardt]

The typedefs for `maintenance_task_fn` and `maintenance_auto_fn` are
somewhat confusingly not true function pointers. As such, any user of
those typedefs needs to manually add the pointer to make use of them.

Fix this by making these true function pointers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cfbf9d8a2b9..447e5800846 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1533,20 +1533,20 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
 	return 0;
 }
 
-typedef int maintenance_task_fn(struct maintenance_run_opts *opts,
-				struct gc_config *cfg);
+typedef int (*maintenance_task_fn)(struct maintenance_run_opts *opts,
+				   struct gc_config *cfg);
 
 /*
  * An auto condition function returns 1 if the task should run
  * and 0 if the task should NOT run. See needs_to_gc() for an
  * example.
  */
-typedef int maintenance_auto_fn(struct gc_config *cfg);
+typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
 
 struct maintenance_task {
 	const char *name;
-	maintenance_task_fn *fn;
-	maintenance_auto_fn *auto_condition;
+	maintenance_task_fn fn;
+	maintenance_auto_fn auto_condition;
 };
 
 static const struct maintenance_task tasks[] = {

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 08/12] 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
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

[PATCH v2 09/12] builtin/maintenance: fix locking race when packing refs and reflogs

[Patrick Steinhardt]

As explained in the preceding commit, git-gc(1) knows to detach only
after it has already packed references and reflogs. This is done to
avoid racing around their respective lockfiles.

Adapt git-maintenance(1) accordingly and run the "pack-refs" and
"reflog-expire" tasks before detaching. Note that the "gc" task has the
same issue, but the fix is a bit more involved there and will thus be
done in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index f64bae0a825..e92015887a7 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1577,12 +1577,12 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_PACK_REFS] = {
 		.name = "pack-refs",
-		.after_detach = maintenance_task_pack_refs,
+		.before_detach = maintenance_task_pack_refs,
 		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
 		.name = "reflog-expire",
-		.after_detach = maintenance_task_reflog_expire,
+		.before_detach = maintenance_task_reflog_expire,
 		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 10/12] usage: allow dying without writing an error message

[Patrick Steinhardt]

Sometimes code wants to die in a situation where it already has written
an error message. To use the same error code as `die()` we have to open
code the code with a call to `exit(128)` in such cases, which is easy to
get wrong and leaves magical numbers all over our codebase.

Teach `die_message_builtin()` to not print any error when passed a
`NULL` pointer as error string. Like this, such users can now call
`die(NULL)` to achieve the same result without any hardcoded error
codes.

Adapt a couple of builtins to use this new pattern to demonstrate that
there is a need for such a helper.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/am.c                |  4 ++--
 builtin/checkout.c          |  4 ++--
 builtin/fetch.c             |  2 +-
 builtin/submodule--helper.c | 12 ++++++------
 usage.c                     |  2 ++
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index e32a3b4c973..a800003340f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1000,7 +1000,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 
 	if (!patch_format) {
 		fprintf_ln(stderr, _("Patch format detection failed."));
-		exit(128);
+		die(NULL);
 	}
 
 	if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
@@ -1178,7 +1178,7 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 		strbuf_release(&sb);
 	}
 
-	exit(128);
+	die(NULL);
 }
 
 /**
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d185982f3a6..536192d3456 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -838,7 +838,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		init_tree_desc(&trees[0], &tree->object.oid,
 			       tree->buffer, tree->size);
 		if (parse_tree(new_tree) < 0)
-			exit(128);
+			die(NULL);
 		tree = new_tree;
 		init_tree_desc(&trees[1], &tree->object.oid,
 			       tree->buffer, tree->size);
@@ -913,7 +913,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 						     work,
 						     old_tree);
 			if (ret < 0)
-				exit(128);
+				die(NULL);
 			ret = reset_tree(new_tree,
 					 opts, 0,
 					 writeout_error, new_branch_info);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index cda6eaf1fd6..b0800ea5829 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -992,7 +992,7 @@ static int update_local_ref(struct ref *ref,
 		fast_forward = repo_in_merge_bases(the_repository, current,
 						   updated);
 		if (fast_forward < 0)
-			exit(128);
+			die(NULL);
 		forced_updates_ms += (getnanotime() - t_before) / 1000000;
 	} else {
 		fast_forward = 1;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116ddf..4255caca579 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -303,7 +303,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 	char *displaypath;
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	displaypath = get_submodule_displaypath(path, info->prefix,
 						info->super_prefix);
@@ -643,7 +643,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	};
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	if (!submodule_from_path(the_repository, null_oid(the_hash_algo), path))
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -1257,7 +1257,7 @@ static void sync_submodule(const char *path, const char *prefix,
 		return;
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), path);
 
@@ -1402,7 +1402,7 @@ static void deinit_submodule(const char *path, const char *prefix,
 	char *sub_git_dir = xstrfmt("%s/.git", path);
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), path);
 
@@ -1724,7 +1724,7 @@ static int clone_submodule(const struct module_clone_data *clone_data,
 	char *to_free = NULL;
 
 	if (validate_submodule_path(clone_data_path) < 0)
-		exit(128);
+		die(NULL);
 
 	if (!is_absolute_path(clone_data->path))
 		clone_data_path = to_free = xstrfmt("%s/%s", repo_get_work_tree(the_repository),
@@ -3524,7 +3524,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	strip_dir_trailing_slashes(add_data.sm_path);
 
 	if (validate_submodule_path(add_data.sm_path) < 0)
-		exit(128);
+		die(NULL);
 
 	die_on_index_match(add_data.sm_path, force);
 	die_on_repo_without_commits(add_data.sm_path);
diff --git a/usage.c b/usage.c
index 38b46bbbfe7..cd7b57d6446 100644
--- a/usage.c
+++ b/usage.c
@@ -67,6 +67,8 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 
 static void die_message_builtin(const char *err, va_list params)
 {
+	if (!err)
+		return;
 	trace2_cmd_error_va(err, params);
 	vreportf(_("fatal: "), err, params);
 }

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 11/12] builtin/gc: avoid global state in `gc_before_repack()`

[Patrick Steinhardt]

The `gc_before_repack()` should only ever run once in git-gc(1), but we
may end up calling it twice when the "--detach" flag is passed. The
duplicated call is avoided though via a static flag in this function.

This pattern is somewhat unintuitive though. Refactor it to drop the
static flag and instead guard the second call of `gc_before_repack()`
via `opts.detach`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e92015887a7..e910a99e033 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,22 +816,14 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static void gc_before_repack(struct maintenance_run_opts *opts,
-			     struct gc_config *cfg)
+static int gc_before_repack(struct maintenance_run_opts *opts,
+			    struct gc_config *cfg)
 {
-	/*
-	 * We may be called twice, as both the pre- and
-	 * post-daemonized phases will call us, but running these
-	 * commands more than once is pointless and wasteful.
-	 */
-	static int done = 0;
-	if (done++)
-		return;
-
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
-		die(FAILED_RUN, "pack-refs");
+		return error(FAILED_RUN, "pack-refs");
 	if (cfg->prune_reflogs && maintenance_task_reflog_expire(opts, cfg))
-		die(FAILED_RUN, "reflog");
+		return error(FAILED_RUN, "reflog");
+	return 0;
 }
 
 int cmd_gc(int argc,
@@ -965,7 +957,8 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		gc_before_repack(&opts, &cfg); /* dies on failure */
+		if (gc_before_repack(&opts, &cfg) < 0)
+			die(NULL);
 		delete_tempfile(&pidfile);
 
 		/*
@@ -995,7 +988,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0)
+		gc_before_repack(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v2 12/12] builtin/maintenance: fix locking race when handling "gc" task

[Patrick Steinhardt]

The "gc" task has a similar locking race as the one that we have fixed
for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix
this by splitting up the logic of the "gc" task:

  - Before detaching we execute `gc_before_repack()`, which contains the
    logic that git-gc(1) itself would execute before detaching.

  - After detaching we spawn git-gc(1), but with a new hidden flag that
    suppresses calling `gc_before_repack()`.

Like this we have roughly the same logic as git-gc(1) itself and know to
repack refs and reflogs before detaching, thus fixing the race.

Note that `gc_before_repack()` is renamed to `gc_before_detach()` to
better reflect what this function does.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 39 ++++++++++++++++++++++++++-------------
 t/t7900-maintenance.sh | 12 ++++++------
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e910a99e033..dfbf20491e3 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,7 +816,7 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static int gc_before_repack(struct maintenance_run_opts *opts,
+static int gc_before_detach(struct maintenance_run_opts *opts,
 			    struct gc_config *cfg)
 {
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
@@ -837,6 +837,7 @@ int cmd_gc(int argc,
 	pid_t pid;
 	int daemonized = 0;
 	int keep_largest_pack = -1;
+	int skip_maintenance_before_detach = 0;
 	timestamp_t dummy;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
 	struct gc_config cfg = GC_CONFIG_INIT;
@@ -869,6 +870,8 @@ int cmd_gc(int argc,
 			 N_("repack all other packs except the largest pack")),
 		OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"),
 			   N_("pack prefix to store a pack containing pruned objects")),
+		OPT_HIDDEN_BOOL(0, "skip-maintenance-before-detach", &skip_maintenance_before_detach,
+			   N_("skip maintenance steps typically done before detaching")),
 		OPT_END()
 	};
 
@@ -952,14 +955,16 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (lock_repo_for_gc(force, &pid)) {
-			ret = 0;
-			goto out;
-		}
+		if (!skip_maintenance_before_detach) {
+			if (lock_repo_for_gc(force, &pid)) {
+				ret = 0;
+				goto out;
+			}
 
-		if (gc_before_repack(&opts, &cfg) < 0)
-			die(NULL);
-		delete_tempfile(&pidfile);
+			if (gc_before_detach(&opts, &cfg) < 0)
+				die(NULL);
+			delete_tempfile(&pidfile);
+		}
 
 		/*
 		 * failure to daemonize is ok, we'll continue
@@ -988,8 +993,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	if (opts.detach <= 0)
-		gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0 && !skip_maintenance_before_detach)
+		gc_before_detach(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;
@@ -1225,8 +1230,14 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,
 	return 0;
 }
 
-static int maintenance_task_gc(struct maintenance_run_opts *opts,
-			       struct gc_config *cfg UNUSED)
+static int maintenance_task_gc_before_detach(struct maintenance_run_opts *opts,
+					     struct gc_config *cfg)
+{
+	return gc_before_detach(opts, cfg);
+}
+
+static int maintenance_task_gc_after_detach(struct maintenance_run_opts *opts,
+					    struct gc_config *cfg UNUSED)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
@@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts,
 	else
 		strvec_push(&child.args, "--no-quiet");
 	strvec_push(&child.args, "--no-detach");
+	strvec_push(&child.args, "--skip-maintenance-before-detach");
 
 	return run_command(&child);
 }
@@ -1561,7 +1573,8 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_GC] = {
 		.name = "gc",
-		.after_detach = maintenance_task_gc,
+		.before_detach = maintenance_task_gc_before_detach,
+		.after_detach = maintenance_task_gc_after_detach,
 		.auto_condition = need_to_gc,
 	},
 	[TASK_COMMIT_GRAPH] = {
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 1ada5246606..e09a36ab021 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -49,9 +49,9 @@ test_expect_success 'run [--auto|--quiet]' '
 		git maintenance run --auto 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \
 		git maintenance run --no-quiet 2>/dev/null &&
-	test_subcommand git gc --quiet --no-detach <run-no-auto.txt &&
-	test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
-	test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-no-auto.txt &&
+	test_subcommand ! git gc --auto --quiet --no-detach --skip-maintenance-before-detach <run-auto.txt &&
+	test_subcommand git gc --no-quiet --no-detach --skip-maintenance-before-detach <run-no-quiet.txt
 '
 
 test_expect_success 'maintenance.auto config option' '
@@ -154,9 +154,9 @@ test_expect_success 'run --task=<task>' '
 		git maintenance run --task=commit-graph 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \
 		git maintenance run --task=commit-graph --task=gc 2>/dev/null &&
-	test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt &&
-	test_subcommand git gc --quiet --no-detach <run-gc.txt &&
-	test_subcommand git gc --quiet --no-detach <run-both.txt &&
+	test_subcommand ! git gc --quiet --no-detach --skip-maintenance-before-detach <run-commit-graph.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-gc.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-both.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
 	test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt

-- 
2.50.0.rc0.604.gd4ff7b7c86.dirty

[PATCH v3 00/12] builtin/maintenance: fix ref lock races when detaching

[Patrick Steinhardt]

Hi,

this patch series fixes races around locking the "packed-refs" file when
auto-maintenance decides to repack it. This issue has been reported e.g.
via [1] and [2].

The root cause is that git-gc(1) used to know to detach _after_ having
repacked references. As such, callers wouldn't continue with their thing
until we have already packed refs, and thus the race does not exist
there. git-maintenance(1) didn't have the same split though, so this
patch series retrofits that logic.

The series is structured as follows:

  - Patches 1 and 2 do some light refactorings.

  - Patches 3 to 5 refactor how we set up the list of tasks to not rely
    on globals anymore. Instead, we now have a single source of truth
    for which tasks exactly will be run.

  - The remaining patches introduce the split of before/after-detach
    tasks and wire them up for "pack-refs", "reflog-expire" and "gc"
    tasks.

Changes in v2:
  - A couple of commit message improvements.
  - Introduce `die(NULL)` to die with the correct exit code but no error
    message. This gets rid of some magic numbers.
  - Introduce an enum to discern the phases before and after detach.
  - Link to v1: https://lore.kernel.org/r/20250527-b4-pks-maintenance-ref-lock-race-v1-0-e1ceb2dea66e@pks.im

Changes in v3:
  - Rework logic to talk about foreground/background tasks instead of
    before/after detach.
  - Link to v2: https://lore.kernel.org/r/20250530-b4-pks-maintenance-ref-lock-race-v2-0-d04e2f93e51f@pks.im

Thanks!

Patrick

[1]: <CAJR-fbZ4X1+gN75m2dUvocR6NkowLOZ9F26cjBy8w1qd181OoQ@mail.gmail.com>
[2]: <CANi7bVAkNc+gY1NoXfJuDRjxjZLTgL8Lfn8_ZmWsvLAoiLPkNg@mail.gmail.com>

---
Patrick Steinhardt (12):
      builtin/gc: use designated field initializers for maintenance tasks
      builtin/gc: drop redundant local variable
      builtin/maintenance: centralize configuration of explicit tasks
      builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
      builtin/maintenance: stop modifying global array of tasks
      builtin/maintenance: extract function to run tasks
      builtin/maintenance: fix typedef for function pointers
      builtin/maintenance: split into foreground and background tasks
      builtin/maintenance: fix locking race when packing refs and reflogs
      usage: allow dying without writing an error message
      builtin/gc: avoid global state in `gc_before_repack()`
      builtin/maintenance: fix locking race when handling "gc" task

 builtin/am.c                |   4 +-
 builtin/checkout.c          |   4 +-
 builtin/fetch.c             |   2 +-
 builtin/gc.c                | 410 +++++++++++++++++++++++++-------------------
 builtin/submodule--helper.c |  12 +-
 t/t7900-maintenance.sh      |  19 +-
 usage.c                     |   2 +
 7 files changed, 263 insertions(+), 190 deletions(-)

Range-diff versus v2:

 1:  2c90fadd640 =  1:  00fa348fb9c builtin/gc: use designated field initializers for maintenance tasks
 2:  ebd37a92895 =  2:  d7a2072da28 builtin/gc: drop redundant local variable
 3:  90863ae9957 =  3:  2eef35388e1 builtin/maintenance: centralize configuration of explicit tasks
 4:  f0c05004ea3 =  4:  372efcf7fec builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
 5:  85c8273d4f4 =  5:  979f27b19ce builtin/maintenance: stop modifying global array of tasks
 6:  a9d49980f8a =  6:  b89c8d237ed builtin/maintenance: extract function to run tasks
 7:  35a78300572 =  7:  96554899fd9 builtin/maintenance: fix typedef for function pointers
 8:  b5fe34348aa !  8:  b79befc82be builtin/maintenance: let tasks do maintenance before and after detach
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    builtin/maintenance: let tasks do maintenance before and after detach
    +    builtin/maintenance: split into foreground and background tasks
     
         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
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/gc.c ##
    -@@ builtin/gc.c: typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
    +@@ builtin/gc.c: static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
    + 
    + typedef int (*maintenance_task_fn)(struct maintenance_run_opts *opts,
    + 				   struct gc_config *cfg);
    +-
    +-/*
    +- * An auto condition function returns 1 if the task should run
    +- * and 0 if the task should NOT run. See needs_to_gc() for an
    +- * example.
    +- */
    + 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;
    ++
    ++	/*
    ++	 * Work that will be executed before detaching. This should not include
    ++	 * tasks that may run for an extended amount of time as it does cause
    ++	 * auto-maintenance to block until foreground tasks have been run.
    ++	 */
    ++	maintenance_task_fn foreground;
    ++
    ++	/*
    ++	 * Work that will be executed after detaching. When not detaching the
    ++	 * work will be run in the foreground, as well.
    ++	 */
    ++	maintenance_task_fn background;
    ++
    ++	/*
    ++	 * An auto condition function returns 1 if the task should run and 0 if
    ++	 * the task should NOT run. See needs_to_gc() for an example.
    ++	 */
      	maintenance_auto_fn auto_condition;
      };
      
    @@ builtin/gc.c: typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
      	[TASK_PREFETCH] = {
      		.name = "prefetch",
     -		.fn = maintenance_task_prefetch,
    -+		.after_detach = maintenance_task_prefetch,
    ++		.background = maintenance_task_prefetch,
      	},
      	[TASK_LOOSE_OBJECTS] = {
      		.name = "loose-objects",
     -		.fn = maintenance_task_loose_objects,
    -+		.after_detach = maintenance_task_loose_objects,
    ++		.background = 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,
    ++		.background = maintenance_task_incremental_repack,
      		.auto_condition = incremental_repack_auto_condition,
      	},
      	[TASK_GC] = {
      		.name = "gc",
     -		.fn = maintenance_task_gc,
    -+		.after_detach = maintenance_task_gc,
    ++		.background = 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,
    ++		.background = 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,
    ++		.background = 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,
    ++		.background = 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,
    ++		.background = 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,
    ++		.background = maintenance_task_rerere_gc,
      		.auto_condition = rerere_gc_condition,
      	},
      };
      
     +enum task_phase {
    -+	TASK_PHASE_BEFORE_DETACH,
    -+	TASK_PHASE_AFTER_DETACH,
    ++	TASK_PHASE_FOREGROUND,
    ++	TASK_PHASE_BACKGROUND,
     +};
     +
      static int maybe_run_task(const struct maintenance_task *task,
    @@ builtin/gc.c: typedef int (*maintenance_auto_fn)(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 foreground = (phase == TASK_PHASE_FOREGROUND);
    ++	maintenance_task_fn fn = foreground ? task->foreground : task->background;
    ++	const char *region = foreground ? "maintenance foreground" : "maintenance";
      	int ret = 0;
      
     +	if (!fn)
    @@ builtin/gc.c: 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,
    -+				   TASK_PHASE_BEFORE_DETACH))
    ++				   TASK_PHASE_FOREGROUND))
     +			result = 1;
     +
      	/* Failure to daemonize is ok, we'll continue in foreground. */
    @@ builtin/gc.c: 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))
    ++				   TASK_PHASE_BACKGROUND))
      			result = 1;
      
      	rollback_lock_file(&lk);
 9:  79766b0afa0 <  -:  ----------- builtin/maintenance: fix locking race when packing refs and reflogs
 -:  ----------- >  9:  8b3f82b871d builtin/maintenance: fix locking race when packing refs and reflogs
10:  d74a6a3fe7f = 10:  7c61970f4aa usage: allow dying without writing an error message
11:  f8e77819f6e = 11:  39a1aa9dcb0 builtin/gc: avoid global state in `gc_before_repack()`
12:  31007c209cb ! 12:  8b263d0fc2b builtin/maintenance: fix locking race when handling "gc" task
    @@ Commit message
         for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix
         this by splitting up the logic of the "gc" task:
     
    -      - Before detaching we execute `gc_before_repack()`, which contains the
    -        logic that git-gc(1) itself would execute before detaching.
    +      - We execute `gc_before_repack()` in the foreground, which contains
    +        the logic that git-gc(1) itself would execute in the foreground, as
    +        well.
     
    -      - After detaching we spawn git-gc(1), but with a new hidden flag that
    +      - We spawn git-gc(1) after detaching, but with a new hidden flag that
             suppresses calling `gc_before_repack()`.
     
         Like this we have roughly the same logic as git-gc(1) itself and know to
         repack refs and reflogs before detaching, thus fixing the race.
     
    -    Note that `gc_before_repack()` is renamed to `gc_before_detach()` to
    +    Note that `gc_before_repack()` is renamed to `gc_foreground_tasks()` to
         better reflect what this function does.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
    @@ builtin/gc.c: static int report_last_gc_error(void)
      }
      
     -static int gc_before_repack(struct maintenance_run_opts *opts,
    -+static int gc_before_detach(struct maintenance_run_opts *opts,
    - 			    struct gc_config *cfg)
    +-			    struct gc_config *cfg)
    ++static int gc_foreground_tasks(struct maintenance_run_opts *opts,
    ++			       struct gc_config *cfg)
      {
      	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
    + 		return error(FAILED_RUN, "pack-refs");
     @@ builtin/gc.c: int cmd_gc(int argc,
      	pid_t pid;
      	int daemonized = 0;
      	int keep_largest_pack = -1;
    -+	int skip_maintenance_before_detach = 0;
    ++	int skip_foreground_tasks = 0;
      	timestamp_t dummy;
      	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
      	struct gc_config cfg = GC_CONFIG_INIT;
    @@ builtin/gc.c: int cmd_gc(int argc,
      			 N_("repack all other packs except the largest pack")),
      		OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"),
      			   N_("pack prefix to store a pack containing pruned objects")),
    -+		OPT_HIDDEN_BOOL(0, "skip-maintenance-before-detach", &skip_maintenance_before_detach,
    -+			   N_("skip maintenance steps typically done before detaching")),
    ++		OPT_HIDDEN_BOOL(0, "skip-foreground-tasks", &skip_foreground_tasks,
    ++			   N_("skip maintenance tasks typically done in the foreground")),
      		OPT_END()
      	};
      
    @@ builtin/gc.c: int cmd_gc(int argc,
     -			ret = 0;
     -			goto out;
     -		}
    -+		if (!skip_maintenance_before_detach) {
    ++		if (!skip_foreground_tasks) {
     +			if (lock_repo_for_gc(force, &pid)) {
     +				ret = 0;
     +				goto out;
    @@ builtin/gc.c: int cmd_gc(int argc,
     -		if (gc_before_repack(&opts, &cfg) < 0)
     -			die(NULL);
     -		delete_tempfile(&pidfile);
    -+			if (gc_before_detach(&opts, &cfg) < 0)
    ++			if (gc_foreground_tasks(&opts, &cfg) < 0)
     +				die(NULL);
     +			delete_tempfile(&pidfile);
     +		}
    @@ builtin/gc.c: int cmd_gc(int argc,
      
     -	if (opts.detach <= 0)
     -		gc_before_repack(&opts, &cfg);
    -+	if (opts.detach <= 0 && !skip_maintenance_before_detach)
    -+		gc_before_detach(&opts, &cfg);
    ++	if (opts.detach <= 0 && !skip_foreground_tasks)
    ++		gc_foreground_tasks(&opts, &cfg);
      
      	if (!repository_format_precious_objects) {
      		struct child_process repack_cmd = CHILD_PROCESS_INIT;
    @@ builtin/gc.c: static int maintenance_task_prefetch(struct maintenance_run_opts *
      
     -static int maintenance_task_gc(struct maintenance_run_opts *opts,
     -			       struct gc_config *cfg UNUSED)
    -+static int maintenance_task_gc_before_detach(struct maintenance_run_opts *opts,
    -+					     struct gc_config *cfg)
    ++static int maintenance_task_gc_foreground(struct maintenance_run_opts *opts,
    ++					  struct gc_config *cfg)
     +{
    -+	return gc_before_detach(opts, cfg);
    ++	return gc_foreground_tasks(opts, cfg);
     +}
     +
    -+static int maintenance_task_gc_after_detach(struct maintenance_run_opts *opts,
    -+					    struct gc_config *cfg UNUSED)
    ++static int maintenance_task_gc_background(struct maintenance_run_opts *opts,
    ++					  struct gc_config *cfg UNUSED)
      {
      	struct child_process child = CHILD_PROCESS_INIT;
      
    @@ builtin/gc.c: static int maintenance_task_gc(struct maintenance_run_opts *opts,
      	else
      		strvec_push(&child.args, "--no-quiet");
      	strvec_push(&child.args, "--no-detach");
    -+	strvec_push(&child.args, "--skip-maintenance-before-detach");
    ++	strvec_push(&child.args, "--skip-foreground-tasks");
      
      	return run_command(&child);
      }
    @@ builtin/gc.c: static const struct maintenance_task tasks[] = {
      	},
      	[TASK_GC] = {
      		.name = "gc",
    --		.after_detach = maintenance_task_gc,
    -+		.before_detach = maintenance_task_gc_before_detach,
    -+		.after_detach = maintenance_task_gc_after_detach,
    +-		.background = maintenance_task_gc,
    ++		.foreground = maintenance_task_gc_foreground,
    ++		.background = maintenance_task_gc_background,
      		.auto_condition = need_to_gc,
      	},
      	[TASK_COMMIT_GRAPH] = {
    @@ t/t7900-maintenance.sh: test_expect_success 'run [--auto|--quiet]' '
     -	test_subcommand git gc --quiet --no-detach <run-no-auto.txt &&
     -	test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
     -	test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
    -+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-no-auto.txt &&
    -+	test_subcommand ! git gc --auto --quiet --no-detach --skip-maintenance-before-detach <run-auto.txt &&
    -+	test_subcommand git gc --no-quiet --no-detach --skip-maintenance-before-detach <run-no-quiet.txt
    ++	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-no-auto.txt &&
    ++	test_subcommand ! git gc --auto --quiet --no-detach --skip-foreground-tasks <run-auto.txt &&
    ++	test_subcommand git gc --no-quiet --no-detach --skip-foreground-tasks <run-no-quiet.txt
      '
      
      test_expect_success 'maintenance.auto config option' '
    @@ t/t7900-maintenance.sh: test_expect_success 'run --task=<task>' '
     -	test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt &&
     -	test_subcommand git gc --quiet --no-detach <run-gc.txt &&
     -	test_subcommand git gc --quiet --no-detach <run-both.txt &&
    -+	test_subcommand ! git gc --quiet --no-detach --skip-maintenance-before-detach <run-commit-graph.txt &&
    -+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-gc.txt &&
    -+	test_subcommand git gc --quiet --no-detach --skip-maintenance-before-detach <run-both.txt &&
    ++	test_subcommand ! git gc --quiet --no-detach --skip-foreground-tasks <run-commit-graph.txt &&
    ++	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-gc.txt &&
    ++	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-both.txt &&
      	test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
      	test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
      	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt

---
base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
change-id: 20250527-b4-pks-maintenance-ref-lock-race-11ae5d68e06f

[PATCH v3 01/12] builtin/gc: use designated field initializers for maintenance tasks

[Patrick Steinhardt]

Convert the array of maintenance tasks to use designated field
initializers. This makes it easier to add more fields to the struct
without having to modify all tasks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 54 +++++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e33ba946e43..54fc7f299a9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1550,49 +1550,49 @@ enum maintenance_task_label {
 
 static struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
-		"prefetch",
-		maintenance_task_prefetch,
+		.name = "prefetch",
+		.fn = maintenance_task_prefetch,
 	},
 	[TASK_LOOSE_OBJECTS] = {
-		"loose-objects",
-		maintenance_task_loose_objects,
-		loose_object_auto_condition,
+		.name = "loose-objects",
+		.fn = maintenance_task_loose_objects,
+		.auto_condition = loose_object_auto_condition,
 	},
 	[TASK_INCREMENTAL_REPACK] = {
-		"incremental-repack",
-		maintenance_task_incremental_repack,
-		incremental_repack_auto_condition,
+		.name = "incremental-repack",
+		.fn = maintenance_task_incremental_repack,
+		.auto_condition = incremental_repack_auto_condition,
 	},
 	[TASK_GC] = {
-		"gc",
-		maintenance_task_gc,
-		need_to_gc,
-		1,
+		.name = "gc",
+		.fn = maintenance_task_gc,
+		.auto_condition = need_to_gc,
+		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
-		"commit-graph",
-		maintenance_task_commit_graph,
-		should_write_commit_graph,
+		.name = "commit-graph",
+		.fn = maintenance_task_commit_graph,
+		.auto_condition = should_write_commit_graph,
 	},
 	[TASK_PACK_REFS] = {
-		"pack-refs",
-		maintenance_task_pack_refs,
-		pack_refs_condition,
+		.name = "pack-refs",
+		.fn = maintenance_task_pack_refs,
+		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
-		"reflog-expire",
-		maintenance_task_reflog_expire,
-		reflog_expire_condition,
+		.name = "reflog-expire",
+		.fn = maintenance_task_reflog_expire,
+		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {
-		"worktree-prune",
-		maintenance_task_worktree_prune,
-		worktree_prune_condition,
+		.name = "worktree-prune",
+		.fn = maintenance_task_worktree_prune,
+		.auto_condition = worktree_prune_condition,
 	},
 	[TASK_RERERE_GC] = {
-		"rerere-gc",
-		maintenance_task_rerere_gc,
-		rerere_gc_condition,
+		.name = "rerere-gc",
+		.fn = maintenance_task_rerere_gc,
+		.auto_condition = rerere_gc_condition,
 	},
 };
 

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 02/12] builtin/gc: drop redundant local variable

[Patrick Steinhardt]

We have two different variables that track the quietness for git-gc(1):

  - The local variable `quiet`, which we wire up.

  - The `quiet` field of `struct maintenance_run_opts`.

This leads to confusion which of these variables should be used and what
the respective effect is.

Simplify this logic by dropping the local variable in favor of the
options field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 54fc7f299a9..7adda8d2d0d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -818,7 +818,6 @@ int cmd_gc(int argc,
 	   struct repository *repo UNUSED)
 {
 	int aggressive = 0;
-	int quiet = 0;
 	int force = 0;
 	const char *name;
 	pid_t pid;
@@ -831,7 +830,7 @@ int cmd_gc(int argc,
 	const char *prune_expire_arg = prune_expire_sentinel;
 	int ret;
 	struct option builtin_gc_options[] = {
-		OPT__QUIET(&quiet, N_("suppress progress reporting")),
+		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		{
 			.type = OPTION_STRING,
 			.long_name = "prune",
@@ -891,7 +890,7 @@ int cmd_gc(int argc,
 		if (cfg.aggressive_window > 0)
 			strvec_pushf(&repack, "--window=%d", cfg.aggressive_window);
 	}
-	if (quiet)
+	if (opts.quiet)
 		strvec_push(&repack, "-q");
 
 	if (opts.auto_flag) {
@@ -906,7 +905,7 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (!quiet) {
+		if (!opts.quiet) {
 			if (opts.detach > 0)
 				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
 			else
@@ -991,7 +990,7 @@ int cmd_gc(int argc,
 			strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL);
 			/* run `git prune` even if using cruft packs */
 			strvec_push(&prune_cmd.args, cfg.prune_expire);
-			if (quiet)
+			if (opts.quiet)
 				strvec_push(&prune_cmd.args, "--no-progress");
 			if (repo_has_promisor_remote(the_repository))
 				strvec_push(&prune_cmd.args,
@@ -1019,7 +1018,7 @@ int cmd_gc(int argc,
 
 	if (the_repository->settings.gc_write_commit_graph == 1)
 		write_commit_graph_reachable(the_repository->objects->odb,
-					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
+					     !opts.quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
 					     NULL);
 
 	if (opts.auto_flag && too_many_loose_objects(&cfg))

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 03/12] builtin/maintenance: centralize configuration of explicit tasks

[Patrick Steinhardt]

Users of git-maintenance(1) can explicitly ask it to run specific tasks
by passing the `--task=` command line option. This option can be passed
multiple times, which causes us to execute tasks in the same order as
the tasks have been provided by the user.

The order in which tasks are run is computed in `task_option_parse()`:
every time we parse such a command line argument, we modify the global
array of tasks by seting the selected index for that specific task.
This has two downsides:

  - We modify global state, which makes it hard to follow the logic.

  - The configuration of tasks is split across multiple different
    functions, so it is not easy to figure out the different factors
    that play a role in selecting tasks.

Refactor the logic so that `task_option_parse()` does not modify global
state anymore. Instead, this function now only collects the list of
configured tasks. The logic to configure ordering of the respective
tasks is then deferred to `initialize_task_config()`.

This refactoring solves the second problem, that the configuration of
tasks is spread across multiple different locations. The first problem,
that we modify global state, will be fixed in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7adda8d2d0d..c4af9b11287 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1690,15 +1690,22 @@ static void initialize_maintenance_strategy(void)
 	}
 }
 
-static void initialize_task_config(int schedule)
+static void initialize_task_config(const struct string_list *selected_tasks,
+				   int schedule)
 {
-	int i;
 	struct strbuf config_name = STRBUF_INIT;
 
+	for (size_t i = 0; i < TASK__COUNT; i++)
+		tasks[i].selected_order = -1;
+	for (size_t i = 0; i < selected_tasks->nr; i++) {
+		struct maintenance_task *task = selected_tasks->items[i].util;
+		task->selected_order = i;
+	}
+
 	if (schedule)
 		initialize_maintenance_strategy();
 
-	for (i = 0; i < TASK__COUNT; i++) {
+	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
 		char *config_str;
 
@@ -1722,33 +1729,28 @@ static void initialize_task_config(int schedule)
 	strbuf_release(&config_name);
 }
 
-static int task_option_parse(const struct option *opt UNUSED,
+static int task_option_parse(const struct option *opt,
 			     const char *arg, int unset)
 {
-	int i, num_selected = 0;
-	struct maintenance_task *task = NULL;
+	struct string_list *selected_tasks = opt->value;
+	size_t i;
 
 	BUG_ON_OPT_NEG(unset);
 
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (tasks[i].selected_order >= 0)
-			num_selected++;
-		if (!strcasecmp(tasks[i].name, arg)) {
-			task = &tasks[i];
-		}
-	}
-
-	if (!task) {
+	for (i = 0; i < TASK__COUNT; i++)
+		if (!strcasecmp(tasks[i].name, arg))
+			break;
+	if (i >= TASK__COUNT) {
 		error(_("'%s' is not a valid task"), arg);
 		return 1;
 	}
 
-	if (task->selected_order >= 0) {
+	if (unsorted_string_list_has_string(selected_tasks, arg)) {
 		error(_("task '%s' cannot be selected multiple times"), arg);
 		return 1;
 	}
 
-	task->selected_order = num_selected + 1;
+	string_list_append(selected_tasks, arg)->util = &tasks[i];
 
 	return 0;
 }
@@ -1756,8 +1758,8 @@ static int task_option_parse(const struct option *opt UNUSED,
 static int maintenance_run(int argc, const char **argv, const char *prefix,
 			   struct repository *repo UNUSED)
 {
-	int i;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
+	struct string_list selected_tasks = STRING_LIST_INIT_DUP;
 	struct gc_config cfg = GC_CONFIG_INIT;
 	struct option builtin_maintenance_run_options[] = {
 		OPT_BOOL(0, "auto", &opts.auto_flag,
@@ -1769,7 +1771,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     maintenance_opt_schedule),
 		OPT_BOOL(0, "quiet", &opts.quiet,
 			 N_("do not report progress or other information over stderr")),
-		OPT_CALLBACK_F(0, "task", NULL, N_("task"),
+		OPT_CALLBACK_F(0, "task", &selected_tasks, N_("task"),
 			N_("run a specific task"),
 			PARSE_OPT_NONEG, task_option_parse),
 		OPT_END()
@@ -1778,9 +1780,6 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 
 	opts.quiet = !isatty(2);
 
-	for (i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_run_options,
 			     builtin_maintenance_run_usage,
@@ -1790,13 +1789,15 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 		die(_("use at most one of --auto and --schedule=<frequency>"));
 
 	gc_config(&cfg);
-	initialize_task_config(opts.schedule);
+	initialize_task_config(&selected_tasks, opts.schedule);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
 				   builtin_maintenance_run_options);
 
 	ret = maintenance_run_tasks(&opts, &cfg);
+
+	string_list_clear(&selected_tasks, 0);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 04/12] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible

[Patrick Steinhardt]

The "--task=" option explicitly allows the user to say which maintenance
tasks should be run, whereas "--schedule=" only respects the maintenance
strategy configured for a specific repository. As such, it is not
sensible to accept both options at the same time.

Mark them as incompatible with one another. While at it, also convert
the existing logic that marks "--auto" and "--schedule=" as incompatible
to use `die_for_incompatible_opt2()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 6 ++++--
 t/t7900-maintenance.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c4af9b11287..57d7602596a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1785,8 +1785,10 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     builtin_maintenance_run_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (opts.auto_flag && opts.schedule)
-		die(_("use at most one of --auto and --schedule=<frequency>"));
+	die_for_incompatible_opt2(opts.auto_flag, "--auto",
+				  opts.schedule, "--schedule=");
+	die_for_incompatible_opt2(selected_tasks.nr, "--task=",
+				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
 	initialize_task_config(&selected_tasks, opts.schedule);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 8cf89e285f4..1ada5246606 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -610,7 +610,12 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
 
 test_expect_success '--auto and --schedule incompatible' '
 	test_must_fail git maintenance run --auto --schedule=daily 2>err &&
-	test_grep "at most one" err
+	test_grep "cannot be used together" err
+'
+
+test_expect_success '--task and --schedule incompatible' '
+	test_must_fail git maintenance run --task=pack-refs --schedule=daily 2>err &&
+	test_grep "cannot be used together" err
 '
 
 test_expect_success 'invalid --schedule value' '

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 05/12] builtin/maintenance: stop modifying global array of tasks

[Patrick Steinhardt]

When configuring maintenance tasks run by git-maintenance(1) we do so by
modifying the global array of tasks directly. This is already quite bad
on its own, as global state makes for logic that is hard to follow.

Even more importantly though we use multiple different fields to track
whether or not a task should be run:

  - "enabled" tracks the "maintenance.*.enabled" config key. This field
    disables execution of a task, unless the user has explicitly asked
    for the task.

  - "selected_order" tracks the order in which jobs have been asked for
    by the user via the "--task=" command line option. It overrides
    everything else, but only has an effect if at least one job has been
    selected.

  - "schedule" tracks the schedule priority for a job, that is how often
    it should run. This field only plays a role when the user has passed
    the "--schedule=" command line option.

All of this makes it non-trivial to figure out which job really should
be running right now. The logic to configure these fields and the logic
that interprets them is distributed across multiple functions, making it
even harder to follow it.

Refactor the logic so that we stop modifying global state. Instead, we
now compute which jobs should be run in `initialize_task_config()`,
represented as an array of jobs to run that is stored in the options
structure. Like this, all logic becomes self-contained and any users of
this array only need to iterate through the tasks and execute them one
by one.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 206 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 112 insertions(+), 94 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 57d7602596a..4d636237cac 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -251,7 +251,24 @@ static enum schedule_priority parse_schedule(const char *value)
 	return SCHEDULE_NONE;
 }
 
+enum maintenance_task_label {
+	TASK_PREFETCH,
+	TASK_LOOSE_OBJECTS,
+	TASK_INCREMENTAL_REPACK,
+	TASK_GC,
+	TASK_COMMIT_GRAPH,
+	TASK_PACK_REFS,
+	TASK_REFLOG_EXPIRE,
+	TASK_WORKTREE_PRUNE,
+	TASK_RERERE_GC,
+
+	/* Leave as final value */
+	TASK__COUNT
+};
+
 struct maintenance_run_opts {
+	enum maintenance_task_label *tasks;
+	size_t tasks_nr, tasks_alloc;
 	int auto_flag;
 	int detach;
 	int quiet;
@@ -261,6 +278,11 @@ struct maintenance_run_opts {
 	.detach = -1, \
 }
 
+static void maintenance_run_opts_release(struct maintenance_run_opts *opts)
+{
+	free(opts->tasks);
+}
+
 static int pack_refs_condition(UNUSED struct gc_config *cfg)
 {
 	/*
@@ -1032,6 +1054,7 @@ int cmd_gc(int argc,
 	}
 
 out:
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return 0;
 }
@@ -1524,30 +1547,9 @@ struct maintenance_task {
 	const char *name;
 	maintenance_task_fn *fn;
 	maintenance_auto_fn *auto_condition;
-	unsigned enabled:1;
-
-	enum schedule_priority schedule;
-
-	/* -1 if not selected. */
-	int selected_order;
-};
-
-enum maintenance_task_label {
-	TASK_PREFETCH,
-	TASK_LOOSE_OBJECTS,
-	TASK_INCREMENTAL_REPACK,
-	TASK_GC,
-	TASK_COMMIT_GRAPH,
-	TASK_PACK_REFS,
-	TASK_REFLOG_EXPIRE,
-	TASK_WORKTREE_PRUNE,
-	TASK_RERERE_GC,
-
-	/* Leave as final value */
-	TASK__COUNT
 };
 
-static struct maintenance_task tasks[] = {
+static const struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
 		.name = "prefetch",
 		.fn = maintenance_task_prefetch,
@@ -1566,7 +1568,6 @@ static struct maintenance_task tasks[] = {
 		.name = "gc",
 		.fn = maintenance_task_gc,
 		.auto_condition = need_to_gc,
-		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
 		.name = "commit-graph",
@@ -1595,18 +1596,9 @@ static struct maintenance_task tasks[] = {
 	},
 };
 
-static int compare_tasks_by_selection(const void *a_, const void *b_)
-{
-	const struct maintenance_task *a = a_;
-	const struct maintenance_task *b = b_;
-
-	return b->selected_order - a->selected_order;
-}
-
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
-	int i, found_selected = 0;
 	int result = 0;
 	struct lock_file lk;
 	struct repository *r = the_repository;
@@ -1635,95 +1627,120 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (i = 0; !found_selected && i < TASK__COUNT; i++)
-		found_selected = tasks[i].selected_order >= 0;
-
-	if (found_selected)
-		QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);
-
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (found_selected && tasks[i].selected_order < 0)
-			continue;
-
-		if (!found_selected && !tasks[i].enabled)
-			continue;
-
+	for (size_t i = 0; i < opts->tasks_nr; i++) {
 		if (opts->auto_flag &&
-		    (!tasks[i].auto_condition ||
-		     !tasks[i].auto_condition(cfg)))
-			continue;
-
-		if (opts->schedule && tasks[i].schedule < opts->schedule)
+		    (!tasks[opts->tasks[i]].auto_condition ||
+		     !tasks[opts->tasks[i]].auto_condition(cfg)))
 			continue;
 
-		trace2_region_enter("maintenance", tasks[i].name, r);
-		if (tasks[i].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[i].name);
+		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
+		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
+			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
 			result = 1;
 		}
-		trace2_region_leave("maintenance", tasks[i].name, r);
+		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
 	}
 
 	rollback_lock_file(&lk);
 	return result;
 }
 
-static void initialize_maintenance_strategy(void)
+struct maintenance_strategy {
+	struct {
+		int enabled;
+		enum schedule_priority schedule;
+	} tasks[TASK__COUNT];
+};
+
+static const struct maintenance_strategy none_strategy = { 0 };
+static const struct maintenance_strategy default_strategy = {
+	.tasks = {
+		[TASK_GC].enabled = 1,
+	},
+};
+static const struct maintenance_strategy incremental_strategy = {
+	.tasks = {
+		[TASK_COMMIT_GRAPH].enabled = 1,
+		[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY,
+		[TASK_PREFETCH].enabled = 1,
+		[TASK_PREFETCH].schedule = SCHEDULE_HOURLY,
+		[TASK_INCREMENTAL_REPACK].enabled = 1,
+		[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY,
+		[TASK_LOOSE_OBJECTS].enabled = 1,
+		[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY,
+		[TASK_PACK_REFS].enabled = 1,
+		[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY,
+	},
+};
+
+static void initialize_task_config(struct maintenance_run_opts *opts,
+				   const struct string_list *selected_tasks)
 {
+	struct strbuf config_name = STRBUF_INIT;
+	struct maintenance_strategy strategy;
 	const char *config_str;
 
-	if (git_config_get_string_tmp("maintenance.strategy", &config_str))
-		return;
+	/*
+	 * In case the user has asked us to run tasks explicitly we only use
+	 * those specified tasks. Specifically, we do _not_ want to consult the
+	 * config or maintenance strategy.
+	 */
+	if (selected_tasks->nr) {
+		for (size_t i = 0; i < selected_tasks->nr; i++) {
+			enum maintenance_task_label label = (intptr_t)selected_tasks->items[i].util;;
+			ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+			opts->tasks[opts->tasks_nr++] = label;
+		}
 
-	if (!strcasecmp(config_str, "incremental")) {
-		tasks[TASK_GC].schedule = SCHEDULE_NONE;
-		tasks[TASK_COMMIT_GRAPH].enabled = 1;
-		tasks[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_PREFETCH].enabled = 1;
-		tasks[TASK_PREFETCH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_INCREMENTAL_REPACK].enabled = 1;
-		tasks[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY;
-		tasks[TASK_LOOSE_OBJECTS].enabled = 1;
-		tasks[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY;
-		tasks[TASK_PACK_REFS].enabled = 1;
-		tasks[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY;
+		return;
 	}
-}
 
-static void initialize_task_config(const struct string_list *selected_tasks,
-				   int schedule)
-{
-	struct strbuf config_name = STRBUF_INIT;
+	/*
+	 * Otherwise, the strategy depends on whether we run as part of a
+	 * scheduled job or not:
+	 *
+	 *   - Scheduled maintenance does not perform any housekeeping by
+	 *     default, but requires the user to pick a maintenance strategy.
+	 *
+	 *   - Unscheduled maintenance uses our default strategy.
+	 *
+	 * Both of these are affected by the gitconfig though, which may
+	 * override specific aspects of our strategy.
+	 */
+	if (opts->schedule) {
+		strategy = none_strategy;
 
-	for (size_t i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-	for (size_t i = 0; i < selected_tasks->nr; i++) {
-		struct maintenance_task *task = selected_tasks->items[i].util;
-		task->selected_order = i;
+		if (!git_config_get_string_tmp("maintenance.strategy", &config_str)) {
+			if (!strcasecmp(config_str, "incremental"))
+				strategy = incremental_strategy;
+		}
+	} else {
+		strategy = default_strategy;
 	}
 
-	if (schedule)
-		initialize_maintenance_strategy();
-
 	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
-		char *config_str;
 
 		strbuf_reset(&config_name);
 		strbuf_addf(&config_name, "maintenance.%s.enabled",
 			    tasks[i].name);
-
 		if (!git_config_get_bool(config_name.buf, &config_value))
-			tasks[i].enabled = config_value;
-
-		strbuf_reset(&config_name);
-		strbuf_addf(&config_name, "maintenance.%s.schedule",
-			    tasks[i].name);
+			strategy.tasks[i].enabled = config_value;
+		if (!strategy.tasks[i].enabled)
+			continue;
 
-		if (!git_config_get_string(config_name.buf, &config_str)) {
-			tasks[i].schedule = parse_schedule(config_str);
-			free(config_str);
+		if (opts->schedule) {
+			strbuf_reset(&config_name);
+			strbuf_addf(&config_name, "maintenance.%s.schedule",
+				    tasks[i].name);
+			if (!git_config_get_string_tmp(config_name.buf, &config_str))
+				strategy.tasks[i].schedule = parse_schedule(config_str);
+			if (strategy.tasks[i].schedule < opts->schedule)
+				continue;
 		}
+
+		ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+		opts->tasks[opts->tasks_nr++] = i;
 	}
 
 	strbuf_release(&config_name);
@@ -1750,7 +1767,7 @@ static int task_option_parse(const struct option *opt,
 		return 1;
 	}
 
-	string_list_append(selected_tasks, arg)->util = &tasks[i];
+	string_list_append(selected_tasks, arg)->util = (void *)(intptr_t)i;
 
 	return 0;
 }
@@ -1791,7 +1808,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
-	initialize_task_config(&selected_tasks, opts.schedule);
+	initialize_task_config(&opts, &selected_tasks);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
@@ -1800,6 +1817,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 	ret = maintenance_run_tasks(&opts, &cfg);
 
 	string_list_clear(&selected_tasks, 0);
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 06/12] builtin/maintenance: extract function to run tasks

[Patrick Steinhardt]

Extract the function to run maintenance tasks. This function will be
reused in a subsequent commit where we introduce a split between
maintenance tasks that run before and after daemonizing the process.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4d636237cac..cfbf9d8a2b9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1596,6 +1596,27 @@ 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)
+{
+	int ret = 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)) {
+		error(_("task '%s' failed"), task->name);
+		ret = 1;
+	}
+	trace2_region_leave("maintenance", task->name, repo);
+
+	return ret;
+}
+
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
@@ -1627,19 +1648,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (size_t i = 0; i < opts->tasks_nr; i++) {
-		if (opts->auto_flag &&
-		    (!tasks[opts->tasks[i]].auto_condition ||
-		     !tasks[opts->tasks[i]].auto_condition(cfg)))
-			continue;
-
-		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
-		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
+	for (size_t i = 0; i < opts->tasks_nr; i++)
+		if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg))
 			result = 1;
-		}
-		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
-	}
 
 	rollback_lock_file(&lk);
 	return result;

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 07/12] builtin/maintenance: fix typedef for function pointers

[Patrick Steinhardt]

The typedefs for `maintenance_task_fn` and `maintenance_auto_fn` are
somewhat confusingly not true function pointers. As such, any user of
those typedefs needs to manually add the pointer to make use of them.

Fix this by making these true function pointers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cfbf9d8a2b9..447e5800846 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1533,20 +1533,20 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
 	return 0;
 }
 
-typedef int maintenance_task_fn(struct maintenance_run_opts *opts,
-				struct gc_config *cfg);
+typedef int (*maintenance_task_fn)(struct maintenance_run_opts *opts,
+				   struct gc_config *cfg);
 
 /*
  * An auto condition function returns 1 if the task should run
  * and 0 if the task should NOT run. See needs_to_gc() for an
  * example.
  */
-typedef int maintenance_auto_fn(struct gc_config *cfg);
+typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
 
 struct maintenance_task {
 	const char *name;
-	maintenance_task_fn *fn;
-	maintenance_auto_fn *auto_condition;
+	maintenance_task_fn fn;
+	maintenance_auto_fn auto_condition;
 };
 
 static const struct maintenance_task tasks[] = {

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 08/12] builtin/maintenance: split into foreground and background tasks

[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
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 | 70 ++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 21 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 447e5800846..72a695853e5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1535,84 +1535,106 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
 
 typedef int (*maintenance_task_fn)(struct maintenance_run_opts *opts,
 				   struct gc_config *cfg);
-
-/*
- * An auto condition function returns 1 if the task should run
- * and 0 if the task should NOT run. See needs_to_gc() for an
- * example.
- */
 typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
 
 struct maintenance_task {
 	const char *name;
-	maintenance_task_fn fn;
+
+	/*
+	 * Work that will be executed before detaching. This should not include
+	 * tasks that may run for an extended amount of time as it does cause
+	 * auto-maintenance to block until foreground tasks have been run.
+	 */
+	maintenance_task_fn foreground;
+
+	/*
+	 * Work that will be executed after detaching. When not detaching the
+	 * work will be run in the foreground, as well.
+	 */
+	maintenance_task_fn background;
+
+	/*
+	 * An auto condition function returns 1 if the task should run and 0 if
+	 * the task should NOT run. See needs_to_gc() for an example.
+	 */
 	maintenance_auto_fn auto_condition;
 };
 
 static const struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
 		.name = "prefetch",
-		.fn = maintenance_task_prefetch,
+		.background = maintenance_task_prefetch,
 	},
 	[TASK_LOOSE_OBJECTS] = {
 		.name = "loose-objects",
-		.fn = maintenance_task_loose_objects,
+		.background = maintenance_task_loose_objects,
 		.auto_condition = loose_object_auto_condition,
 	},
 	[TASK_INCREMENTAL_REPACK] = {
 		.name = "incremental-repack",
-		.fn = maintenance_task_incremental_repack,
+		.background = maintenance_task_incremental_repack,
 		.auto_condition = incremental_repack_auto_condition,
 	},
 	[TASK_GC] = {
 		.name = "gc",
-		.fn = maintenance_task_gc,
+		.background = maintenance_task_gc,
 		.auto_condition = need_to_gc,
 	},
 	[TASK_COMMIT_GRAPH] = {
 		.name = "commit-graph",
-		.fn = maintenance_task_commit_graph,
+		.background = maintenance_task_commit_graph,
 		.auto_condition = should_write_commit_graph,
 	},
 	[TASK_PACK_REFS] = {
 		.name = "pack-refs",
-		.fn = maintenance_task_pack_refs,
+		.background = maintenance_task_pack_refs,
 		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
 		.name = "reflog-expire",
-		.fn = maintenance_task_reflog_expire,
+		.background = maintenance_task_reflog_expire,
 		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {
 		.name = "worktree-prune",
-		.fn = maintenance_task_worktree_prune,
+		.background = maintenance_task_worktree_prune,
 		.auto_condition = worktree_prune_condition,
 	},
 	[TASK_RERERE_GC] = {
 		.name = "rerere-gc",
-		.fn = maintenance_task_rerere_gc,
+		.background = maintenance_task_rerere_gc,
 		.auto_condition = rerere_gc_condition,
 	},
 };
 
+enum task_phase {
+	TASK_PHASE_FOREGROUND,
+	TASK_PHASE_BACKGROUND,
+};
+
 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 foreground = (phase == TASK_PHASE_FOREGROUND);
+	maintenance_task_fn fn = foreground ? task->foreground : task->background;
+	const char *region = foreground ? "maintenance foreground" : "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 +1663,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_FOREGROUND))
+			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 +1676,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_BACKGROUND))
 			result = 1;
 
 	rollback_lock_file(&lk);

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 09/12] builtin/maintenance: fix locking race when packing refs and reflogs

[Patrick Steinhardt]

As explained in the preceding commit, git-gc(1) knows to detach only
after it has already packed references and reflogs. This is done to
avoid racing around their respective lockfiles.

Adapt git-maintenance(1) accordingly and run the "pack-refs" and
"reflog-expire" tasks in the foreground. Note that the "gc" task has the
same issue, but the fix is a bit more involved there and will thus be
done in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 72a695853e5..fdd0dd09be7 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1587,12 +1587,12 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_PACK_REFS] = {
 		.name = "pack-refs",
-		.background = maintenance_task_pack_refs,
+		.foreground = maintenance_task_pack_refs,
 		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
 		.name = "reflog-expire",
-		.background = maintenance_task_reflog_expire,
+		.foreground = maintenance_task_reflog_expire,
 		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 10/12] usage: allow dying without writing an error message

[Patrick Steinhardt]

Sometimes code wants to die in a situation where it already has written
an error message. To use the same error code as `die()` we have to open
code the code with a call to `exit(128)` in such cases, which is easy to
get wrong and leaves magical numbers all over our codebase.

Teach `die_message_builtin()` to not print any error when passed a
`NULL` pointer as error string. Like this, such users can now call
`die(NULL)` to achieve the same result without any hardcoded error
codes.

Adapt a couple of builtins to use this new pattern to demonstrate that
there is a need for such a helper.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/am.c                |  4 ++--
 builtin/checkout.c          |  4 ++--
 builtin/fetch.c             |  2 +-
 builtin/submodule--helper.c | 12 ++++++------
 usage.c                     |  2 ++
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index e32a3b4c973..a800003340f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1000,7 +1000,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 
 	if (!patch_format) {
 		fprintf_ln(stderr, _("Patch format detection failed."));
-		exit(128);
+		die(NULL);
 	}
 
 	if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
@@ -1178,7 +1178,7 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 		strbuf_release(&sb);
 	}
 
-	exit(128);
+	die(NULL);
 }
 
 /**
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d185982f3a6..536192d3456 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -838,7 +838,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		init_tree_desc(&trees[0], &tree->object.oid,
 			       tree->buffer, tree->size);
 		if (parse_tree(new_tree) < 0)
-			exit(128);
+			die(NULL);
 		tree = new_tree;
 		init_tree_desc(&trees[1], &tree->object.oid,
 			       tree->buffer, tree->size);
@@ -913,7 +913,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 						     work,
 						     old_tree);
 			if (ret < 0)
-				exit(128);
+				die(NULL);
 			ret = reset_tree(new_tree,
 					 opts, 0,
 					 writeout_error, new_branch_info);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index cda6eaf1fd6..b0800ea5829 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -992,7 +992,7 @@ static int update_local_ref(struct ref *ref,
 		fast_forward = repo_in_merge_bases(the_repository, current,
 						   updated);
 		if (fast_forward < 0)
-			exit(128);
+			die(NULL);
 		forced_updates_ms += (getnanotime() - t_before) / 1000000;
 	} else {
 		fast_forward = 1;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116ddf..4255caca579 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -303,7 +303,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 	char *displaypath;
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	displaypath = get_submodule_displaypath(path, info->prefix,
 						info->super_prefix);
@@ -643,7 +643,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	};
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	if (!submodule_from_path(the_repository, null_oid(the_hash_algo), path))
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -1257,7 +1257,7 @@ static void sync_submodule(const char *path, const char *prefix,
 		return;
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), path);
 
@@ -1402,7 +1402,7 @@ static void deinit_submodule(const char *path, const char *prefix,
 	char *sub_git_dir = xstrfmt("%s/.git", path);
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), path);
 
@@ -1724,7 +1724,7 @@ static int clone_submodule(const struct module_clone_data *clone_data,
 	char *to_free = NULL;
 
 	if (validate_submodule_path(clone_data_path) < 0)
-		exit(128);
+		die(NULL);
 
 	if (!is_absolute_path(clone_data->path))
 		clone_data_path = to_free = xstrfmt("%s/%s", repo_get_work_tree(the_repository),
@@ -3524,7 +3524,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	strip_dir_trailing_slashes(add_data.sm_path);
 
 	if (validate_submodule_path(add_data.sm_path) < 0)
-		exit(128);
+		die(NULL);
 
 	die_on_index_match(add_data.sm_path, force);
 	die_on_repo_without_commits(add_data.sm_path);
diff --git a/usage.c b/usage.c
index 38b46bbbfe7..cd7b57d6446 100644
--- a/usage.c
+++ b/usage.c
@@ -67,6 +67,8 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 
 static void die_message_builtin(const char *err, va_list params)
 {
+	if (!err)
+		return;
 	trace2_cmd_error_va(err, params);
 	vreportf(_("fatal: "), err, params);
 }

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 11/12] builtin/gc: avoid global state in `gc_before_repack()`

[Patrick Steinhardt]

The `gc_before_repack()` should only ever run once in git-gc(1), but we
may end up calling it twice when the "--detach" flag is passed. The
duplicated call is avoided though via a static flag in this function.

This pattern is somewhat unintuitive though. Refactor it to drop the
static flag and instead guard the second call of `gc_before_repack()`
via `opts.detach`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index fdd0dd09be7..4a5c4b20442 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,22 +816,14 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static void gc_before_repack(struct maintenance_run_opts *opts,
-			     struct gc_config *cfg)
+static int gc_before_repack(struct maintenance_run_opts *opts,
+			    struct gc_config *cfg)
 {
-	/*
-	 * We may be called twice, as both the pre- and
-	 * post-daemonized phases will call us, but running these
-	 * commands more than once is pointless and wasteful.
-	 */
-	static int done = 0;
-	if (done++)
-		return;
-
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
-		die(FAILED_RUN, "pack-refs");
+		return error(FAILED_RUN, "pack-refs");
 	if (cfg->prune_reflogs && maintenance_task_reflog_expire(opts, cfg))
-		die(FAILED_RUN, "reflog");
+		return error(FAILED_RUN, "reflog");
+	return 0;
 }
 
 int cmd_gc(int argc,
@@ -965,7 +957,8 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		gc_before_repack(&opts, &cfg); /* dies on failure */
+		if (gc_before_repack(&opts, &cfg) < 0)
+			die(NULL);
 		delete_tempfile(&pidfile);
 
 		/*
@@ -995,7 +988,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0)
+		gc_before_repack(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v3 12/12] builtin/maintenance: fix locking race when handling "gc" task

[Patrick Steinhardt]

The "gc" task has a similar locking race as the one that we have fixed
for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix
this by splitting up the logic of the "gc" task:

  - We execute `gc_before_repack()` in the foreground, which contains
    the logic that git-gc(1) itself would execute in the foreground, as
    well.

  - We spawn git-gc(1) after detaching, but with a new hidden flag that
    suppresses calling `gc_before_repack()`.

Like this we have roughly the same logic as git-gc(1) itself and know to
repack refs and reflogs before detaching, thus fixing the race.

Note that `gc_before_repack()` is renamed to `gc_foreground_tasks()` to
better reflect what this function does.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 41 +++++++++++++++++++++++++++--------------
 t/t7900-maintenance.sh | 12 ++++++------
 2 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4a5c4b20442..b5e6519d597 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,8 +816,8 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static int gc_before_repack(struct maintenance_run_opts *opts,
-			    struct gc_config *cfg)
+static int gc_foreground_tasks(struct maintenance_run_opts *opts,
+			       struct gc_config *cfg)
 {
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
 		return error(FAILED_RUN, "pack-refs");
@@ -837,6 +837,7 @@ int cmd_gc(int argc,
 	pid_t pid;
 	int daemonized = 0;
 	int keep_largest_pack = -1;
+	int skip_foreground_tasks = 0;
 	timestamp_t dummy;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
 	struct gc_config cfg = GC_CONFIG_INIT;
@@ -869,6 +870,8 @@ int cmd_gc(int argc,
 			 N_("repack all other packs except the largest pack")),
 		OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"),
 			   N_("pack prefix to store a pack containing pruned objects")),
+		OPT_HIDDEN_BOOL(0, "skip-foreground-tasks", &skip_foreground_tasks,
+			   N_("skip maintenance tasks typically done in the foreground")),
 		OPT_END()
 	};
 
@@ -952,14 +955,16 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (lock_repo_for_gc(force, &pid)) {
-			ret = 0;
-			goto out;
-		}
+		if (!skip_foreground_tasks) {
+			if (lock_repo_for_gc(force, &pid)) {
+				ret = 0;
+				goto out;
+			}
 
-		if (gc_before_repack(&opts, &cfg) < 0)
-			die(NULL);
-		delete_tempfile(&pidfile);
+			if (gc_foreground_tasks(&opts, &cfg) < 0)
+				die(NULL);
+			delete_tempfile(&pidfile);
+		}
 
 		/*
 		 * failure to daemonize is ok, we'll continue
@@ -988,8 +993,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	if (opts.detach <= 0)
-		gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0 && !skip_foreground_tasks)
+		gc_foreground_tasks(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;
@@ -1225,8 +1230,14 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,
 	return 0;
 }
 
-static int maintenance_task_gc(struct maintenance_run_opts *opts,
-			       struct gc_config *cfg UNUSED)
+static int maintenance_task_gc_foreground(struct maintenance_run_opts *opts,
+					  struct gc_config *cfg)
+{
+	return gc_foreground_tasks(opts, cfg);
+}
+
+static int maintenance_task_gc_background(struct maintenance_run_opts *opts,
+					  struct gc_config *cfg UNUSED)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
@@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts,
 	else
 		strvec_push(&child.args, "--no-quiet");
 	strvec_push(&child.args, "--no-detach");
+	strvec_push(&child.args, "--skip-foreground-tasks");
 
 	return run_command(&child);
 }
@@ -1571,7 +1583,8 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_GC] = {
 		.name = "gc",
-		.background = maintenance_task_gc,
+		.foreground = maintenance_task_gc_foreground,
+		.background = maintenance_task_gc_background,
 		.auto_condition = need_to_gc,
 	},
 	[TASK_COMMIT_GRAPH] = {
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 1ada5246606..ddd273d8dc2 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -49,9 +49,9 @@ test_expect_success 'run [--auto|--quiet]' '
 		git maintenance run --auto 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \
 		git maintenance run --no-quiet 2>/dev/null &&
-	test_subcommand git gc --quiet --no-detach <run-no-auto.txt &&
-	test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
-	test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
+	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-no-auto.txt &&
+	test_subcommand ! git gc --auto --quiet --no-detach --skip-foreground-tasks <run-auto.txt &&
+	test_subcommand git gc --no-quiet --no-detach --skip-foreground-tasks <run-no-quiet.txt
 '
 
 test_expect_success 'maintenance.auto config option' '
@@ -154,9 +154,9 @@ test_expect_success 'run --task=<task>' '
 		git maintenance run --task=commit-graph 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \
 		git maintenance run --task=commit-graph --task=gc 2>/dev/null &&
-	test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt &&
-	test_subcommand git gc --quiet --no-detach <run-gc.txt &&
-	test_subcommand git gc --quiet --no-detach <run-both.txt &&
+	test_subcommand ! git gc --quiet --no-detach --skip-foreground-tasks <run-commit-graph.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-gc.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-both.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
 	test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

Re: [PATCH v3 09/12] builtin/maintenance: fix locking race when packing refs and reflogs

[Kristoffer Haugsbakk]

On Mon, Jun 2, 2025, at 09:17, Patrick Steinhardt wrote:
> As explained in the preceding commit, git-gc(1) knows to detach only
> after it has already packed references and reflogs. This is done to

I’m a naïve reader.  When I read this I immediately thought that reflogs
can be packed now.  But going by the last paragraph it is packed
references and expired reflogs?

> avoid racing around their respective lockfiles.
>
> Adapt git-maintenance(1) accordingly and run the "pack-refs" and
> "reflog-expire" tasks in the foreground. Note that the "gc" task has the
> same issue, but the fix is a bit more involved there and will thus be
> done in a subsequent commit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Re: [PATCH v3 01/12] builtin/gc: use designated field initializers for maintenance tasks

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Convert the array of maintenance tasks to use designated field
> initializers. This makes it easier to add more fields to the struct
> without having to modify all tasks.
>

I was wondering why we don't catch missing field initializers when not
using designators. Seems like we explicitly set
'-Wno-missing-field-initializers'. Perhaps something to slowly tackle
over time.

The patch looks good.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3 09/12] builtin/maintenance: fix locking race when packing refs and reflogs

[Patrick Steinhardt]

On Mon, Jun 02, 2025 at 12:03:21PM +0200, Kristoffer Haugsbakk wrote:
> On Mon, Jun 2, 2025, at 09:17, Patrick Steinhardt wrote:
> > As explained in the preceding commit, git-gc(1) knows to detach only
> > after it has already packed references and reflogs. This is done to
> 
> I’m a naïve reader.  When I read this I immediately thought that reflogs
> can be packed now.  But going by the last paragraph it is packed
> references and expired reflogs?

Yeah, I see how this is misleading. But indeed, it is packing refs and
expiring reflogs. I've rephrased this locally, but if that's okay with
you I'll hold off sending a new version for just this change.

Thanks!

Patrick

Re: [PATCH v3 09/12] builtin/maintenance: fix locking race when packing refs and reflogs

[Kristoffer Haugsbakk]

On Tue, Jun 3, 2025, at 08:34, Patrick Steinhardt wrote:
>> [snip]
>> I’m a naïve reader.  When I read this I immediately thought that reflogs
>> can be packed now.  But going by the last paragraph it is packed
>> references and expired reflogs?
>
> Yeah, I see how this is misleading. But indeed, it is packing refs and
> expiring reflogs. I've rephrased this locally, but if that's okay with
> you I'll hold off sending a new version for just this change.

Certainly!  I forgot to prefix: nit

-- 
Kristoffer Haugsbakk

 

Re: [PATCH v3 10/12] usage: allow dying without writing an error message

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Sometimes code wants to die in a situation where it already has written
> an error message. To use the same error code as `die()` we have to open
> code the code with a call to `exit(128)` in such cases, which is easy to

Nit: This reads a little weird.

> get wrong and leaves magical numbers all over our codebase.
>
> Teach `die_message_builtin()` to not print any error when passed a
> `NULL` pointer as error string. Like this, such users can now call
> `die(NULL)` to achieve the same result without any hardcoded error
> codes.
>
> Adapt a couple of builtins to use this new pattern to demonstrate that
> there is a need for such a helper.
>

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3 10/12] usage: allow dying without writing an error message

[Kristoffer Haugsbakk]

On Tue, Jun 3, 2025, at 10:31, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> Sometimes code wants to die in a situation where it already has written
>> an error message. To use the same error code as `die()` we have to open
>> code the code with a call to `exit(128)` in such cases, which is easy to
>
> Nit: This reads a little weird.

Maybe s/to open code the code/to open-code the code/

Re: [PATCH v3 10/12] usage: allow dying without writing an error message

[Karthik Nayak]

On Tue, Jun 3, 2025 at 10:34 AM Kristoffer Haugsbakk
<kristofferhaugsbakk@fastmail.com> wrote:
>
> On Tue, Jun 3, 2025, at 10:31, Karthik Nayak wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> Sometimes code wants to die in a situation where it already has written
> >> an error message. To use the same error code as `die()` we have to open
> >> code the code with a call to `exit(128)` in such cases, which is easy to
> >
> > Nit: This reads a little weird.
>
> Maybe s/to open code the code/to open-code the code/

Ah, that makes so much difference.

Re: [PATCH v3 10/12] usage: allow dying without writing an error message

[Patrick Steinhardt]

On Tue, Jun 03, 2025 at 11:04:56AM +0200, Karthik Nayak wrote:
> On Tue, Jun 3, 2025 at 10:34 AM Kristoffer Haugsbakk
> <kristofferhaugsbakk@fastmail.com> wrote:
> >
> > On Tue, Jun 3, 2025, at 10:31, Karthik Nayak wrote:
> > > Patrick Steinhardt <ps@pks.im> writes:
> > >
> > >> Sometimes code wants to die in a situation where it already has written
> > >> an error message. To use the same error code as `die()` we have to open
> > >> code the code with a call to `exit(128)` in such cases, which is easy to
> > >
> > > Nit: This reads a little weird.
> >
> > Maybe s/to open code the code/to open-code the code/
> 
> Ah, that makes so much difference.

That reads better, but I guess we can improve it even further. How about
this instead:

    To retain the same error code as `die()` we have to use `exit(128)`,
    which is easy to get wrong and leaves magical numbers all over our
    codebase.

Patrick

Re: [PATCH v3 10/12] usage: allow dying without writing an error message

[Kristoffer Haugsbakk]

On Tue, Jun 3, 2025, at 12:46, Patrick Steinhardt wrote:
>> Ah, that makes so much difference.
>
> That reads better, but I guess we can improve it even further. How about
> this instead:
>
>     To retain the same error code as `die()` we have to use `exit(128)`,
>     which is easy to get wrong and leaves magical numbers all over our
>     codebase.

Looks great.  Just s/magical numbers/magic numbers/

[PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching

[Patrick Steinhardt]

Hi,

this patch series fixes races around locking the "packed-refs" file when
auto-maintenance decides to repack it. This issue has been reported e.g.
via [1] and [2].

The root cause is that git-gc(1) used to know to detach _after_ having
repacked references. As such, callers wouldn't continue with their thing
until we have already packed refs, and thus the race does not exist
there. git-maintenance(1) didn't have the same split though, so this
patch series retrofits that logic.

The series is structured as follows:

  - Patches 1 and 2 do some light refactorings.

  - Patches 3 to 5 refactor how we set up the list of tasks to not rely
    on globals anymore. Instead, we now have a single source of truth
    for which tasks exactly will be run.

  - The remaining patches introduce the split of before/after-detach
    tasks and wire them up for "pack-refs", "reflog-expire" and "gc"
    tasks.

Changes in v2:
  - A couple of commit message improvements.
  - Introduce `die(NULL)` to die with the correct exit code but no error
    message. This gets rid of some magic numbers.
  - Introduce an enum to discern the phases before and after detach.
  - Link to v1: https://lore.kernel.org/r/20250527-b4-pks-maintenance-ref-lock-race-v1-0-e1ceb2dea66e@pks.im

Changes in v3:
  - Rework logic to talk about foreground/background tasks instead of
    before/after detach.
  - Link to v2: https://lore.kernel.org/r/20250530-b4-pks-maintenance-ref-lock-race-v2-0-d04e2f93e51f@pks.im

Changes in v4:
  - Some more massaging of commit messages.
  - Link to v3: https://lore.kernel.org/r/20250602-b4-pks-maintenance-ref-lock-race-v3-0-587d44252dcb@pks.im

Thanks!

Patrick

[1]: <CAJR-fbZ4X1+gN75m2dUvocR6NkowLOZ9F26cjBy8w1qd181OoQ@mail.gmail.com>
[2]: <CANi7bVAkNc+gY1NoXfJuDRjxjZLTgL8Lfn8_ZmWsvLAoiLPkNg@mail.gmail.com>

---
Patrick Steinhardt (12):
      builtin/gc: use designated field initializers for maintenance tasks
      builtin/gc: drop redundant local variable
      builtin/maintenance: centralize configuration of explicit tasks
      builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
      builtin/maintenance: stop modifying global array of tasks
      builtin/maintenance: extract function to run tasks
      builtin/maintenance: fix typedef for function pointers
      builtin/maintenance: split into foreground and background tasks
      builtin/maintenance: fix locking race with refs and reflogs tasks
      usage: allow dying without writing an error message
      builtin/gc: avoid global state in `gc_before_repack()`
      builtin/maintenance: fix locking race when handling "gc" task

 builtin/am.c                |   4 +-
 builtin/checkout.c          |   4 +-
 builtin/fetch.c             |   2 +-
 builtin/gc.c                | 410 +++++++++++++++++++++++++-------------------
 builtin/submodule--helper.c |  12 +-
 t/t7900-maintenance.sh      |  19 +-
 usage.c                     |   2 +
 7 files changed, 263 insertions(+), 190 deletions(-)

Range-diff versus v3:

 1:  e46a65951b9 =  1:  280f13d2895 builtin/gc: use designated field initializers for maintenance tasks
 2:  73cd67f3e1a =  2:  16a017fb819 builtin/gc: drop redundant local variable
 3:  a02452a6d6f =  3:  0ab3344ddb0 builtin/maintenance: centralize configuration of explicit tasks
 4:  ccd7691e4d5 =  4:  69e768cb54e builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
 5:  0e243fd81e6 =  5:  295e9e5ee9f builtin/maintenance: stop modifying global array of tasks
 6:  c95bd62823e =  6:  d94b0c86622 builtin/maintenance: extract function to run tasks
 7:  43d28434d8e =  7:  0bbba671cd0 builtin/maintenance: fix typedef for function pointers
 8:  d5740a5c9d9 =  8:  4ce38539bb6 builtin/maintenance: split into foreground and background tasks
 9:  168eb3a9372 !  9:  28092b9bed1 builtin/maintenance: fix locking race when packing refs and reflogs
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    builtin/maintenance: fix locking race when packing refs and reflogs
    +    builtin/maintenance: fix locking race with refs and reflogs tasks
     
         As explained in the preceding commit, git-gc(1) knows to detach only
    -    after it has already packed references and reflogs. This is done to
    -    avoid racing around their respective lockfiles.
    +    after it has already packed references and expired reflogs. This is done
    +    to avoid racing around their respective lockfiles.
     
         Adapt git-maintenance(1) accordingly and run the "pack-refs" and
         "reflog-expire" tasks in the foreground. Note that the "gc" task has the
10:  0ff01f6e2aa ! 10:  b8ed080c67d usage: allow dying without writing an error message
    @@ Commit message
         usage: allow dying without writing an error message
     
         Sometimes code wants to die in a situation where it already has written
    -    an error message. To use the same error code as `die()` we have to open
    -    code the code with a call to `exit(128)` in such cases, which is easy to
    -    get wrong and leaves magical numbers all over our codebase.
    +    an error message. To use the same error code as `die()` we have to use
    +    `exit(128)`, which is easy to get wrong and leaves magic numbers all
    +    over our codebase.
     
         Teach `die_message_builtin()` to not print any error when passed a
         `NULL` pointer as error string. Like this, such users can now call
11:  93f53000e47 = 11:  5b149886263 builtin/gc: avoid global state in `gc_before_repack()`
12:  01095d1bf88 = 12:  9ba01f143b3 builtin/maintenance: fix locking race when handling "gc" task

---
base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
change-id: 20250527-b4-pks-maintenance-ref-lock-race-11ae5d68e06f

[PATCH v4 01/12] builtin/gc: use designated field initializers for maintenance tasks

[Patrick Steinhardt]

Convert the array of maintenance tasks to use designated field
initializers. This makes it easier to add more fields to the struct
without having to modify all tasks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 54 +++++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e33ba946e43..54fc7f299a9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1550,49 +1550,49 @@ enum maintenance_task_label {
 
 static struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
-		"prefetch",
-		maintenance_task_prefetch,
+		.name = "prefetch",
+		.fn = maintenance_task_prefetch,
 	},
 	[TASK_LOOSE_OBJECTS] = {
-		"loose-objects",
-		maintenance_task_loose_objects,
-		loose_object_auto_condition,
+		.name = "loose-objects",
+		.fn = maintenance_task_loose_objects,
+		.auto_condition = loose_object_auto_condition,
 	},
 	[TASK_INCREMENTAL_REPACK] = {
-		"incremental-repack",
-		maintenance_task_incremental_repack,
-		incremental_repack_auto_condition,
+		.name = "incremental-repack",
+		.fn = maintenance_task_incremental_repack,
+		.auto_condition = incremental_repack_auto_condition,
 	},
 	[TASK_GC] = {
-		"gc",
-		maintenance_task_gc,
-		need_to_gc,
-		1,
+		.name = "gc",
+		.fn = maintenance_task_gc,
+		.auto_condition = need_to_gc,
+		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
-		"commit-graph",
-		maintenance_task_commit_graph,
-		should_write_commit_graph,
+		.name = "commit-graph",
+		.fn = maintenance_task_commit_graph,
+		.auto_condition = should_write_commit_graph,
 	},
 	[TASK_PACK_REFS] = {
-		"pack-refs",
-		maintenance_task_pack_refs,
-		pack_refs_condition,
+		.name = "pack-refs",
+		.fn = maintenance_task_pack_refs,
+		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
-		"reflog-expire",
-		maintenance_task_reflog_expire,
-		reflog_expire_condition,
+		.name = "reflog-expire",
+		.fn = maintenance_task_reflog_expire,
+		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {
-		"worktree-prune",
-		maintenance_task_worktree_prune,
-		worktree_prune_condition,
+		.name = "worktree-prune",
+		.fn = maintenance_task_worktree_prune,
+		.auto_condition = worktree_prune_condition,
 	},
 	[TASK_RERERE_GC] = {
-		"rerere-gc",
-		maintenance_task_rerere_gc,
-		rerere_gc_condition,
+		.name = "rerere-gc",
+		.fn = maintenance_task_rerere_gc,
+		.auto_condition = rerere_gc_condition,
 	},
 };
 

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 02/12] builtin/gc: drop redundant local variable

[Patrick Steinhardt]

We have two different variables that track the quietness for git-gc(1):

  - The local variable `quiet`, which we wire up.

  - The `quiet` field of `struct maintenance_run_opts`.

This leads to confusion which of these variables should be used and what
the respective effect is.

Simplify this logic by dropping the local variable in favor of the
options field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 54fc7f299a9..7adda8d2d0d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -818,7 +818,6 @@ int cmd_gc(int argc,
 	   struct repository *repo UNUSED)
 {
 	int aggressive = 0;
-	int quiet = 0;
 	int force = 0;
 	const char *name;
 	pid_t pid;
@@ -831,7 +830,7 @@ int cmd_gc(int argc,
 	const char *prune_expire_arg = prune_expire_sentinel;
 	int ret;
 	struct option builtin_gc_options[] = {
-		OPT__QUIET(&quiet, N_("suppress progress reporting")),
+		OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		{
 			.type = OPTION_STRING,
 			.long_name = "prune",
@@ -891,7 +890,7 @@ int cmd_gc(int argc,
 		if (cfg.aggressive_window > 0)
 			strvec_pushf(&repack, "--window=%d", cfg.aggressive_window);
 	}
-	if (quiet)
+	if (opts.quiet)
 		strvec_push(&repack, "-q");
 
 	if (opts.auto_flag) {
@@ -906,7 +905,7 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (!quiet) {
+		if (!opts.quiet) {
 			if (opts.detach > 0)
 				fprintf(stderr, _("Auto packing the repository in background for optimum performance.\n"));
 			else
@@ -991,7 +990,7 @@ int cmd_gc(int argc,
 			strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL);
 			/* run `git prune` even if using cruft packs */
 			strvec_push(&prune_cmd.args, cfg.prune_expire);
-			if (quiet)
+			if (opts.quiet)
 				strvec_push(&prune_cmd.args, "--no-progress");
 			if (repo_has_promisor_remote(the_repository))
 				strvec_push(&prune_cmd.args,
@@ -1019,7 +1018,7 @@ int cmd_gc(int argc,
 
 	if (the_repository->settings.gc_write_commit_graph == 1)
 		write_commit_graph_reachable(the_repository->objects->odb,
-					     !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
+					     !opts.quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0,
 					     NULL);
 
 	if (opts.auto_flag && too_many_loose_objects(&cfg))

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 03/12] builtin/maintenance: centralize configuration of explicit tasks

[Patrick Steinhardt]

Users of git-maintenance(1) can explicitly ask it to run specific tasks
by passing the `--task=` command line option. This option can be passed
multiple times, which causes us to execute tasks in the same order as
the tasks have been provided by the user.

The order in which tasks are run is computed in `task_option_parse()`:
every time we parse such a command line argument, we modify the global
array of tasks by seting the selected index for that specific task.
This has two downsides:

  - We modify global state, which makes it hard to follow the logic.

  - The configuration of tasks is split across multiple different
    functions, so it is not easy to figure out the different factors
    that play a role in selecting tasks.

Refactor the logic so that `task_option_parse()` does not modify global
state anymore. Instead, this function now only collects the list of
configured tasks. The logic to configure ordering of the respective
tasks is then deferred to `initialize_task_config()`.

This refactoring solves the second problem, that the configuration of
tasks is spread across multiple different locations. The first problem,
that we modify global state, will be fixed in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 7adda8d2d0d..c4af9b11287 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1690,15 +1690,22 @@ static void initialize_maintenance_strategy(void)
 	}
 }
 
-static void initialize_task_config(int schedule)
+static void initialize_task_config(const struct string_list *selected_tasks,
+				   int schedule)
 {
-	int i;
 	struct strbuf config_name = STRBUF_INIT;
 
+	for (size_t i = 0; i < TASK__COUNT; i++)
+		tasks[i].selected_order = -1;
+	for (size_t i = 0; i < selected_tasks->nr; i++) {
+		struct maintenance_task *task = selected_tasks->items[i].util;
+		task->selected_order = i;
+	}
+
 	if (schedule)
 		initialize_maintenance_strategy();
 
-	for (i = 0; i < TASK__COUNT; i++) {
+	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
 		char *config_str;
 
@@ -1722,33 +1729,28 @@ static void initialize_task_config(int schedule)
 	strbuf_release(&config_name);
 }
 
-static int task_option_parse(const struct option *opt UNUSED,
+static int task_option_parse(const struct option *opt,
 			     const char *arg, int unset)
 {
-	int i, num_selected = 0;
-	struct maintenance_task *task = NULL;
+	struct string_list *selected_tasks = opt->value;
+	size_t i;
 
 	BUG_ON_OPT_NEG(unset);
 
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (tasks[i].selected_order >= 0)
-			num_selected++;
-		if (!strcasecmp(tasks[i].name, arg)) {
-			task = &tasks[i];
-		}
-	}
-
-	if (!task) {
+	for (i = 0; i < TASK__COUNT; i++)
+		if (!strcasecmp(tasks[i].name, arg))
+			break;
+	if (i >= TASK__COUNT) {
 		error(_("'%s' is not a valid task"), arg);
 		return 1;
 	}
 
-	if (task->selected_order >= 0) {
+	if (unsorted_string_list_has_string(selected_tasks, arg)) {
 		error(_("task '%s' cannot be selected multiple times"), arg);
 		return 1;
 	}
 
-	task->selected_order = num_selected + 1;
+	string_list_append(selected_tasks, arg)->util = &tasks[i];
 
 	return 0;
 }
@@ -1756,8 +1758,8 @@ static int task_option_parse(const struct option *opt UNUSED,
 static int maintenance_run(int argc, const char **argv, const char *prefix,
 			   struct repository *repo UNUSED)
 {
-	int i;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
+	struct string_list selected_tasks = STRING_LIST_INIT_DUP;
 	struct gc_config cfg = GC_CONFIG_INIT;
 	struct option builtin_maintenance_run_options[] = {
 		OPT_BOOL(0, "auto", &opts.auto_flag,
@@ -1769,7 +1771,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     maintenance_opt_schedule),
 		OPT_BOOL(0, "quiet", &opts.quiet,
 			 N_("do not report progress or other information over stderr")),
-		OPT_CALLBACK_F(0, "task", NULL, N_("task"),
+		OPT_CALLBACK_F(0, "task", &selected_tasks, N_("task"),
 			N_("run a specific task"),
 			PARSE_OPT_NONEG, task_option_parse),
 		OPT_END()
@@ -1778,9 +1780,6 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 
 	opts.quiet = !isatty(2);
 
-	for (i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-
 	argc = parse_options(argc, argv, prefix,
 			     builtin_maintenance_run_options,
 			     builtin_maintenance_run_usage,
@@ -1790,13 +1789,15 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 		die(_("use at most one of --auto and --schedule=<frequency>"));
 
 	gc_config(&cfg);
-	initialize_task_config(opts.schedule);
+	initialize_task_config(&selected_tasks, opts.schedule);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
 				   builtin_maintenance_run_options);
 
 	ret = maintenance_run_tasks(&opts, &cfg);
+
+	string_list_clear(&selected_tasks, 0);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 04/12] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible

[Patrick Steinhardt]

The "--task=" option explicitly allows the user to say which maintenance
tasks should be run, whereas "--schedule=" only respects the maintenance
strategy configured for a specific repository. As such, it is not
sensible to accept both options at the same time.

Mark them as incompatible with one another. While at it, also convert
the existing logic that marks "--auto" and "--schedule=" as incompatible
to use `die_for_incompatible_opt2()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 6 ++++--
 t/t7900-maintenance.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c4af9b11287..57d7602596a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1785,8 +1785,10 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 			     builtin_maintenance_run_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (opts.auto_flag && opts.schedule)
-		die(_("use at most one of --auto and --schedule=<frequency>"));
+	die_for_incompatible_opt2(opts.auto_flag, "--auto",
+				  opts.schedule, "--schedule=");
+	die_for_incompatible_opt2(selected_tasks.nr, "--task=",
+				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
 	initialize_task_config(&selected_tasks, opts.schedule);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 8cf89e285f4..1ada5246606 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -610,7 +610,12 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
 
 test_expect_success '--auto and --schedule incompatible' '
 	test_must_fail git maintenance run --auto --schedule=daily 2>err &&
-	test_grep "at most one" err
+	test_grep "cannot be used together" err
+'
+
+test_expect_success '--task and --schedule incompatible' '
+	test_must_fail git maintenance run --task=pack-refs --schedule=daily 2>err &&
+	test_grep "cannot be used together" err
 '
 
 test_expect_success 'invalid --schedule value' '

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 05/12] builtin/maintenance: stop modifying global array of tasks

[Patrick Steinhardt]

When configuring maintenance tasks run by git-maintenance(1) we do so by
modifying the global array of tasks directly. This is already quite bad
on its own, as global state makes for logic that is hard to follow.

Even more importantly though we use multiple different fields to track
whether or not a task should be run:

  - "enabled" tracks the "maintenance.*.enabled" config key. This field
    disables execution of a task, unless the user has explicitly asked
    for the task.

  - "selected_order" tracks the order in which jobs have been asked for
    by the user via the "--task=" command line option. It overrides
    everything else, but only has an effect if at least one job has been
    selected.

  - "schedule" tracks the schedule priority for a job, that is how often
    it should run. This field only plays a role when the user has passed
    the "--schedule=" command line option.

All of this makes it non-trivial to figure out which job really should
be running right now. The logic to configure these fields and the logic
that interprets them is distributed across multiple functions, making it
even harder to follow it.

Refactor the logic so that we stop modifying global state. Instead, we
now compute which jobs should be run in `initialize_task_config()`,
represented as an array of jobs to run that is stored in the options
structure. Like this, all logic becomes self-contained and any users of
this array only need to iterate through the tasks and execute them one
by one.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 206 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 112 insertions(+), 94 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 57d7602596a..4d636237cac 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -251,7 +251,24 @@ static enum schedule_priority parse_schedule(const char *value)
 	return SCHEDULE_NONE;
 }
 
+enum maintenance_task_label {
+	TASK_PREFETCH,
+	TASK_LOOSE_OBJECTS,
+	TASK_INCREMENTAL_REPACK,
+	TASK_GC,
+	TASK_COMMIT_GRAPH,
+	TASK_PACK_REFS,
+	TASK_REFLOG_EXPIRE,
+	TASK_WORKTREE_PRUNE,
+	TASK_RERERE_GC,
+
+	/* Leave as final value */
+	TASK__COUNT
+};
+
 struct maintenance_run_opts {
+	enum maintenance_task_label *tasks;
+	size_t tasks_nr, tasks_alloc;
 	int auto_flag;
 	int detach;
 	int quiet;
@@ -261,6 +278,11 @@ struct maintenance_run_opts {
 	.detach = -1, \
 }
 
+static void maintenance_run_opts_release(struct maintenance_run_opts *opts)
+{
+	free(opts->tasks);
+}
+
 static int pack_refs_condition(UNUSED struct gc_config *cfg)
 {
 	/*
@@ -1032,6 +1054,7 @@ int cmd_gc(int argc,
 	}
 
 out:
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return 0;
 }
@@ -1524,30 +1547,9 @@ struct maintenance_task {
 	const char *name;
 	maintenance_task_fn *fn;
 	maintenance_auto_fn *auto_condition;
-	unsigned enabled:1;
-
-	enum schedule_priority schedule;
-
-	/* -1 if not selected. */
-	int selected_order;
-};
-
-enum maintenance_task_label {
-	TASK_PREFETCH,
-	TASK_LOOSE_OBJECTS,
-	TASK_INCREMENTAL_REPACK,
-	TASK_GC,
-	TASK_COMMIT_GRAPH,
-	TASK_PACK_REFS,
-	TASK_REFLOG_EXPIRE,
-	TASK_WORKTREE_PRUNE,
-	TASK_RERERE_GC,
-
-	/* Leave as final value */
-	TASK__COUNT
 };
 
-static struct maintenance_task tasks[] = {
+static const struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
 		.name = "prefetch",
 		.fn = maintenance_task_prefetch,
@@ -1566,7 +1568,6 @@ static struct maintenance_task tasks[] = {
 		.name = "gc",
 		.fn = maintenance_task_gc,
 		.auto_condition = need_to_gc,
-		.enabled = 1,
 	},
 	[TASK_COMMIT_GRAPH] = {
 		.name = "commit-graph",
@@ -1595,18 +1596,9 @@ static struct maintenance_task tasks[] = {
 	},
 };
 
-static int compare_tasks_by_selection(const void *a_, const void *b_)
-{
-	const struct maintenance_task *a = a_;
-	const struct maintenance_task *b = b_;
-
-	return b->selected_order - a->selected_order;
-}
-
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
-	int i, found_selected = 0;
 	int result = 0;
 	struct lock_file lk;
 	struct repository *r = the_repository;
@@ -1635,95 +1627,120 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (i = 0; !found_selected && i < TASK__COUNT; i++)
-		found_selected = tasks[i].selected_order >= 0;
-
-	if (found_selected)
-		QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);
-
-	for (i = 0; i < TASK__COUNT; i++) {
-		if (found_selected && tasks[i].selected_order < 0)
-			continue;
-
-		if (!found_selected && !tasks[i].enabled)
-			continue;
-
+	for (size_t i = 0; i < opts->tasks_nr; i++) {
 		if (opts->auto_flag &&
-		    (!tasks[i].auto_condition ||
-		     !tasks[i].auto_condition(cfg)))
-			continue;
-
-		if (opts->schedule && tasks[i].schedule < opts->schedule)
+		    (!tasks[opts->tasks[i]].auto_condition ||
+		     !tasks[opts->tasks[i]].auto_condition(cfg)))
 			continue;
 
-		trace2_region_enter("maintenance", tasks[i].name, r);
-		if (tasks[i].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[i].name);
+		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
+		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
+			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
 			result = 1;
 		}
-		trace2_region_leave("maintenance", tasks[i].name, r);
+		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
 	}
 
 	rollback_lock_file(&lk);
 	return result;
 }
 
-static void initialize_maintenance_strategy(void)
+struct maintenance_strategy {
+	struct {
+		int enabled;
+		enum schedule_priority schedule;
+	} tasks[TASK__COUNT];
+};
+
+static const struct maintenance_strategy none_strategy = { 0 };
+static const struct maintenance_strategy default_strategy = {
+	.tasks = {
+		[TASK_GC].enabled = 1,
+	},
+};
+static const struct maintenance_strategy incremental_strategy = {
+	.tasks = {
+		[TASK_COMMIT_GRAPH].enabled = 1,
+		[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY,
+		[TASK_PREFETCH].enabled = 1,
+		[TASK_PREFETCH].schedule = SCHEDULE_HOURLY,
+		[TASK_INCREMENTAL_REPACK].enabled = 1,
+		[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY,
+		[TASK_LOOSE_OBJECTS].enabled = 1,
+		[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY,
+		[TASK_PACK_REFS].enabled = 1,
+		[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY,
+	},
+};
+
+static void initialize_task_config(struct maintenance_run_opts *opts,
+				   const struct string_list *selected_tasks)
 {
+	struct strbuf config_name = STRBUF_INIT;
+	struct maintenance_strategy strategy;
 	const char *config_str;
 
-	if (git_config_get_string_tmp("maintenance.strategy", &config_str))
-		return;
+	/*
+	 * In case the user has asked us to run tasks explicitly we only use
+	 * those specified tasks. Specifically, we do _not_ want to consult the
+	 * config or maintenance strategy.
+	 */
+	if (selected_tasks->nr) {
+		for (size_t i = 0; i < selected_tasks->nr; i++) {
+			enum maintenance_task_label label = (intptr_t)selected_tasks->items[i].util;;
+			ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+			opts->tasks[opts->tasks_nr++] = label;
+		}
 
-	if (!strcasecmp(config_str, "incremental")) {
-		tasks[TASK_GC].schedule = SCHEDULE_NONE;
-		tasks[TASK_COMMIT_GRAPH].enabled = 1;
-		tasks[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_PREFETCH].enabled = 1;
-		tasks[TASK_PREFETCH].schedule = SCHEDULE_HOURLY;
-		tasks[TASK_INCREMENTAL_REPACK].enabled = 1;
-		tasks[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY;
-		tasks[TASK_LOOSE_OBJECTS].enabled = 1;
-		tasks[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY;
-		tasks[TASK_PACK_REFS].enabled = 1;
-		tasks[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY;
+		return;
 	}
-}
 
-static void initialize_task_config(const struct string_list *selected_tasks,
-				   int schedule)
-{
-	struct strbuf config_name = STRBUF_INIT;
+	/*
+	 * Otherwise, the strategy depends on whether we run as part of a
+	 * scheduled job or not:
+	 *
+	 *   - Scheduled maintenance does not perform any housekeeping by
+	 *     default, but requires the user to pick a maintenance strategy.
+	 *
+	 *   - Unscheduled maintenance uses our default strategy.
+	 *
+	 * Both of these are affected by the gitconfig though, which may
+	 * override specific aspects of our strategy.
+	 */
+	if (opts->schedule) {
+		strategy = none_strategy;
 
-	for (size_t i = 0; i < TASK__COUNT; i++)
-		tasks[i].selected_order = -1;
-	for (size_t i = 0; i < selected_tasks->nr; i++) {
-		struct maintenance_task *task = selected_tasks->items[i].util;
-		task->selected_order = i;
+		if (!git_config_get_string_tmp("maintenance.strategy", &config_str)) {
+			if (!strcasecmp(config_str, "incremental"))
+				strategy = incremental_strategy;
+		}
+	} else {
+		strategy = default_strategy;
 	}
 
-	if (schedule)
-		initialize_maintenance_strategy();
-
 	for (size_t i = 0; i < TASK__COUNT; i++) {
 		int config_value;
-		char *config_str;
 
 		strbuf_reset(&config_name);
 		strbuf_addf(&config_name, "maintenance.%s.enabled",
 			    tasks[i].name);
-
 		if (!git_config_get_bool(config_name.buf, &config_value))
-			tasks[i].enabled = config_value;
-
-		strbuf_reset(&config_name);
-		strbuf_addf(&config_name, "maintenance.%s.schedule",
-			    tasks[i].name);
+			strategy.tasks[i].enabled = config_value;
+		if (!strategy.tasks[i].enabled)
+			continue;
 
-		if (!git_config_get_string(config_name.buf, &config_str)) {
-			tasks[i].schedule = parse_schedule(config_str);
-			free(config_str);
+		if (opts->schedule) {
+			strbuf_reset(&config_name);
+			strbuf_addf(&config_name, "maintenance.%s.schedule",
+				    tasks[i].name);
+			if (!git_config_get_string_tmp(config_name.buf, &config_str))
+				strategy.tasks[i].schedule = parse_schedule(config_str);
+			if (strategy.tasks[i].schedule < opts->schedule)
+				continue;
 		}
+
+		ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);
+		opts->tasks[opts->tasks_nr++] = i;
 	}
 
 	strbuf_release(&config_name);
@@ -1750,7 +1767,7 @@ static int task_option_parse(const struct option *opt,
 		return 1;
 	}
 
-	string_list_append(selected_tasks, arg)->util = &tasks[i];
+	string_list_append(selected_tasks, arg)->util = (void *)(intptr_t)i;
 
 	return 0;
 }
@@ -1791,7 +1808,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 				  opts.schedule, "--schedule=");
 
 	gc_config(&cfg);
-	initialize_task_config(&selected_tasks, opts.schedule);
+	initialize_task_config(&opts, &selected_tasks);
 
 	if (argc != 0)
 		usage_with_options(builtin_maintenance_run_usage,
@@ -1800,6 +1817,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,
 	ret = maintenance_run_tasks(&opts, &cfg);
 
 	string_list_clear(&selected_tasks, 0);
+	maintenance_run_opts_release(&opts);
 	gc_config_release(&cfg);
 	return ret;
 }

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 06/12] builtin/maintenance: extract function to run tasks

[Patrick Steinhardt]

Extract the function to run maintenance tasks. This function will be
reused in a subsequent commit where we introduce a split between
maintenance tasks that run before and after daemonizing the process.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4d636237cac..cfbf9d8a2b9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1596,6 +1596,27 @@ 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)
+{
+	int ret = 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)) {
+		error(_("task '%s' failed"), task->name);
+		ret = 1;
+	}
+	trace2_region_leave("maintenance", task->name, repo);
+
+	return ret;
+}
+
 static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 				 struct gc_config *cfg)
 {
@@ -1627,19 +1648,9 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
 		trace2_region_leave("maintenance", "detach", the_repository);
 	}
 
-	for (size_t i = 0; i < opts->tasks_nr; i++) {
-		if (opts->auto_flag &&
-		    (!tasks[opts->tasks[i]].auto_condition ||
-		     !tasks[opts->tasks[i]].auto_condition(cfg)))
-			continue;
-
-		trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);
-		if (tasks[opts->tasks[i]].fn(opts, cfg)) {
-			error(_("task '%s' failed"), tasks[opts->tasks[i]].name);
+	for (size_t i = 0; i < opts->tasks_nr; i++)
+		if (maybe_run_task(&tasks[opts->tasks[i]], r, opts, cfg))
 			result = 1;
-		}
-		trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);
-	}
 
 	rollback_lock_file(&lk);
 	return result;

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 07/12] builtin/maintenance: fix typedef for function pointers

[Patrick Steinhardt]

The typedefs for `maintenance_task_fn` and `maintenance_auto_fn` are
somewhat confusingly not true function pointers. As such, any user of
those typedefs needs to manually add the pointer to make use of them.

Fix this by making these true function pointers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index cfbf9d8a2b9..447e5800846 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1533,20 +1533,20 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
 	return 0;
 }
 
-typedef int maintenance_task_fn(struct maintenance_run_opts *opts,
-				struct gc_config *cfg);
+typedef int (*maintenance_task_fn)(struct maintenance_run_opts *opts,
+				   struct gc_config *cfg);
 
 /*
  * An auto condition function returns 1 if the task should run
  * and 0 if the task should NOT run. See needs_to_gc() for an
  * example.
  */
-typedef int maintenance_auto_fn(struct gc_config *cfg);
+typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
 
 struct maintenance_task {
 	const char *name;
-	maintenance_task_fn *fn;
-	maintenance_auto_fn *auto_condition;
+	maintenance_task_fn fn;
+	maintenance_auto_fn auto_condition;
 };
 
 static const struct maintenance_task tasks[] = {

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 08/12] builtin/maintenance: split into foreground and background tasks

[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
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 | 70 ++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 21 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 447e5800846..72a695853e5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1535,84 +1535,106 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
 
 typedef int (*maintenance_task_fn)(struct maintenance_run_opts *opts,
 				   struct gc_config *cfg);
-
-/*
- * An auto condition function returns 1 if the task should run
- * and 0 if the task should NOT run. See needs_to_gc() for an
- * example.
- */
 typedef int (*maintenance_auto_fn)(struct gc_config *cfg);
 
 struct maintenance_task {
 	const char *name;
-	maintenance_task_fn fn;
+
+	/*
+	 * Work that will be executed before detaching. This should not include
+	 * tasks that may run for an extended amount of time as it does cause
+	 * auto-maintenance to block until foreground tasks have been run.
+	 */
+	maintenance_task_fn foreground;
+
+	/*
+	 * Work that will be executed after detaching. When not detaching the
+	 * work will be run in the foreground, as well.
+	 */
+	maintenance_task_fn background;
+
+	/*
+	 * An auto condition function returns 1 if the task should run and 0 if
+	 * the task should NOT run. See needs_to_gc() for an example.
+	 */
 	maintenance_auto_fn auto_condition;
 };
 
 static const struct maintenance_task tasks[] = {
 	[TASK_PREFETCH] = {
 		.name = "prefetch",
-		.fn = maintenance_task_prefetch,
+		.background = maintenance_task_prefetch,
 	},
 	[TASK_LOOSE_OBJECTS] = {
 		.name = "loose-objects",
-		.fn = maintenance_task_loose_objects,
+		.background = maintenance_task_loose_objects,
 		.auto_condition = loose_object_auto_condition,
 	},
 	[TASK_INCREMENTAL_REPACK] = {
 		.name = "incremental-repack",
-		.fn = maintenance_task_incremental_repack,
+		.background = maintenance_task_incremental_repack,
 		.auto_condition = incremental_repack_auto_condition,
 	},
 	[TASK_GC] = {
 		.name = "gc",
-		.fn = maintenance_task_gc,
+		.background = maintenance_task_gc,
 		.auto_condition = need_to_gc,
 	},
 	[TASK_COMMIT_GRAPH] = {
 		.name = "commit-graph",
-		.fn = maintenance_task_commit_graph,
+		.background = maintenance_task_commit_graph,
 		.auto_condition = should_write_commit_graph,
 	},
 	[TASK_PACK_REFS] = {
 		.name = "pack-refs",
-		.fn = maintenance_task_pack_refs,
+		.background = maintenance_task_pack_refs,
 		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
 		.name = "reflog-expire",
-		.fn = maintenance_task_reflog_expire,
+		.background = maintenance_task_reflog_expire,
 		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {
 		.name = "worktree-prune",
-		.fn = maintenance_task_worktree_prune,
+		.background = maintenance_task_worktree_prune,
 		.auto_condition = worktree_prune_condition,
 	},
 	[TASK_RERERE_GC] = {
 		.name = "rerere-gc",
-		.fn = maintenance_task_rerere_gc,
+		.background = maintenance_task_rerere_gc,
 		.auto_condition = rerere_gc_condition,
 	},
 };
 
+enum task_phase {
+	TASK_PHASE_FOREGROUND,
+	TASK_PHASE_BACKGROUND,
+};
+
 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 foreground = (phase == TASK_PHASE_FOREGROUND);
+	maintenance_task_fn fn = foreground ? task->foreground : task->background;
+	const char *region = foreground ? "maintenance foreground" : "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 +1663,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_FOREGROUND))
+			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 +1676,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_BACKGROUND))
 			result = 1;
 
 	rollback_lock_file(&lk);

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 09/12] builtin/maintenance: fix locking race with refs and reflogs tasks

[Patrick Steinhardt]

As explained in the preceding commit, git-gc(1) knows to detach only
after it has already packed references and expired reflogs. This is done
to avoid racing around their respective lockfiles.

Adapt git-maintenance(1) accordingly and run the "pack-refs" and
"reflog-expire" tasks in the foreground. Note that the "gc" task has the
same issue, but the fix is a bit more involved there and will thus be
done in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 72a695853e5..fdd0dd09be7 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1587,12 +1587,12 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_PACK_REFS] = {
 		.name = "pack-refs",
-		.background = maintenance_task_pack_refs,
+		.foreground = maintenance_task_pack_refs,
 		.auto_condition = pack_refs_condition,
 	},
 	[TASK_REFLOG_EXPIRE] = {
 		.name = "reflog-expire",
-		.background = maintenance_task_reflog_expire,
+		.foreground = maintenance_task_reflog_expire,
 		.auto_condition = reflog_expire_condition,
 	},
 	[TASK_WORKTREE_PRUNE] = {

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 10/12] usage: allow dying without writing an error message

[Patrick Steinhardt]

Sometimes code wants to die in a situation where it already has written
an error message. To use the same error code as `die()` we have to use
`exit(128)`, which is easy to get wrong and leaves magic numbers all
over our codebase.

Teach `die_message_builtin()` to not print any error when passed a
`NULL` pointer as error string. Like this, such users can now call
`die(NULL)` to achieve the same result without any hardcoded error
codes.

Adapt a couple of builtins to use this new pattern to demonstrate that
there is a need for such a helper.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/am.c                |  4 ++--
 builtin/checkout.c          |  4 ++--
 builtin/fetch.c             |  2 +-
 builtin/submodule--helper.c | 12 ++++++------
 usage.c                     |  2 ++
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index e32a3b4c973..a800003340f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1000,7 +1000,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 
 	if (!patch_format) {
 		fprintf_ln(stderr, _("Patch format detection failed."));
-		exit(128);
+		die(NULL);
 	}
 
 	if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
@@ -1178,7 +1178,7 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 		strbuf_release(&sb);
 	}
 
-	exit(128);
+	die(NULL);
 }
 
 /**
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d185982f3a6..536192d3456 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -838,7 +838,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 		init_tree_desc(&trees[0], &tree->object.oid,
 			       tree->buffer, tree->size);
 		if (parse_tree(new_tree) < 0)
-			exit(128);
+			die(NULL);
 		tree = new_tree;
 		init_tree_desc(&trees[1], &tree->object.oid,
 			       tree->buffer, tree->size);
@@ -913,7 +913,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 						     work,
 						     old_tree);
 			if (ret < 0)
-				exit(128);
+				die(NULL);
 			ret = reset_tree(new_tree,
 					 opts, 0,
 					 writeout_error, new_branch_info);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index cda6eaf1fd6..b0800ea5829 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -992,7 +992,7 @@ static int update_local_ref(struct ref *ref,
 		fast_forward = repo_in_merge_bases(the_repository, current,
 						   updated);
 		if (fast_forward < 0)
-			exit(128);
+			die(NULL);
 		forced_updates_ms += (getnanotime() - t_before) / 1000000;
 	} else {
 		fast_forward = 1;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116ddf..4255caca579 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -303,7 +303,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 	char *displaypath;
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	displaypath = get_submodule_displaypath(path, info->prefix,
 						info->super_prefix);
@@ -643,7 +643,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	};
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	if (!submodule_from_path(the_repository, null_oid(the_hash_algo), path))
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -1257,7 +1257,7 @@ static void sync_submodule(const char *path, const char *prefix,
 		return;
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), path);
 
@@ -1402,7 +1402,7 @@ static void deinit_submodule(const char *path, const char *prefix,
 	char *sub_git_dir = xstrfmt("%s/.git", path);
 
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		die(NULL);
 
 	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), path);
 
@@ -1724,7 +1724,7 @@ static int clone_submodule(const struct module_clone_data *clone_data,
 	char *to_free = NULL;
 
 	if (validate_submodule_path(clone_data_path) < 0)
-		exit(128);
+		die(NULL);
 
 	if (!is_absolute_path(clone_data->path))
 		clone_data_path = to_free = xstrfmt("%s/%s", repo_get_work_tree(the_repository),
@@ -3524,7 +3524,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	strip_dir_trailing_slashes(add_data.sm_path);
 
 	if (validate_submodule_path(add_data.sm_path) < 0)
-		exit(128);
+		die(NULL);
 
 	die_on_index_match(add_data.sm_path, force);
 	die_on_repo_without_commits(add_data.sm_path);
diff --git a/usage.c b/usage.c
index 38b46bbbfe7..cd7b57d6446 100644
--- a/usage.c
+++ b/usage.c
@@ -67,6 +67,8 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 
 static void die_message_builtin(const char *err, va_list params)
 {
+	if (!err)
+		return;
 	trace2_cmd_error_va(err, params);
 	vreportf(_("fatal: "), err, params);
 }

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 11/12] builtin/gc: avoid global state in `gc_before_repack()`

[Patrick Steinhardt]

The `gc_before_repack()` should only ever run once in git-gc(1), but we
may end up calling it twice when the "--detach" flag is passed. The
duplicated call is avoided though via a static flag in this function.

This pattern is somewhat unintuitive though. Refactor it to drop the
static flag and instead guard the second call of `gc_before_repack()`
via `opts.detach`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index fdd0dd09be7..4a5c4b20442 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,22 +816,14 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static void gc_before_repack(struct maintenance_run_opts *opts,
-			     struct gc_config *cfg)
+static int gc_before_repack(struct maintenance_run_opts *opts,
+			    struct gc_config *cfg)
 {
-	/*
-	 * We may be called twice, as both the pre- and
-	 * post-daemonized phases will call us, but running these
-	 * commands more than once is pointless and wasteful.
-	 */
-	static int done = 0;
-	if (done++)
-		return;
-
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
-		die(FAILED_RUN, "pack-refs");
+		return error(FAILED_RUN, "pack-refs");
 	if (cfg->prune_reflogs && maintenance_task_reflog_expire(opts, cfg))
-		die(FAILED_RUN, "reflog");
+		return error(FAILED_RUN, "reflog");
+	return 0;
 }
 
 int cmd_gc(int argc,
@@ -965,7 +957,8 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		gc_before_repack(&opts, &cfg); /* dies on failure */
+		if (gc_before_repack(&opts, &cfg) < 0)
+			die(NULL);
 		delete_tempfile(&pidfile);
 
 		/*
@@ -995,7 +988,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0)
+		gc_before_repack(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

[PATCH v4 12/12] builtin/maintenance: fix locking race when handling "gc" task

[Patrick Steinhardt]

The "gc" task has a similar locking race as the one that we have fixed
for the "pack-refs" and "reflog-expire" tasks in preceding commits. Fix
this by splitting up the logic of the "gc" task:

  - We execute `gc_before_repack()` in the foreground, which contains
    the logic that git-gc(1) itself would execute in the foreground, as
    well.

  - We spawn git-gc(1) after detaching, but with a new hidden flag that
    suppresses calling `gc_before_repack()`.

Like this we have roughly the same logic as git-gc(1) itself and know to
repack refs and reflogs before detaching, thus fixing the race.

Note that `gc_before_repack()` is renamed to `gc_foreground_tasks()` to
better reflect what this function does.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           | 41 +++++++++++++++++++++++++++--------------
 t/t7900-maintenance.sh | 12 ++++++------
 2 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4a5c4b20442..b5e6519d597 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,8 +816,8 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static int gc_before_repack(struct maintenance_run_opts *opts,
-			    struct gc_config *cfg)
+static int gc_foreground_tasks(struct maintenance_run_opts *opts,
+			       struct gc_config *cfg)
 {
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
 		return error(FAILED_RUN, "pack-refs");
@@ -837,6 +837,7 @@ int cmd_gc(int argc,
 	pid_t pid;
 	int daemonized = 0;
 	int keep_largest_pack = -1;
+	int skip_foreground_tasks = 0;
 	timestamp_t dummy;
 	struct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;
 	struct gc_config cfg = GC_CONFIG_INIT;
@@ -869,6 +870,8 @@ int cmd_gc(int argc,
 			 N_("repack all other packs except the largest pack")),
 		OPT_STRING(0, "expire-to", &cfg.repack_expire_to, N_("dir"),
 			   N_("pack prefix to store a pack containing pruned objects")),
+		OPT_HIDDEN_BOOL(0, "skip-foreground-tasks", &skip_foreground_tasks,
+			   N_("skip maintenance tasks typically done in the foreground")),
 		OPT_END()
 	};
 
@@ -952,14 +955,16 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		if (lock_repo_for_gc(force, &pid)) {
-			ret = 0;
-			goto out;
-		}
+		if (!skip_foreground_tasks) {
+			if (lock_repo_for_gc(force, &pid)) {
+				ret = 0;
+				goto out;
+			}
 
-		if (gc_before_repack(&opts, &cfg) < 0)
-			die(NULL);
-		delete_tempfile(&pidfile);
+			if (gc_foreground_tasks(&opts, &cfg) < 0)
+				die(NULL);
+			delete_tempfile(&pidfile);
+		}
 
 		/*
 		 * failure to daemonize is ok, we'll continue
@@ -988,8 +993,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	if (opts.detach <= 0)
-		gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0 && !skip_foreground_tasks)
+		gc_foreground_tasks(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;
@@ -1225,8 +1230,14 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,
 	return 0;
 }
 
-static int maintenance_task_gc(struct maintenance_run_opts *opts,
-			       struct gc_config *cfg UNUSED)
+static int maintenance_task_gc_foreground(struct maintenance_run_opts *opts,
+					  struct gc_config *cfg)
+{
+	return gc_foreground_tasks(opts, cfg);
+}
+
+static int maintenance_task_gc_background(struct maintenance_run_opts *opts,
+					  struct gc_config *cfg UNUSED)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 
@@ -1240,6 +1251,7 @@ static int maintenance_task_gc(struct maintenance_run_opts *opts,
 	else
 		strvec_push(&child.args, "--no-quiet");
 	strvec_push(&child.args, "--no-detach");
+	strvec_push(&child.args, "--skip-foreground-tasks");
 
 	return run_command(&child);
 }
@@ -1571,7 +1583,8 @@ static const struct maintenance_task tasks[] = {
 	},
 	[TASK_GC] = {
 		.name = "gc",
-		.background = maintenance_task_gc,
+		.foreground = maintenance_task_gc_foreground,
+		.background = maintenance_task_gc_background,
 		.auto_condition = need_to_gc,
 	},
 	[TASK_COMMIT_GRAPH] = {
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 1ada5246606..ddd273d8dc2 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -49,9 +49,9 @@ test_expect_success 'run [--auto|--quiet]' '
 		git maintenance run --auto 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \
 		git maintenance run --no-quiet 2>/dev/null &&
-	test_subcommand git gc --quiet --no-detach <run-no-auto.txt &&
-	test_subcommand ! git gc --auto --quiet --no-detach <run-auto.txt &&
-	test_subcommand git gc --no-quiet --no-detach <run-no-quiet.txt
+	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-no-auto.txt &&
+	test_subcommand ! git gc --auto --quiet --no-detach --skip-foreground-tasks <run-auto.txt &&
+	test_subcommand git gc --no-quiet --no-detach --skip-foreground-tasks <run-no-quiet.txt
 '
 
 test_expect_success 'maintenance.auto config option' '
@@ -154,9 +154,9 @@ test_expect_success 'run --task=<task>' '
 		git maintenance run --task=commit-graph 2>/dev/null &&
 	GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \
 		git maintenance run --task=commit-graph --task=gc 2>/dev/null &&
-	test_subcommand ! git gc --quiet --no-detach <run-commit-graph.txt &&
-	test_subcommand git gc --quiet --no-detach <run-gc.txt &&
-	test_subcommand git gc --quiet --no-detach <run-both.txt &&
+	test_subcommand ! git gc --quiet --no-detach --skip-foreground-tasks <run-commit-graph.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-gc.txt &&
+	test_subcommand git gc --quiet --no-detach --skip-foreground-tasks <run-both.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt &&
 	test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt &&
 	test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt

-- 
2.50.0.rc0.629.g846fc57c9e.dirty

Re: [PATCH v3 10/12] usage: allow dying without writing an error message

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Sometimes code wants to die in a situation where it already has written
> an error message. To use the same error code as `die()` we have to open
> code the code with a call to `exit(128)` in such cases, which is easy to
> get wrong and leaves magical numbers all over our codebase.
> ...
>  	if (!patch_format) {
>  		fprintf_ln(stderr, _("Patch format detection failed."));
> -		exit(128);
> +		die(NULL);

It is somewhat surprising that the compiler would not complain with
"Hey, a NULL string as printf format string???" given its decl.

NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));

As long as we are sure that compilers we care about are OK with
this, it is a very nice ergonomics enhancement.

Re: [PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching

[Kristoffer Haugsbakk]

On Tue, Jun 3, 2025, at 16:01, Patrick Steinhardt wrote:
> [snip]
>     @@ Metadata
>      Author: Patrick Steinhardt <ps@pks.im>
>     
>       ## Commit message ##
> [snip]

Looks good!

Re: [PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching

[Ben Knoble]

> 
> Le 3 juin 2025 à 10:01, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> Hi,
> 
> this patch series fixes races around locking the "packed-refs" file when
> auto-maintenance decides to repack it. This issue has been reported e.g.
> via [1] and [2].
> 
> The root cause is that git-gc(1) used to know to detach _after_ having
> repacked references. As such, callers wouldn't continue with their thing
> until we have already packed refs, and thus the race does not exist
> there. git-maintenance(1) didn't have the same split though, so this
> patch series retrofits that logic.
> 
> The series is structured as follows:
> 
>  - Patches 1 and 2 do some light refactorings.
> 
>  - Patches 3 to 5 refactor how we set up the list of tasks to not rely
>    on globals anymore. Instead, we now have a single source of truth
>    for which tasks exactly will be run.
> 
>  - The remaining patches introduce the split of before/after-detach
>    tasks and wire them up for "pack-refs", "reflog-expire" and "gc"
>    tasks.
> 
> Changes in v2:
>  - A couple of commit message improvements.
>  - Introduce `die(NULL)` to die with the correct exit code but no error
>    message. This gets rid of some magic numbers.
>  - Introduce an enum to discern the phases before and after detach.
>  - Link to v1: https://lore.kernel.org/r/20250527-b4-pks-maintenance-ref-lock-race-v1-0-e1ceb2dea66e@pks.im
> 
> Changes in v3:
>  - Rework logic to talk about foreground/background tasks instead of
>    before/after detach.
>  - Link to v2: https://lore.kernel.org/r/20250530-b4-pks-maintenance-ref-lock-race-v2-0-d04e2f93e51f@pks.im
> 
> Changes in v4:
>  - Some more massaging of commit messages.
>  - Link to v3: https://lore.kernel.org/r/20250602-b4-pks-maintenance-ref-lock-race-v3-0-587d44252dcb@pks.im
> 
> Thanks!
> 
> Patrick
> 
> [1]: <CAJR-fbZ4X1+gN75m2dUvocR6NkowLOZ9F26cjBy8w1qd181OoQ@mail.gmail.com>
> [2]: <CANi7bVAkNc+gY1NoXfJuDRjxjZLTgL8Lfn8_ZmWsvLAoiLPkNg@mail.gmail.com>
> 
> ---
> Patrick Steinhardt (12):
>      builtin/gc: use designated field initializers for maintenance tasks
>      builtin/gc: drop redundant local variable
>      builtin/maintenance: centralize configuration of explicit tasks
>      builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
>      builtin/maintenance: stop modifying global array of tasks
>      builtin/maintenance: extract function to run tasks
>      builtin/maintenance: fix typedef for function pointers
>      builtin/maintenance: split into foreground and background tasks
>      builtin/maintenance: fix locking race with refs and reflogs tasks
>      usage: allow dying without writing an error message
>      builtin/gc: avoid global state in `gc_before_repack()`
>      builtin/maintenance: fix locking race when handling "gc" task
> 
> builtin/am.c                |   4 +-
> builtin/checkout.c          |   4 +-
> builtin/fetch.c             |   2 +-
> builtin/gc.c                | 410 +++++++++++++++++++++++++-------------------
> builtin/submodule--helper.c |  12 +-
> t/t7900-maintenance.sh      |  19 +-
> usage.c                     |   2 +
> 7 files changed, 263 insertions(+), 190 deletions(-)
> 
> Range-diff versus v3:
> 
> 1:  e46a65951b9 =  1:  280f13d2895 builtin/gc: use designated field initializers for maintenance tasks
> 2:  73cd67f3e1a =  2:  16a017fb819 builtin/gc: drop redundant local variable
> 3:  a02452a6d6f =  3:  0ab3344ddb0 builtin/maintenance: centralize configuration of explicit tasks
> 4:  ccd7691e4d5 =  4:  69e768cb54e builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
> 5:  0e243fd81e6 =  5:  295e9e5ee9f builtin/maintenance: stop modifying global array of tasks
> 6:  c95bd62823e =  6:  d94b0c86622 builtin/maintenance: extract function to run tasks
> 7:  43d28434d8e =  7:  0bbba671cd0 builtin/maintenance: fix typedef for function pointers
> 8:  d5740a5c9d9 =  8:  4ce38539bb6 builtin/maintenance: split into foreground and background tasks
> 9:  168eb3a9372 !  9:  28092b9bed1 builtin/maintenance: fix locking race when packing refs and reflogs
>    @@ Metadata
>     Author: Patrick Steinhardt <ps@pks.im>
> 
>      ## Commit message ##
>    -    builtin/maintenance: fix locking race when packing refs and reflogs
>    +    builtin/maintenance: fix locking race with refs and reflogs tasks
> 
>         As explained in the preceding commit, git-gc(1) knows to detach only
>    -    after it has already packed references and reflogs. This is done to
>    -    avoid racing around their respective lockfiles.
>    +    after it has already packed references and expired reflogs. This is done
>    +    to avoid racing around their respective lockfiles.
> 
>         Adapt git-maintenance(1) accordingly and run the "pack-refs" and
>         "reflog-expire" tasks in the foreground. Note that the "gc" task has the
> 10:  0ff01f6e2aa ! 10:  b8ed080c67d usage: allow dying without writing an error message
>    @@ Commit message
>         usage: allow dying without writing an error message
> 
>         Sometimes code wants to die in a situation where it already has written
>    -    an error message. To use the same error code as `die()` we have to open
>    -    code the code with a call to `exit(128)` in such cases, which is easy to
>    -    get wrong and leaves magical numbers all over our codebase.
>    +    an error message. To use the same error code as `die()` we have to use
>    +    `exit(128)`, which is easy to get wrong and leaves magic numbers all
>    +    over our codebase.
> 
>         Teach `die_message_builtin()` to not print any error when passed a
>         `NULL` pointer as error string. Like this, such users can now call
> 11:  93f53000e47 = 11:  5b149886263 builtin/gc: avoid global state in `gc_before_repack()`
> 12:  01095d1bf88 = 12:  9ba01f143b3 builtin/maintenance: fix locking race when handling "gc" task
> 
> ---
> base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
> change-id: 20250527-b4-pks-maintenance-ref-lock-race-11ae5d68e06f

Range-diff vs v3 (and v2!) looks good to me; thanks for taking the time!

Re: [PATCH v3 10/12] usage: allow dying without writing an error message

[Patrick Steinhardt]

On Tue, Jun 03, 2025 at 08:24:59AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Sometimes code wants to die in a situation where it already has written
> > an error message. To use the same error code as `die()` we have to open
> > code the code with a call to `exit(128)` in such cases, which is easy to
> > get wrong and leaves magical numbers all over our codebase.
> > ...
> >  	if (!patch_format) {
> >  		fprintf_ln(stderr, _("Patch format detection failed."));
> > -		exit(128);
> > +		die(NULL);
> 
> It is somewhat surprising that the compiler would not complain with
> "Hey, a NULL string as printf format string???" given its decl.
> 
> NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
> 
> As long as we are sure that compilers we care about are OK with
> this, it is a very nice ergonomics enhancement.

Hard to say. The documentation of -Wformat doesn't explicitly mention
what is expected to happen in that case. I tried compiling with
-Wformat=2, and that didn't generate any warnings for those calls to
`die(NULL)`. It did generate a bunch of unrelated warnings though.

I haven't seen any issues in our CI, either.

So I'm inclined to just go with this variant for now. If we ever see
that it does cause problems with some compiler it's trivial to just
`s/die(NULL)/die_silent()` or something similar.

Patrick

Re: [PATCH v4 04/12] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible

[Derrick Stolee]

On 6/3/2025 10:01 AM, Patrick Steinhardt wrote:
> The "--task=" option explicitly allows the user to say which maintenance
> tasks should be run, whereas "--schedule=" only respects the maintenance
> strategy configured for a specific repository. As such, it is not
> sensible to accept both options at the same time.
> 
> Mark them as incompatible with one another. While at it, also convert
> the existing logic that marks "--auto" and "--schedule=" as incompatible
> to use `die_for_incompatible_opt2()`.

This is a good change. Please consider squashing in this change to the
documentation to match this expectation:

--- >8 ---

diff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc
index 931f3e02e85..a901c46ce0e 100644
--- a/Documentation/git-maintenance.adoc
+++ b/Documentation/git-maintenance.adoc
@@ -193,6 +193,8 @@ OPTIONS
 	config value. The tasks that are tested are those provided by
 	the `--task=<task>` option(s) or those with
 	`maintenance.<task>.enabled` set to true.
++
+The `--schedule` option cannot be used with the `--task` option.
 
 --quiet::
 	Do not report progress or other information over `stderr`.
@@ -203,6 +205,8 @@ OPTIONS
 	arguments are specified, then only the tasks with
 	`maintenance.<task>.enabled` configured as `true` are considered.
 	See the 'TASKS' section for the list of accepted `<task>` values.
++
+The `--task` option cannot be used with the `--schedule` option.
 
 --scheduler=auto|crontab|systemd-timer|launchctl|schtasks::
 	When combined with the `start` subcommand, specify the scheduler

Re: [PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching

[Derrick Stolee]

On 6/3/2025 10:01 AM, Patrick Steinhardt wrote:
> Hi,
> 
> this patch series fixes races around locking the "packed-refs" file when
> auto-maintenance decides to repack it. This issue has been reported e.g.
> via [1] and [2].
> 
> The root cause is that git-gc(1) used to know to detach _after_ having
> repacked references. As such, callers wouldn't continue with their thing
> until we have already packed refs, and thus the race does not exist
> there. git-maintenance(1) didn't have the same split though, so this
> patch series retrofits that logic.

Thanks for making these changes. I read this v4 and only found an
opportunity to improve our docs relative to the more helpful errors
around using --schedule and --task together. I sent a diff that could
be squashed in or skipped.

Thanks,
-Stolee

Re: [PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching

[Patrick Steinhardt]

On Thu, Jun 05, 2025 at 11:53:32AM -0400, Derrick Stolee wrote:
> On 6/3/2025 10:01 AM, Patrick Steinhardt wrote:
> > Hi,
> > 
> > this patch series fixes races around locking the "packed-refs" file when
> > auto-maintenance decides to repack it. This issue has been reported e.g.
> > via [1] and [2].
> > 
> > The root cause is that git-gc(1) used to know to detach _after_ having
> > repacked references. As such, callers wouldn't continue with their thing
> > until we have already packed refs, and thus the race does not exist
> > there. git-maintenance(1) didn't have the same split though, so this
> > patch series retrofits that logic.
> 
> Thanks for making these changes. I read this v4 and only found an
> opportunity to improve our docs relative to the more helpful errors
> around using --schedule and --task together. I sent a diff that could
> be squashed in or skipped.

I see that the series got merged yesterday, so your mail overlapped with
that. I think it's a good addition though, so would you mind sending the
diff as a follow-up patch?

Thanks for your review!

Patrick