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

[Qin ShiCheng <qeesung@live.com>]

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

> 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