[PATCH 03/11] builtin/maintenance: centralize configuration of explicit tasks
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 27, 2025, 14:04 UTC
- Message-ID
- <20250527-b4-pks-maintenance-ref-lock-race-v1-3-e1ceb2dea66e@pks.im>
- In-Reply-To
- <20250527-b4-pks-maintenance-ref-lock-race-v1-0-e1ceb2dea66e@pks.im>
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