Re: [PATCH 1/8] midx: start tracking per object database source
From: Patrick Steinhardt <hidden>
Date: 2025-07-15 08:26:37
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:quoted
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.
quoted
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]
quoted
+ 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.
quoted
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".)quoted
diff --git a/odb.h b/odb.h index e922f256802..8e79c7be520 100644 --- a/odb.h +++ b/odb.h@@ -9,10 +