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

[Junio C Hamano <gitster@pobox.com>]

"Qin ShiCheng via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
>  	/*
>  	 * Re-mark only the fresh packs as kept so that objects in
>  	 * unknown packs do not halt the reachability traversal early.
> +	 * The kept-pack cache was built while those packs were still
> +	 * marked, so drop it too.
>  	 */
>  	repo_for_each_pack(the_repository, p)
>  		p->pack_keep_in_core = 0;
>  	mark_pack_kept_in_core(fresh_packs, 1);
> +	for (source = the_repository->objects->sources; source;
> +	     source = source->next) {
> +		struct odb_source_files *files = odb_source_files_downcast(source);
> +		packfile_store_invalidate_kept_pack_cache(files->packed);
> +	}

This question is primarily meant for folks who are pushing different
ODB backends, but I am not sure this is safe in the long term.

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.

One of the recent design decisions, unrelated to this, was to make
the concept of "alternate object store" an implementation detail of
the files backend, if I recall correctly.  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?

Of course, until an ODB backend other than files materializes, all
of the above is merely academic and the proposed change might be
sufficient.  However, relying on an unchecked downcast feels like
laying mines for our future selves.