[PATCH v2 15/15] builtin/gc: pack refs when using `git maintenance run --auto`

[Patrick Steinhardt <ps@pks.im>]

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

When running `git maintenance run --auto`, then the various subtasks
will only run as needed. Thus, we for example end up only packing loose
objects if we hit a certain threshold.

Interestingly enough, the "pack-refs" task is actually _never_ executed
when the auto-flag is set because it does not have a condition at all.
As 41abfe15d9 (maintenance: add pack-refs task, 2021-02-09) mentions:

    The 'auto_condition' function pointer is left NULL for now. We could
    extend this in the future to have a condition check if pack-refs
    should be run during 'git maintenance run --auto'.

It is not quite clear from that quote whether it is actually intended
that the task doesn't run at all in this mode. Also, no test was added
to verify this behaviour. Ultimately though, it feels quite surprising
that `git maintenance run --auto --task=pack-refs` would quietly never
do anything at all.

In any case, now that we do have the logic in place to let ref backends
decide whether or not to repack refs, it does make sense to wire it up
accordingly. With the "reftable" backend we will thus now perform
auto-compaction, which optimizes the refdb as needed.

But for the "files" backend we now unconditionally pack refs as it does
not yet know to handle the "auto" flag. Arguably, this can be seen as a
bug fix given that previously the task never did anything at all.
Eventually though we should amend the "files" backend to use some
heuristics for auto compaction, as well.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c                  | 12 +++++++++++-
 t/t0601-reffiles-pack-refs.sh | 10 ++++++++++
 t/t0610-reftable-basics.sh    |  2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bf1f2a621a..3c874b248b 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -206,6 +206,16 @@ struct maintenance_run_opts {
 	enum schedule_priority schedule;
 };
 
+static int pack_refs_condition(void)
+{
+	/*
+	 * The auto-repacking logic for refs is handled by the ref backends and
+	 * exposed via `git pack-refs --auto`. We thus always return truish
+	 * here and let the backend decide for us.
+	 */
+	return 1;
+}
+
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -1298,7 +1308,7 @@ static struct maintenance_task tasks[] = {
 	[TASK_PACK_REFS] = {
 		"pack-refs",
 		maintenance_task_pack_refs,
-		NULL,
+		pack_refs_condition,
 	},
 };
 
diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
index 219a495451..7d4ab0b91a 100755
--- a/t/t0601-reffiles-pack-refs.sh
+++ b/t/t0601-reffiles-pack-refs.sh
@@ -370,4 +370,14 @@ test_expect_success 'pack-refs does not drop broken refs during deletion' '
 	test_cmp expect actual
 '
 
+test_expect_success 'maintenance --auto unconditionally packs loose refs' '
+	git update-ref refs/heads/something HEAD &&
+	test_path_is_file .git/refs/heads/something &&
+	git rev-parse refs/heads/something >expect &&
+	git maintenance run --task=pack-refs --auto &&
+	test_path_is_missing .git/refs/heads/something &&
+	git rev-parse refs/heads/something >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index b37d8bf3b1..931d888bbb 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -387,7 +387,7 @@ test_expect_success 'pack-refs: compaction raises locking errors' '
 	test_cmp expect err
 '
 
-for command in pack-refs gc
+for command in pack-refs gc "maintenance run --task=pack-refs"
 do
 test_expect_success "$command: auto compaction" '
 	test_when_finished "rm -rf repo" &&
-- 
2.44.GIT


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