Re: [PATCH 12/13] odb/source-files: move alternates into the backend

On Tue, Oct 06, 2026 at 01:51:33PM -0700, Karthik Nayak wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

Originally, when designing pluggable object databases the goal was that
the object database can have multiple sources, and every source attached
to it could use a different backend. This would have allowed for quite a
lot of flexibility, as you could trivially mix and match different kinds
of object storages in whatever way you like.

But while well-intentioned, this design led to a bunch of conceptual
problems:

  - We're now trying to read objects in source order, whereas we
    previously tried to read objects via packfiles before trying to read
    them via loose objects. This led to a performance regression when
    using alternates or when using a quarantine directory.

Could the design be instead to use a mapping function which allows us to
map objects to sources, based on some characteristics of the object?

You could, but it adds complexity that only needs to exist because of
the needs of the "files" backend. Ideally though, we'd not be leaking
internal implementation details of specific backends into callers and
have the interfaces be as agnoics as possible.

quoted

  - Some data structures are supposed to only ever exist once, like for
    example bitmaps and commit graphs. At the same time, those data
    structures also span across the union of all objects, so they may
    cross sources.

This is not really a problem for having multiple sources though.

Not necessarily, but it makes it extremely awkward. The sources now need
to reach into the other sources and be aware of them, and that is a huge
design smell. I've tried multiple times to squeeze these data structures
into the design, but everything single time the result was atrocious.

[snip]

quoted

In short, there are a bunch of conceptual mismatches when we have
alternates and pluggable object databases coexist. So while the original
idea was nice, it does not result in a system that is easy to reason
about.

Correct course by moving alternates into the "files" source itself so
that it becomes an implementation detail thereof so that we can avoid
all of these shortcomings. While it's unfortunate that we cannot easily
mix and match sources now, that ability doesn't go away. It's still very
much feasible to introduce a new backend that allows for exactly that
use case, and such a backend may also be a lot more flexible as we can
now add new logic to determine which objects should be stored where. So
the original motivation for having per-source backends can still be
realized with the new architecture.

Okay, this makes sense, so the new source could be merged source of some
sorts, with internal logic which it uses to map to different sources.
Nice.

Yes, exactly. And such a design would also have three important benefits:

  - We can start from scratch and be sure that such a filtering system
    is well defined instead of trying to shoehorn this into the object
    database somehow.

  - The design can be a lot more flexible because we start from scratch,
    and it can easily have configuration to fine-tune things.

  - The logic to handle this would be entirely self-contained in such a
    backend, and its design details would not have to leak into callers.

The only downside is that we'd have to have another backend specific to
such a thing. But I'd rather have a backend specifically designed for
this that is entirely self-contained compared to having to support such
a feature with code cluttered around our object subsystems.

It took me a while to realize this myself though.

quoted

diff --git a/midx.c b/midx.c
index c0f82c4163..8638ddf0be 100644
--- a/midx.c
+++ b/midx.c

@@ -829,21 +829,15 @@ void clear_incremental_midx_files_ext(struct odb_source_packed *source, const ch

 void clear_midx_file(struct repository *r)
 {
-	struct odb_source_files *files;
+	struct odb_source_files *files = odb_source_files_downcast(r->objects->source);

We remove the the previous `if(r->objects)` check here, is that okay?

Yes, it is. There's only a single caller, and that caller
unconditionally dereferences `r->objects` already. And we also
dereference that pointer a bit further down in this same function here.
So the check was giving a false sense of security anyway, and we're
basically just moving up the unconditional dereference of the pointer
now.

Thanks!

Patrick