From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 16 Oct 2025 20:51:17 GMT
Subject: Re: [PATCH 3/8] builtin/maintenance: introduce "geometric-repack" task
Message-ID: <uos7cczvzlrwjgcyhzyfirck62qjnb4zcoy6ga2o3pbnba7cfj@ag2pnonze5tu>
In-Reply-To: <20251016-pks-maintenance-geometric-strategy-v1-3-18943d474203@pks.im>

On 25/10/16 09:26AM, Patrick Steinhardt wrote:
> Introduce a new "geometric-repack" task. This task uses our geometric
> repack infrastructure as provided by git-repack(1) itself, which is a
> 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.

So normal geometric repacks don't ever check for unreachable objects,
even if all the packs are being merged together. With this new strategy
though, when a geometric repack would normally merge together all packs,
we instead to an all-into-one repack which does check for unreachable
objects.

Does checking for unreachable objects in this case slow down the repack
significantly?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> @@ -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,
> +	};
> +	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);

Here we do the full repack when the all packs are to be merged. Makes
sense.

[snip]
> @@ -1608,6 +1705,11 @@ static const struct maintenance_task tasks[] = {
>  		.background = maintenance_task_incremental_repack,
>  		.auto_condition = incremental_repack_auto_condition,
>  	},
> +	[TASK_GEOMETRIC_REPACK] = {
> +		.name = "geometric-repack",
> +		.background = maintenance_task_geometric_repack,
> +		.auto_condition = geometric_repack_auto_condition,
> +	},

Here we configure the new maintenance task. Nice :)

The rest of this patch looks good.

-Justin