Skip to contentSkip to conversation

odb: add odb_source_files_try() for heterogeneous source iteration

PATCH In review Aaron Paterson via GitGitGadget 2 participants 4 messages Mar 16 – Mar 19

Subsystems (2)
Kbuild Arm/Amlogic Meson SoC support
lore
Share & tools
lore

Aaron Paterson via GitGitGadget

author
#0 lore raw

cover odb: add odb_source_files_try() for heterogeneous source iteration

The odb_source vtable introduced in this release allows multiple backend implementations via the odb_source_type enum. However, 22 callsites iterate the source chain and downcast every source to odb_source_files to access files-specific internals (pack store, loose cache, MIDX). This prevents adding any non-files source to the chain without hitting a BUG() assertion.

This two-patch series adds odb_source_files_try() as a companion to the existing downcast function. It returns NULL for non-files sources instead of aborting. The 22 source-chain iteration sites are then converted to use this new helper, skipping non-files sources gracefully.

The existing odb_source_files_downcast() is unchanged and continues to BUG() on type mismatch, protecting call sites that should only ever receive a files source (primary source access, vtable callbacks).

This follows the pattern established by the refs subsystem, where eliminating backend downcasts was a prerequisite for adding the reftable backend.

Patch 1/2: Add odb_source_files_try() helper (14 lines, 1 file)

Patch 2/2: Convert 22 iteration sites across 11 files + unit test

Known limitation: repo_approximate_object_count() and has_object_pack() will not account for objects in non-files sources. These are display/optimization functions. A follow-up series can add vtable callbacks to address this.

Aaron Paterson (2):
odb: add odb_source_files_try() for heterogeneous source iteration
odb: use odb_source_files_try() in source-chain iterations

 Makefile                    |  1 +
 builtin/cat-file.c          |  9 ++++++---
 builtin/fast-import.c       |  8 ++++++--
 builtin/grep.c              |  4 +++-
 builtin/pack-objects.c      | 15 +++++++++++----
 commit-graph.c              |  4 +++-
 loose.c                     | 12 +++++++++---
 midx.c                      |  8 ++++++--
 object-file.c               | 20 ++++++++++++++------
 object-name.c               |  8 +++++---
 odb/source-files.h          | 14 ++++++++++++++
 packfile.c                  | 23 +++++++++++++++++------
 packfile.h                  | 14 ++++++++++----
 t/meson.build               |  1 +
 t/unit-tests/u-odb-source.c | 25 +++++++++++++++++++++++++
 15 files changed, 131 insertions(+), 35 deletions(-)
 create mode 100644 t/unit-tests/u-odb-source.c

base-commit: dc6ecd5354dca88d51b6d6562777fc8fc10d77e1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2068%2FMayCXC%2Fps%2Fodb-source-type-guards-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2068/MayCXC/ps/odb-source-type-guards-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2068

Signature · 1 line
gitgitgadget
author
#1 lore raw

PATCH 1/2 odb: add odb_source_files_try() for heterogeneous source iteration

From: Aaron Paterson <[email protected]>

The odb_source vtable introduced in this release allows multiple backend implementations via the odb_source_type enum. However, source-chain iteration sites that need files-specific internals (pack store, loose cache, MIDX) currently use odb_source_files_downcast(), which calls BUG() for non-files source types. This makes it impossible to add a non-files source to the chain without crashing.

Add odb_source_files_try() as a companion to the existing downcast function. It returns NULL for non-files sources instead of aborting. This follows the pattern used elsewhere in git where a "try" or "maybe" variant provides a fallback path while the strict version retains its safety guarantee.

The existing odb_source_files_downcast() is unchanged and continues to BUG() on type mismatch, protecting call sites that should only ever receive a files source.

A subsequent commit will convert the source-chain iteration sites to use this new helper.

Signed-off-by Aaron Paterson <[email protected]>
---
 odb/source-files.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
odb/source-files.h +14 -0
index 23a3b4e04b..abd093b23f 100644
@@ -32,4 +32,18 @@ static inline struct odb_source_files *odb_source_files_downcast(struct odb_sour
 	return container_of(source, struct odb_source_files, base);
 }
 
+/*
+ * Try to cast the given source to the files backend. Returns NULL if
+ * the source uses a different backend. Use this in loops that iterate
+ * over heterogeneous source chains (e.g. when alternates may include
+ * non-files backends). Use odb_source_files_downcast() when the source
+ * is known to be a files backend.
+ */
+static inline struct odb_source_files *odb_source_files_try(struct odb_source *source)
+{
+	if (source->type != ODB_SOURCE_FILES)
+		return NULL;
+	return container_of(source, struct odb_source_files, base);
+}
+
 #endif
Signature · 1 line
gitgitgadget
author
#2 lore raw

PATCH 2/2 odb: use odb_source_files_try() in source-chain iterations

In reply to Aaron Paterson via GitGitGadget

From: Aaron Paterson <[email protected]>

Convert all source-chain iteration sites that access files-specific internals (pack store, loose cache, MIDX) to use odb_source_files_try() instead of odb_source_files_downcast().

These loops iterate the full source chain, which may include alternates. When a non-files backend is added to the chain (as an alternate or additional source), these sites must skip it rather than abort. The _try() helper returns NULL for non-files sources, and each site checks for NULL before accessing files-specific members.

Call sites that access odb->sources directly (the primary source, which is always a files backend) or that are inside files-backend vtable callbacks continue to use odb_source_files_downcast() with its BUG() safety guarantee.

Sites converted (22 across 11 files):

- loose.c: loose object map loading and oid lookup
- midx.c: multi-pack-index access and cleanup
- object-file.c: loose cache and reprepare
- object-name.c: short object filename search
- packfile.c: window/fd management, pack entry lookup
- packfile.h: repo_for_each_pack iterator macros
- commit-graph.c: packed object enumeration
- builtin/cat-file.c: batch object iteration
- builtin/fast-import.c: pack lookup in store/stream
- builtin/grep.c: pack preparation
- builtin/pack-objects.c: cruft/kept pack enumeration

Add a unit test verifying odb_source_files_try() returns the correct backend for ODB_SOURCE_FILES and NULL for other types.

Note: repo_approximate_object_count() and has_object_pack() will not account for objects in non-files sources. These are display and optimization functions respectively, and a follow-up series can add vtable callbacks to address this.

Signed-off-by Aaron Paterson <[email protected]>
---
 Makefile                    |  1 +
 builtin/cat-file.c          |  9 ++++++---
 builtin/fast-import.c       |  8 ++++++--
 builtin/grep.c              |  4 +++-
 builtin/pack-objects.c      | 15 +++++++++++----
 commit-graph.c              |  4 +++-
 loose.c                     | 12 +++++++++---
 midx.c                      |  8 ++++++--
 object-file.c               | 20 ++++++++++++++------
 object-name.c               |  8 +++++---
 packfile.c                  | 23 +++++++++++++++++------
 packfile.h                  | 14 ++++++++++----
 t/meson.build               |  1 +
 t/unit-tests/u-odb-source.c | 25 +++++++++++++++++++++++++
 14 files changed, 117 insertions(+), 35 deletions(-)
 create mode 100644 t/unit-tests/u-odb-source.c
Makefile +1 -0
index 58fb895f4e..3d65093d5a 100644
@@ -1544,6 +1544,7 @@ CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_SUITES += u-trailer
 CLAR_TEST_SUITES += u-urlmatch-normalization
 CLAR_TEST_SUITES += u-utf8-width
+CLAR_TEST_SUITES += u-odb-source
 CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
builtin/cat-file.c +6 -3
index b6f12f41d6..0c63705b72 100644
@@ -882,9 +882,12 @@ static void batch_each_object(struct batch_options *opt,
 		struct object_info oi = { 0 };
 
 		for (source = the_repository->objects->sources; source; source = source->next) {
-			struct odb_source_files *files = odb_source_files_downcast(source);
-			int ret = packfile_store_for_each_object(files->packed, &oi,
-								 batch_one_object_oi, &payload, flags);
+			struct odb_source_files *files = odb_source_files_try(source);
+			int ret;
+			if (!files)
+				continue;
+			ret = packfile_store_for_each_object(files->packed, &oi,
+							     batch_one_object_oi, &payload, flags);
 			if (ret)
 				break;
 		}
builtin/fast-import.c +6 -2
index a41f95191e..cef77ac937 100644
@@ -982,7 +982,9 @@ static int store_object(
 	}
 
 	for (source = the_repository->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
+		if (!files)
+			continue;
 
 		if (!packfile_list_find_oid(packfile_store_get_packs(files->packed), &oid))
 			continue;

@@ -1189,7 +1191,9 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
 	}
 
 	for (source = the_repository->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
+		if (!files)
+			continue;
 
 		if (!packfile_list_find_oid(packfile_store_get_packs(files->packed), &oid))
 			continue;
builtin/grep.c +3 -1
index e33285e5e6..ea41236ce5 100644
@@ -1219,7 +1219,9 @@ int cmd_grep(int argc,
 
 			odb_prepare_alternates(the_repository->objects);
 			for (source = the_repository->objects->sources; source; source = source->next) {
-				struct odb_source_files *files = odb_source_files_downcast(source);
+				struct odb_source_files *files = odb_source_files_try(source);
+				if (!files)
+					continue;
 				packfile_store_prepare(files->packed);
 			}
 		}
builtin/pack-objects.c +11 -4
index cd013c0b68..229e3e40b0 100644
@@ -1531,8 +1531,11 @@ static int want_cruft_object_mtime(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);
-		struct packed_git **cache = packfile_store_get_kept_pack_cache(files->packed, flags);
+		struct odb_source_files *files = odb_source_files_try(source);
+		struct packed_git **cache;
+		if (!files)
+			continue;
+		cache = packfile_store_get_kept_pack_cache(files->packed, flags);
 
 		for (; *cache; cache++) {
 			struct packed_git *p = *cache;

@@ -1754,7 +1757,9 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
 	}
 
 	for (source = the_repository->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
+		if (!files)
+			continue;
 
 		for (e = files->packed->packs.head; e; e = e->next) {
 			struct packed_git *p = e->pack;

@@ -4350,7 +4355,9 @@ static void add_objects_in_unpacked_packs(void)
 
 	odb_prepare_alternates(to_pack.repo->objects);
 	for (source = to_pack.repo->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
+		if (!files)
+			continue;
 
 		if (!source->local)
 			continue;
commit-graph.c +3 -1
index f8e24145a5..b425417735 100644
@@ -1981,7 +1981,9 @@ static void fill_oids_from_all_packs(struct write_commit_graph_context *ctx)
 
 	odb_prepare_alternates(ctx->r->objects);
 	for (source = ctx->r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
+		if (!files)
+			continue;
 		packfile_store_for_each_object(files->packed, &oi, add_packed_commits_oi,
 					       ctx, ODB_FOR_EACH_OBJECT_PACK_ORDER);
 	}
loose.c +9 -3
index 07333be696..1f036a8c2b 100644
@@ -64,10 +64,13 @@ static int insert_loose_map(struct odb_source *source,
 
 static int load_one_loose_object_map(struct repository *repo, struct odb_source *source)
 {
-	struct odb_source_files *files = odb_source_files_downcast(source);
+	struct odb_source_files *files = odb_source_files_try(source);
 	struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
 	FILE *fp;
 
+	if (!files)
+		return 0;
+
 	if (!files->loose->map)
 		loose_object_map_init(&files->loose->map);
 	if (!files->loose->cache) {

@@ -235,8 +238,11 @@ int repo_loose_object_map_oid(struct repository *repo,
 	khiter_t pos;
 
 	for (source = repo->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct loose_object_map *loose_map = files->loose->map;
+		struct odb_source_files *files = odb_source_files_try(source);
+		struct loose_object_map *loose_map;
+		if (!files)
+			continue;
+		loose_map = files->loose->map;
 		if (!loose_map)
 			continue;
 		map = (to == repo->compat_hash_algo) ?
midx.c +6 -2
index ab8e2611d1..d29b72f8d9 100644
@@ -95,7 +95,9 @@ static int midx_read_object_offsets(const unsigned char *chunk_start,
 
 struct multi_pack_index *get_multi_pack_index(struct odb_source *source)
 {
-	struct odb_source_files *files = odb_source_files_downcast(source);
+	struct odb_source_files *files = odb_source_files_try(source);
+	if (!files)
+		return NULL;
 	packfile_store_prepare(files->packed);
 	return files->packed->midx;
 }

@@ -806,7 +808,9 @@ void clear_midx_file(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);
+			struct odb_source_files *files = odb_source_files_try(source);
+			if (!files)
+				continue;
 			if (files->packed->midx)
 				close_midx(files->packed->midx);
 			files->packed->midx = NULL;
object-file.c +14 -6
index a3ff7f586c..11136ee199 100644
@@ -1879,14 +1879,20 @@ static int append_loose_object(const struct object_id *oid,
 struct oidtree *odb_source_loose_cache(struct odb_source *source,
 				       const struct object_id *oid)
 {
-	struct odb_source_files *files = odb_source_files_downcast(source);
-	int subdir_nr = oid->hash[0];
+	struct odb_source_files *files = odb_source_files_try(source);
+	int subdir_nr;
 	struct strbuf buf = STRBUF_INIT;
-	size_t word_bits = bitsizeof(files->loose->subdir_seen[0]);
-	size_t word_index = subdir_nr / word_bits;
-	size_t mask = (size_t)1u << (subdir_nr % word_bits);
+	size_t word_bits, word_index, mask;
 	uint32_t *bitmap;
 
+	if (!files)
+		return NULL;
+
+	subdir_nr = oid->hash[0];
+	word_bits = bitsizeof(files->loose->subdir_seen[0]);
+	word_index = subdir_nr / word_bits;
+	mask = (size_t)1u << (subdir_nr % word_bits);
+
 	if (subdir_nr < 0 ||
 	    (size_t) subdir_nr >= bitsizeof(files->loose->subdir_seen))
 		BUG("subdir_nr out of range");

@@ -1919,7 +1925,9 @@ static void odb_source_loose_clear_cache(struct odb_source_loose *loose)
 
 void odb_source_loose_reprepare(struct odb_source *source)
 {
-	struct odb_source_files *files = odb_source_files_downcast(source);
+	struct odb_source_files *files = odb_source_files_try(source);
+	if (!files)
+		return;
 	odb_source_loose_clear_cache(files->loose);
 }
object-name.c +5 -3
index 7b14c3bf9b..09835a3225 100644
@@ -115,9 +115,11 @@ static void find_short_object_filename(struct disambiguate_state *ds)
 {
 	struct odb_source *source;
 
-	for (source = ds->repo->objects->sources; source && !ds->ambiguous; source = source->next)
-		oidtree_each(odb_source_loose_cache(source, &ds->bin_pfx),
-				&ds->bin_pfx, ds->len, match_prefix, ds);
+	for (source = ds->repo->objects->sources; source && !ds->ambiguous; source = source->next) {
+		struct oidtree *tree = odb_source_loose_cache(source, &ds->bin_pfx);
+		if (tree)
+			oidtree_each(tree, &ds->bin_pfx, ds->len, match_prefix, ds);
+	}
 }
 
 static int match_hash(unsigned len, const unsigned char *a, const unsigned char *b)
packfile.c +17 -6
index 215a23e42b..d7416e577f 100644
@@ -363,7 +363,9 @@ static int unuse_one_window(struct object_database *odb)
 	struct pack_window *lru_w = NULL, *lru_l = NULL;
 
 	for (source = odb->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
+		if (!files)
+			continue;
 		for (e = files->packed->packs.head; e; e = e->next)
 			scan_windows(e->pack, &lru_p, &lru_w, &lru_l);
 	}

@@ -539,7 +541,9 @@ static int close_one_pack(struct repository *r)
 	int accept_windows_inuse = 1;
 
 	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
+		if (!files)
+			continue;
 		for (e = files->packed->packs.head; e; e = e->next) {
 			if (e->pack->pack_fd == -1)
 				continue;

@@ -1251,8 +1255,10 @@ const struct packed_git *has_packed_and_bad(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);
+		struct odb_source_files *files = odb_source_files_try(source);
 		struct packfile_list_entry *e;
+		if (!files)
+			continue;
 
 		for (e = files->packed->packs.head; e; e = e->next)
 			if (oidset_contains(&e->pack->bad_objects, oid))

@@ -2268,8 +2274,11 @@ int has_object_pack(struct repository *r, const struct object_id *oid)
 
 	odb_prepare_alternates(r->objects);
 	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		int ret = find_pack_entry(files->packed, oid, &e);
+		struct odb_source_files *files = odb_source_files_try(source);
+		int ret;
+		if (!files)
+			continue;
+		ret = find_pack_entry(files->packed, oid, &e);
 		if (ret)
 			return ret;
 	}

@@ -2284,8 +2293,10 @@ int has_object_kept_pack(struct repository *r, const struct object_id *oid,
 	struct pack_entry e;
 
 	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
+		struct odb_source_files *files = odb_source_files_try(source);
 		struct packed_git **cache;
+		if (!files)
+			continue;
 
 		cache = packfile_store_get_kept_pack_cache(files->packed, flags);
packfile.h +10 -4
index 8b04a258a7..3859415e4f 100644
@@ -193,8 +193,11 @@ static inline struct repo_for_each_pack_data repo_for_eack_pack_data_init(struct
 	odb_prepare_alternates(repo->objects);
 
 	for (struct odb_source *source = repo->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct packfile_list_entry *entry = packfile_store_get_packs(files->packed);
+		struct odb_source_files *files = odb_source_files_try(source);
+		struct packfile_list_entry *entry;
+		if (!files)
+			continue;
+		entry = packfile_store_get_packs(files->packed);
 		if (!entry)
 			continue;
 		data.source = source;

@@ -214,8 +217,11 @@ static inline void repo_for_each_pack_data_next(struct repo_for_each_pack_data *
 		return;
 
 	for (source = data->source->next; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct packfile_list_entry *entry = packfile_store_get_packs(files->packed);
+		struct odb_source_files *files = odb_source_files_try(source);
+		struct packfile_list_entry *entry;
+		if (!files)
+			continue;
+		entry = packfile_store_get_packs(files->packed);
 		if (!entry)
 			continue;
 		data->source = source;
t/meson.build +1 -0
index f66a73f8a0..e689ee49c7 100644
@@ -6,6 +6,7 @@ clar_test_suites = [
   'unit-tests/u-hashmap.c',
   'unit-tests/u-list-objects-filter-options.c',
   'unit-tests/u-mem-pool.c',
+  'unit-tests/u-odb-source.c',
   'unit-tests/u-oid-array.c',
   'unit-tests/u-oidmap.c',
   'unit-tests/u-oidtree.c',
t/unit-tests/u-odb-source.c new file +25 -0
new file mode 100644
index 0000000000..6f11c5342c
@@ -0,0 +1,25 @@
+#include "unit-test.h"
+#include "odb/source.h"
+#include "odb/source-files.h"
+
+/*
+ * Verify that odb_source_files_try() returns the files backend
+ * for ODB_SOURCE_FILES and NULL for other source types.
+ */
+void test_odb_source__try_returns_files_for_files_type(void)
+{
+	struct odb_source_files files_src;
+	memset(&files_src, 0, sizeof(files_src));
+	files_src.base.type = ODB_SOURCE_FILES;
+
+	cl_assert(odb_source_files_try(&files_src.base) == &files_src);
+}
+
+void test_odb_source__try_returns_null_for_unknown_type(void)
+{
+	struct odb_source other_src;
+	memset(&other_src, 0, sizeof(other_src));
+	other_src.type = ODB_SOURCE_UNKNOWN;
+
+	cl_assert(odb_source_files_try(&other_src) == NULL);
+}
Signature · 1 line
gitgitgadget
#3 lore raw

Re: [PATCH 2/2] odb: use odb_source_files_try() in source-chain iterations

Quoted 5 lines · Aaron Paterson via GitGitGadget

From: Aaron Paterson <[email protected]>

Convert all source-chain iteration sites that access files-specific internals (pack store, loose cache, MIDX) to use odb_source_files_try() instead of odb_source_files_downcast().

I have to wonder what the motivation for this is. We don't have any other sources (yet), and we're still in the process of bootstrapping pluggable object databases. So it's expected that things won't fully work yet that we'll die in lots of places if an alternate backend was in use.

But the solution to this isn't to simply skip non-files backends, but rather to adapt those sites so that they are properly abstracted.

Quoted 6 lines

These loops iterate the full source chain, which may include alternates. When a non-files backend is added to the chain (as an alternate or additional source), these sites must skip it rather than abort. The _try() helper returns NULL for non-files sources, and each site checks for NULL before accessing files-specific members.

The big question here is whether skipping is actually the correct thing to do, and in most cases I would claim it's probably not.

I'm a bit puzzled.

Patrick