Re: [PATCH v2 2/5] pack-objects: reset kept-pack cache for cruft walk

From: Qin ShiCheng <qeesung@live.com>
Date: Sep 23, 2026, 03:08 UTC
Message-ID: <SJ0PR84MB2993BE38DCAD2ECA5159EC24DD822@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM>
In-Reply-To: <xmqqjyocdijn.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

> When downcasting finds that 'source' is not from the files backend,
> we immediately hit BUG().  Is checking the type of 'source' first
> and calling packfile_store_invalidate_kept_pack_cache() only when
> it is from the files backend a sensible workaround?  That sounds
> like a blatant layering violation.

Agreed, and I would rather not have pack-objects look at the type of a source at all.

The assumption is already made two lines above the new loop, though: repo_for_each_pack() downcasts every source in the same way, and so does has_object_kept_pack(), which is what reads this cache in the first place. So the loop is not wrong so much as in the wrong place. It belongs next to its reader in packfile.c, not in the builtin.

For v3 I have this instead:

	void repo_invalidate_kept_pack_caches(struct repository *r)
	{
		struct odb_source *source;

		for (source = r->objects->sources; source; source = source->next) {
			struct odb_source_files *files = odb_source_files_downcast(source);
			invalidate_kept_pack_cache(files->packed);
		}
	}

with the per-store function made static again, and the caller in pack-objects reduced to

	mark_pack_kept_in_core(fresh_packs, 1);
	repo_invalidate_kept_pack_caches(the_repository);

This does not make the code work with another backend -- nothing around it would either -- but pack-objects no longer gains a new dependency on the files backend, and the downcast sits with the others that will have to move together.

> Do we need a similar
> rearchitecting of the code here, pushing details like packfile
> management down to the files backend layer, before we can properly
> fix this?

I hope not. Without this patch, a cruft repack with an expiration drops objects that are only reachable through a pack pack-objects was not told about; the new test in t5329 shows it happening today. When packfile management does move down to the files backend, this function should go along with has_object_kept_pack(), and nothing in the fix depends on where they end up. Patrick may well know better how that is meant to look.

Thanks, Qin