Re: [PATCH 1/8] midx: start tracking per object database source

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 10, 2025 at 07:10:02PM -0400, Taylor Blau wrote:
> On Wed, Jul 09, 2025 at 09:54:49AM +0200, Patrick Steinhardt wrote:
> > Multi-pack indices are tracked via `struct multi_pack_index`. This data
> > structure is stored as a linked list inside `struct object_database`,
> > which is the global database that spans across all of the object
> > sources.
> >
> > This layout causes two problems:
> >
> >   - Multi-pack indices aren't global to an object database, but instead
> >     there can be one multi-pack index per source. This creates a
> >     mismatch between the on-disk layout and how things are organized in
> >     the object database subsystems and makes some parts, like figuring
> >     out whether a source has an MIDX, quite awkward.
> 
> This is a little confusing to me. What do we consider to be an "object
> database", and what do we consider to be a "source"?
> 
> For instance, if I have a repository with one or more alternates, I
> would imagine that each alternate is a separate "source", and the
> sources together comprise the object database. Does that match the way
> you're thinking about it?
> 
> If so, that makes sense. But if not (i.e., we consider all alternates to
> belong to the same object database and share a single source), then I
> don't know how this will interact with the existing MIDX alternates
> mechanism.
> 
> Some clarification here would be helpful, I think.

I'll add some, sure. This whole infra is currently developing, so it
doesn't hurt to reiterate some of the points that have already landed.

> > diff --git a/midx.c b/midx.c
> > index 3c5bc821730..a91231bfcdf 100644
> > --- a/midx.c
> > +++ b/midx.c
> > @@ -724,28 +724,29 @@ int midx_preferred_pack(struct multi_pack_index *m, uint32_t *pack_int_id)
> >  	return 0;
> >  }
> >
> > -int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local)
> > +int prepare_multi_pack_index_one(struct odb_source *source, int local)
> 
> OK, so the combination of a repository and "object_dir" path becomes a
> new "struct odb_source", which makes sense. But shouldn't "local" also
> be a property of the odb_source?
> 
> I am not familiar enough with the odb_source work thus far to know
> whether that is a good idea or not, but just something that stood out
> to me while reading.

Yes, you're right on point. I have a follow-up patch series that'll
eventually get rid of some duplicate information that is now present in
the ODB source connected to an MIDX.

[snip]
> > +		source->multi_pack_index = m;
> 
> As an aside, I see that we're calling this new field "multi_pack_index".
> I wonder if it would be better to call it "midx", since typing out
> "multi_pack_index" is a little verbose, and "midx" is a common
> abbreviation that I don't think we would cause any confusion with.

Sure, I can do that.

> > diff --git a/midx.h b/midx.h
> > index 9d1374cbd58..b1626a9a7c4 100644
> > --- a/midx.h
> > +++ b/midx.h
> > @@ -3,11 +3,12 @@
> >
> >  #include "string-list.h"
> >
> > +struct bitmapped_pack;
> > +struct git_hash_algo;
> >  struct object_id;
> > +struct odb_source;
> >  struct pack_entry;
> >  struct repository;
> > -struct bitmapped_pack;
> > -struct git_hash_algo;
> 
> (I'm nit-picking, but a similar note here that the unrelated changes
> make it harder to see what is actually going on in this hunk, which is
> adding a declaration for "struct odb_source".)
> 
> > diff --git a/odb.h b/odb.h
> > index e922f256802..8e79c7be520 100644
> > --- a/odb.h
> > +++ b/odb.h
> > @@ -9,10 +