Re: [PATCH 4/8] packfile: fix approximation of object counts

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 29, 2025, 22:49 UTC
Message-ID: <aQKZ7FW925zvscgh@nand.local>
In-Reply-To: <20251028-pks-packfiles-store-drop-list-v1-4-1a3b82030a7a@pks.im>

On Tue, Oct 28, 2025 at 12:08:34PM +0100, Patrick Steinhardt wrote:

> None of these are really game-changing. But it's nice to fix those
> issues regardless.

Well explained, thank you.

Show 22 quoted lines

> While at it, convert the code to use `repo_for_each_pack()`.
> Furthermore, use `odb_prepare_alternates()` instead of explicitly
> preparing the packfile store. We really only want to prepare the object
> database sources, and `get_multi_pack_index()` already knows to prepare
> the packfile store for us.
>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  packfile.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/packfile.c b/packfile.c
> index 6aa2ca8ac9e..6722c3b2b88 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1143,16 +1143,16 @@ unsigned long repo_approximate_object_count(struct repository *r)
>  		unsigned long count = 0;
>  		struct packed_git *p;
>
> -		packfile_store_prepare(r->objects->packfiles);
> +		odb_prepare_alternates(r->objects);

I was wondering how this worked, since odb_prepare_alternates() does not eagerly load the packs belonging to a MIDX, but get_multi_pack_index() does, so this makes sense.

(Writing this out, I realized that you wrote this as the last sentence in your patch, which is helpful and I think worth doing.)

Show 5 quoted lines

>  		for (source = r->objects->sources; source; source = source->next) {
>  			struct multi_pack_index *m = get_multi_pack_index(source);
>  			if (m)
> -				count += m->num_objects;
> +				count += m->num_objects + m->num_objects_in_base;

Oops. This fix is definitely right, thanks for spotting and fixing it.

As a general aside, I expect that we're going to find some more of these. I tried my best to audit all the places where we use m->num_objects and m->num_packs, but without having great infrastructure that encourages the use of MIDX chains, most of this code is all dead anyway.

Hopefully soon we will see some more usage of MIDX chains with the incremental repacking work that I've been sending patches for recently. I'm sure that will flush out more of these issues.

> -		for (p = r->objects->packfiles->packs; p; p = p->next) {
> -			if (open_pack_index(p))
> +		repo_for_each_pack(r, p) {
> +			if (open_pack_index(p) || p->multi_pack_index)

Do we care about opening the pack index if we already accounted for it via the MIDX path above? My guess is not, so I would probably suggest writing this conditional as:

    if (p->multi_pack_index || open_pack_index(p))
        continue;

to avoid loading pack indexes unless we have to.

Thanks, Taylor