@matheusbsbs / matheus-bagliano / commits / 38a8fa5a9a

builtin/maintenance: stop modifying global array of tasks

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Jun 3, 2025 at 16:01 UTC 38a8fa5a9a3a74dd606ba37702689a8b07c084bf

1 file changed +112 -94

builtin/gc.c

+112 -94

@@ -251,7 +251,24 @@ static enum schedule_priority parse_schedule(const char *value)

251 return SCHEDULE_NONE;

252 }

253

254 +enum maintenance_task_label {

255 + TASK_PREFETCH,

256 + TASK_LOOSE_OBJECTS,

257 + TASK_INCREMENTAL_REPACK,

258 + TASK_GC,

259 + TASK_COMMIT_GRAPH,

260 + TASK_PACK_REFS,

261 + TASK_REFLOG_EXPIRE,

262 + TASK_WORKTREE_PRUNE,

263 + TASK_RERERE_GC,

264 +

265 + /* Leave as final value */

266 + TASK__COUNT

267 +};

268 +

269 struct maintenance_run_opts {

270 + enum maintenance_task_label *tasks;

271 + size_t tasks_nr, tasks_alloc;

272 int auto_flag;

273 int detach;

274 int quiet;

@@ -261,6 +278,11 @@ struct maintenance_run_opts {

278 .detach = -1, \

279 }

280

281 +static void maintenance_run_opts_release(struct maintenance_run_opts *opts)

282 +{

283 + free(opts->tasks);

284 +}

285 +

286 static int pack_refs_condition(UNUSED struct gc_config *cfg)

287 {

288 /*

@@ -1032,6 +1054,7 @@ int cmd_gc(int argc,

1054 }

1055

1056 out:

1057 + maintenance_run_opts_release(&opts);

1058 gc_config_release(&cfg);

1059 return 0;

1060 }

@@ -1524,30 +1547,9 @@ struct maintenance_task {

1547 const char *name;

1548 maintenance_task_fn *fn;

1549 maintenance_auto_fn *auto_condition;

1527 - unsigned enabled:1;

1528 -

1529 - enum schedule_priority schedule;

1530 -

1531 - /* -1 if not selected. */

1532 - int selected_order;

1533 -};

1534 -

1535 -enum maintenance_task_label {

1536 - TASK_PREFETCH,

1537 - TASK_LOOSE_OBJECTS,

1538 - TASK_INCREMENTAL_REPACK,

1539 - TASK_GC,

1540 - TASK_COMMIT_GRAPH,

1541 - TASK_PACK_REFS,

1542 - TASK_REFLOG_EXPIRE,

1543 - TASK_WORKTREE_PRUNE,

1544 - TASK_RERERE_GC,

1545 -

1546 - /* Leave as final value */

1547 - TASK__COUNT

1550 };

1551

1550 -static struct maintenance_task tasks[] = {

1552 +static const struct maintenance_task tasks[] = {

1553 [TASK_PREFETCH] = {

1554 .name = "prefetch",

1555 .fn = maintenance_task_prefetch,

@@ -1566,7 +1568,6 @@ static struct maintenance_task tasks[] = {

1568 .name = "gc",

1569 .fn = maintenance_task_gc,

1570 .auto_condition = need_to_gc,

1569 - .enabled = 1,

1571 },

1572 [TASK_COMMIT_GRAPH] = {

1573 .name = "commit-graph",

@@ -1595,18 +1596,9 @@ static struct maintenance_task tasks[] = {

1596 },

1597 };

1598

1598 -static int compare_tasks_by_selection(const void *a_, const void *b_)

1599 -{

1600 - const struct maintenance_task *a = a_;

1601 - const struct maintenance_task *b = b_;

1602 -

1603 - return b->selected_order - a->selected_order;

1604 -}

1605 -

1599 static int maintenance_run_tasks(struct maintenance_run_opts *opts,

1600 struct gc_config *cfg)

1601 {

1609 - int i, found_selected = 0;

1602 int result = 0;

1603 struct lock_file lk;

1604 struct repository *r = the_repository;

@@ -1635,95 +1627,120 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,

1627 trace2_region_leave("maintenance", "detach", the_repository);

1628 }

1629

1638 - for (i = 0; !found_selected && i < TASK__COUNT; i++)

1639 - found_selected = tasks[i].selected_order >= 0;

1640 -

1641 - if (found_selected)

1642 - QSORT(tasks, TASK__COUNT, compare_tasks_by_selection);

1643 -

1644 - for (i = 0; i < TASK__COUNT; i++) {

1645 - if (found_selected && tasks[i].selected_order < 0)

1646 - continue;

1647 -

1648 - if (!found_selected && !tasks[i].enabled)

1649 - continue;

1650 -

1630 + for (size_t i = 0; i < opts->tasks_nr; i++) {

1631 if (opts->auto_flag &&

1652 - (!tasks[i].auto_condition ||

1653 - !tasks[i].auto_condition(cfg)))

1654 - continue;

1655 -

1656 - if (opts->schedule && tasks[i].schedule < opts->schedule)

1632 + (!tasks[opts->tasks[i]].auto_condition ||

1633 + !tasks[opts->tasks[i]].auto_condition(cfg)))

1634 continue;

1635

1659 - trace2_region_enter("maintenance", tasks[i].name, r);

1660 - if (tasks[i].fn(opts, cfg)) {

1661 - error(_("task '%s' failed"), tasks[i].name);

1636 + trace2_region_enter("maintenance", tasks[opts->tasks[i]].name, r);

1637 + if (tasks[opts->tasks[i]].fn(opts, cfg)) {

1638 + error(_("task '%s' failed"), tasks[opts->tasks[i]].name);

1639 result = 1;

1640 }

1664 - trace2_region_leave("maintenance", tasks[i].name, r);

1641 + trace2_region_leave("maintenance", tasks[opts->tasks[i]].name, r);

1642 }

1643

1644 rollback_lock_file(&lk);

1645 return result;

1646 }

1647

1671 -static void initialize_maintenance_strategy(void)

1648 +struct maintenance_strategy {

1649 + struct {

1650 + int enabled;

1651 + enum schedule_priority schedule;

1652 + } tasks[TASK__COUNT];

1653 +};

1654 +

1655 +static const struct maintenance_strategy none_strategy = { 0 };

1656 +static const struct maintenance_strategy default_strategy = {

1657 + .tasks = {

1658 + [TASK_GC].enabled = 1,

1659 + },

1660 +};

1661 +static const struct maintenance_strategy incremental_strategy = {

1662 + .tasks = {

1663 + [TASK_COMMIT_GRAPH].enabled = 1,

1664 + [TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY,

1665 + [TASK_PREFETCH].enabled = 1,

1666 + [TASK_PREFETCH].schedule = SCHEDULE_HOURLY,

1667 + [TASK_INCREMENTAL_REPACK].enabled = 1,

1668 + [TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY,

1669 + [TASK_LOOSE_OBJECTS].enabled = 1,

1670 + [TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY,

1671 + [TASK_PACK_REFS].enabled = 1,

1672 + [TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY,

1673 + },

1674 +};

1675 +

1676 +static void initialize_task_config(struct maintenance_run_opts *opts,

1677 + const struct string_list *selected_tasks)

1678 {

1679 + struct strbuf config_name = STRBUF_INIT;

1680 + struct maintenance_strategy strategy;

1681 const char *config_str;

1682

1675 - if (git_config_get_string_tmp("maintenance.strategy", &config_str))

1676 - return;

1683 + /*

1684 + * In case the user has asked us to run tasks explicitly we only use

1685 + * those specified tasks. Specifically, we do _not_ want to consult the

1686 + * config or maintenance strategy.

1687 + */

1688 + if (selected_tasks->nr) {

1689 + for (size_t i = 0; i < selected_tasks->nr; i++) {

1690 + enum maintenance_task_label label = (intptr_t)selected_tasks->items[i].util;;

1691 + ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);

1692 + opts->tasks[opts->tasks_nr++] = label;

1693 + }

1694

1678 - if (!strcasecmp(config_str, "incremental")) {

1679 - tasks[TASK_GC].schedule = SCHEDULE_NONE;

1680 - tasks[TASK_COMMIT_GRAPH].enabled = 1;

1681 - tasks[TASK_COMMIT_GRAPH].schedule = SCHEDULE_HOURLY;

1682 - tasks[TASK_PREFETCH].enabled = 1;

1683 - tasks[TASK_PREFETCH].schedule = SCHEDULE_HOURLY;

1684 - tasks[TASK_INCREMENTAL_REPACK].enabled = 1;

1685 - tasks[TASK_INCREMENTAL_REPACK].schedule = SCHEDULE_DAILY;

1686 - tasks[TASK_LOOSE_OBJECTS].enabled = 1;

1687 - tasks[TASK_LOOSE_OBJECTS].schedule = SCHEDULE_DAILY;

1688 - tasks[TASK_PACK_REFS].enabled = 1;

1689 - tasks[TASK_PACK_REFS].schedule = SCHEDULE_WEEKLY;

1695 + return;

1696 }

1691 -}

1697

1693 -static void initialize_task_config(const struct string_list *selected_tasks,

1694 - int schedule)

1695 -{

1696 - struct strbuf config_name = STRBUF_INIT;

1698 + /*

1699 + * Otherwise, the strategy depends on whether we run as part of a

1700 + * scheduled job or not:

1701 + *

1702 + * - Scheduled maintenance does not perform any housekeeping by

1703 + * default, but requires the user to pick a maintenance strategy.

1704 + *

1705 + * - Unscheduled maintenance uses our default strategy.

1706 + *

1707 + * Both of these are affected by the gitconfig though, which may

1708 + * override specific aspects of our strategy.

1709 + */

1710 + if (opts->schedule) {

1711 + strategy = none_strategy;

1712

1698 - for (size_t i = 0; i < TASK__COUNT; i++)

1699 - tasks[i].selected_order = -1;

1700 - for (size_t i = 0; i < selected_tasks->nr; i++) {

1701 - struct maintenance_task *task = selected_tasks->items[i].util;

1702 - task->selected_order = i;

1713 + if (!git_config_get_string_tmp("maintenance.strategy", &config_str)) {

1714 + if (!strcasecmp(config_str, "incremental"))

1715 + strategy = incremental_strategy;

1716 + }

1717 + } else {

1718 + strategy = default_strategy;

1719 }

1720

1705 - if (schedule)

1706 - initialize_maintenance_strategy();

1707 -

1721 for (size_t i = 0; i < TASK__COUNT; i++) {

1722 int config_value;

1710 - char *config_str;

1723

1724 strbuf_reset(&config_name);

1725 strbuf_addf(&config_name, "maintenance.%s.enabled",

1726 tasks[i].name);

1715 -

1727 if (!git_config_get_bool(config_name.buf, &config_value))

1717 - tasks[i].enabled = config_value;

1718 -

1719 - strbuf_reset(&config_name);

1720 - strbuf_addf(&config_name, "maintenance.%s.schedule",

1721 - tasks[i].name);

1728 + strategy.tasks[i].enabled = config_value;

1729 + if (!strategy.tasks[i].enabled)

1730 + continue;

1731

1723 - if (!git_config_get_string(config_name.buf, &config_str)) {

1724 - tasks[i].schedule = parse_schedule(config_str);

1725 - free(config_str);

1732 + if (opts->schedule) {

1733 + strbuf_reset(&config_name);

1734 + strbuf_addf(&config_name, "maintenance.%s.schedule",

1735 + tasks[i].name);

1736 + if (!git_config_get_string_tmp(config_name.buf, &config_str))

1737 + strategy.tasks[i].schedule = parse_schedule(config_str);

1738 + if (strategy.tasks[i].schedule < opts->schedule)

1739 + continue;

1740 }

1741 +

1742 + ALLOC_GROW(opts->tasks, opts->tasks_nr + 1, opts->tasks_alloc);

1743 + opts->tasks[opts->tasks_nr++] = i;

1744 }

1745

1746 strbuf_release(&config_name);

@@ -1750,7 +1767,7 @@ static int task_option_parse(const struct option *opt,

1767 return 1;

1768 }

1769

1753 - string_list_append(selected_tasks, arg)->util = &tasks[i];

1770 + string_list_append(selected_tasks, arg)->util = (void *)(intptr_t)i;

1771

1772 return 0;

1773 }

@@ -1791,7 +1808,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,

1808 opts.schedule, "--schedule=");

1809

1810 gc_config(&cfg);

1794 - initialize_task_config(&selected_tasks, opts.schedule);

1811 + initialize_task_config(&opts, &selected_tasks);

1812

1813 if (argc != 0)

1814 usage_with_options(builtin_maintenance_run_usage,

@@ -1800,6 +1817,7 @@ static int maintenance_run(int argc, const char **argv, const char *prefix,

1817 ret = maintenance_run_tasks(&opts, &cfg);

1818

1819 string_list_clear(&selected_tasks, 0);

1820 + maintenance_run_opts_release(&opts);

1821 gc_config_release(&cfg);

1822 return ret;

1823 }