On Fri, Oct 17, 2025 at 06:28:04PM -0400, Taylor Blau wrote:

On Thu, Oct 16, 2025 at 09:26:34AM +0200, Patrick Steinhardt wrote:

quoted

Introduce a new "geometric-repack" task. This task uses our geometric
repack infrastructure as provided by git-repack(1) itself, which is ab
strategy that especially hosting providers tend to use to amortize the
costs of repacking objects.

There is one issue though with geometric repacks, namely that they
unconditionally pack all loose objects, regardless of whether or not
they are reachable. This is done because it means that we can completely
skip the reachability step, which significantly speeds up the operation.
But it has the big downside that we are unable to expire objects over
time.

To address this issue we thus use a split strategy in this new task:
whenever a geometric repack would merge together all packs, we instead
do an all-into-one repack. By default, these all-into-one repacks have
cruft packs enabled, so unreachable objects would now be written into
their own pack. Consequently, they won't be soaked up during geometric
repacking anymore and can be expired with the next full repack, assuming
that their expiry date has surpassed.

Well put. Geometric repacking today is really only what objects appear
in the packfiles, not whether those objects are reachable or not. That's
partially by design: geometric repack operations are meant to be as
lightweight and quick as possible, so performing a potentially expensive
reachability traversal defeats the purpose.

This mirrors what GitHub does today with their own repository
maintenance implementation. There is some number of geometric repack
operations interspersed between full repacks which collapse the
geometric progression and move unreachable objects out into cruft packs.

Yeah, we also do the same at GitLab.

So I think that what you did here makes a ton of sense to me. Ultimately
I think there is a middle ground for geometric repacking (well outside
of the scope for this series ;-), don't worry) that could make it do a
little bit of both.

If 'git pack-objects --stdin-packs' (what ultimately implements the
portion of geometric repacking that combines packs together) knew the
current state of a repository's references, it could mark the objects in
the packs to be combined as either reachable or unreachable. Then in a
specialized mode, you could exclude any objects which are unreachable
from the resulting pack, and take a separate pass to write out a cruft
pack containing those objects before ultimately deleting the combined
packs.

I think that is all possible to do, and I think there is a way we could
do it relatively quickly without harming the performance of geometric
repacking. When traversing and marking objects, we can stop as soon as
we see an object that is not contained in the packs that that we're
combining.

So I don't think we have to do a whole-repository walk, which would
indeed defeat the purpose of geometric repacking. The above procedure
would cause us to write out small cruft packs, but we could use the
--combine-cruft-below-size option of 'git repack' to prevent too many
small cruft packs from accumulating together.

Anyway, nothing of that has anything to do with what you wrote here ;-).
It was mostly an excuse for me to write down some of these thoughts that
I've had in my head and alluded to briefly a couple of weeks ago at Git
Merge. Expect some actual patches in this direction from me in the not
too distant future :-).

Looking forward to them :)

quoted

Signed-off-by: Patrick Steinhardt <redacted>
---
 Documentation/config/maintenance.adoc |  11 +++
 builtin/gc.c                          | 102 +++++++++++++++++++++++++
 t/t7900-maintenance.sh                | 137 ++++++++++++++++++++++++++++++++++
 3 files changed, 250 insertions(+)

diff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc
index 2f719342183..26dc5de423f 100644
--- a/Documentation/config/maintenance.adoc
+++ b/Documentation/config/maintenance.adoc

@@ -75,6 +75,17 @@ maintenance.incremental-repack.auto::
 	number of pack-files not in the multi-pack-index is at least the value
 	of `maintenance.incremental-repack.auto`. The default value is 10.

+maintenance.geometric-repack.auto::
+	This integer config option controls how often the `geometric-repack`
+	task should be run as part of `git maintenance run --auto`. If zero,
+	then the `geometric-repack` task will not run with the `--auto`
+	option. A negative value will force the task to run every time.
+	Otherwise, a positive value implies the command should run either when
+	there are packfiles that need to be merged together to retain the
+	geometric progression, or when there are at least this many loose
+	objects that would be written into a new packfile. The default value is
+	100.
+

OK. To make sure I understand: this limit is the minimum number of loose
objects would cause the geometric-repack task to run, unless there are
pack(s) which would be combined as a result of running a geometric
repack, in which case we run it regardless.

Is that right?

Yeah, exactly. I was initially thinking to only frame this in the
context of "how many packs would be merged"? But the problem with that
is that we wouldn't ever generate _new_ packs if the repository only
ever grows loose objects, and consequently we also wouldn't ever merge
any of them.

quoted

 maintenance.reflog-expire.auto::
 	This integer config option controls how often the `reflog-expire` task
 	should be run as part of `git maintenance run --auto`. If zero, then

diff --git a/builtin/gc.c b/builtin/gc.c
index 026d3a1d714..2c9ecd464d2 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -34,6 +34,7 @@
 #include "pack-objects.h"
 #include "path.h"
 #include "reflog.h"
+#include "repack.h"

Hey, neat ;-).

Yup, your refactorings helped a bunch :)

quoted

@@ -1566,6 +1568,101 @@ static int maintenance_task_incremental_repack(struct maintenance_run_opts *opts
 	return 0;
 }

+static int maintenance_task_geometric_repack(struct maintenance_run_opts *opts,
+					     struct gc_config *cfg)
+{
+	struct pack_geometry geometry = {
+		.split_factor = 2,

I wonder if this should be configurable somewhere. It might not be a bad
idea to introduce a 'repack.geometricSplitFactor' configuration
variable, defaulting to two, and use that here. It would also be nice to
be able to run 'git repack --geometric -d' and have it fallback to that
split factor, since using "2" is so common that it's frustrating when I
forget to type it out explicitly ;-).

I was also pondering over this. I think the way to do so would be to
introduce "maintenance.geometric-repack.splitFactor", as that follows
all the other maintenance configuration we have there, as well.

I decided to not do it yet as I wanted to keep the scope of this patch
series contained. But honestly, it's an easy-enough change to make, so
let me introduce another patch to do this.

quoted

+	};
+	struct pack_objects_args po_args = {
+		.local = 1,
+	};
+	struct existing_packs existing_packs = EXISTING_PACKS_INIT;
+	struct string_list kept_packs = STRING_LIST_INIT_DUP;
+	struct child_process child = CHILD_PROCESS_INIT;
+	int ret;
+
+	existing_packs.repo = the_repository;
+	existing_packs_collect(&existing_packs, &kept_packs);
+	pack_geometry_init(&geometry, &existing_packs, &po_args);
+	pack_geometry_split(&geometry);
+
+	child.git_cmd = 1;
+
+	strvec_pushl(&child.args, "repack", "-d", "-l", NULL);
+	if (geometry.split < geometry.pack_nr)
+		strvec_push(&child.args, "--geometric=2");
+	else
+		add_repack_all_option(cfg, NULL, &child.args);

Makes sense; if we're not merging any packs, we do an all-into-one
repack, otherwise we do a geometric one. Looks like the function
geometric_repack_auto_condition() below controls whether or not we even
take this path, which makes sense relative to the documentation you
wrote above.

It does, but only in case we do `git maintenance run --auto`. If you run
`git maintenance run` without the flag we unconditionally execute this
code here. But that's fine: if the repository is already well-optimized
we don't end up doing anything.

quoted

+static int geometric_repack_auto_condition(struct gc_config *cfg UNUSED)
+{
+	struct pack_geometry geometry = {
+		.split_factor = 2,
+	};
+	struct pack_objects_args po_args = {
+		.local = 1,
+	};
+	struct existing_packs existing_packs = EXISTING_PACKS_INIT;
+	struct string_list kept_packs = STRING_LIST_INIT_DUP;
+	int auto_value = 100;
+	int ret;
+
+	repo_config_get_int(the_repository, "maintenance.geometric-repack.auto",
+			    &auto_value);
+	if (!auto_value)
+		return 0;
+	if (auto_value < 0)
+		return 1;
+
+	existing_packs.repo = the_repository;
+	existing_packs_collect(&existing_packs, &kept_packs);
+	pack_geometry_init(&geometry, &existing_packs, &po_args);
+	pack_geometry_split(&geometry);
+
+	/*
+	 * When we'd merge at least two packs with one another we always
+	 * perform the repack.
+	 */
+	if (geometry.split) {
+		ret = 1;
+		goto out;
+	}

Hmm. I wish that we could somehow pass this information to the function
above so that we don't have to re-discover the fact that there are packs
to combine. I'm not familiar enough with the maintenance code to know
how difficult that would be to do, but it looks like at least the
gc_config pointer is shared between the auto condition and the task
itself.

That's kind of gross to tack on there, but I could see a compelling
argument for passing around an extra void pointer between the two that
would allow us to propagate this kind of data between the auto condition
and the task itself. It's not super expensive to do so I don't think not
doing it is a show-stopper at least from a performance perspective, but
it does seem like a good opportunity to DRY things up a bit.

The problem is that the auto-condition is not evaluated when running
without the "--auto" flag. We of course can conditionally compute the
split in case we figure that the auto-condition didn't run, but it does
get somewhat dirty.

So I'd propose to defer such a change into the future in case we notice
that this indeed is a problem. Is that fine with you?

quoted

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index ddd273d8dc2..83a373fe94b 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh

@@ -465,6 +465,143 @@ test_expect_success 'maintenance.incremental-repack.auto (when config is unset)'
 	)
 '

+run_and_verify_geometric_pack () {
+	EXPECTED_PACKS="$1" &&
+
+	# Verify that we perform a geometric repack.
+	rm -f "trace2.txt" &&
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
+		git maintenance run --task=geometric-repack 2>/dev/null &&
+	test_subcommand git repack -d -l --geometric=2 --quiet --write-midx <trace2.txt &&

Makes sense. I do think the test_subcommand thing is a little fragile
here, but verifying that the resulting pack structure forms a geometric
progression feels like overkill for this test, so I think what you wrote
here makes sense.

Oh, yeah, it's fragile and somewhat gross indeed. Couldn't really find a
nicer way to do it though :/

As an aside, would you mind wrapping these lines instead of putting the
command-line invocation all together on a single line?

Sure, can do.

quoted

+
+	# Verify that the number of packfiles matches our expectation.
+	ls -l .git/objects/pack/*.pack >packfiles &&
+	test_line_count = "$EXPECTED_PACKS" packfiles &&
+
+	# And verify that there are no loose objects anymore.
+	cat >expect <<-\EOF &&
+	info
+	pack
+	EOF
+	ls .git/objects >actual &&

I wonder if there is an easier way to check for loose objects here that
doesn't require you to know that the "info" and "pack" directories
exist. Perhaps something like:

test_stdout_line_count = 0 find .git/objects/?? -type f

, or even

    find .git/objects/?? -type f >loose.objs &&
    test_must_be_empty loose.objs

This doesn't work though in case there is not even a single sharding
directory:

    find: '.git/objects/??': No such file or directory

I didn't really have any other idea for now to do this.

quoted

+test_expect_success 'geometric repacking task' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		git config set maintenance.auto false &&
+		test_commit initial &&
+
+		# The initial repack causes an all-into-one repack.
+		GIT_TRACE2_EVENT="$(pwd)/initial-repack.txt" \
+			git maintenance run --task=geometric-repack 2>/dev/null &&
+		test_subcommand git repack -d -l --cruft --cruft-expiration=2.weeks.ago --quiet --write-midx <initial-repack.txt &&
+
+		# Repacking should now cause a no-op geometric repack because
+		# no packfiles need to be combined.
+		ls -l .git/objects/pack >before &&
+		run_and_verify_geometric_pack 1 &&
+		ls -l .git/objects/pack >after &&
+		test_cmp before after &&
+
+		# This incremental change creates a new packfile that only
+		# soaks up loose objects. The packfiles are not getting merged
+		# at this point.
+		test_commit loose &&
+		run_and_verify_geometric_pack 2 &&

I wonder if you want to harden this test a little bit to ensure that the
there is only one new pack being created here, and we're not seeing
e.g., the removal of the existing pack and creation of two new packs.

I dunno, that may be overkill for this test, and I certainly don't feel
strongly about it.

Yeah, I had the same thought when writing this, but it quickly got ugly.
I then decided to not do this and instead only verify that the structure
loosely looks like we expect, and that the expected command actually
ran.

In the end we can rely on t7703 to verify the internals of how exactly
the geometric repack works, whereas here we only verify that the
strategy works as expected.

quoted

+
+		# Both packfiles have 3 objects, so the next run would cause us
+		# to merge both packfiles together. This should be turned into

Perhaps s/both/all/ ? What you wrote is not wrong, of course, but I
think "all" more clearly communicates that we are only doing an
all-into-one because the geometric repack would have combined everything
together anyway.

Sure, happy to reword.

Thanks for your review!

Patrick