Skip to content

odb: add odb_source_files_try() for heterogeneous source iteration #2068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MayCXC wants to merge 2 commits into from
Closed

odb: add odb_source_files_try() for heterogeneous source iteration #2068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6877b3
odb: add odb_source_files_try() for heterogeneous source iteration
MayCXC Mar 16, 2026
a8303b1
odb: use odb_source_files_try() in source-chain iterations
MayCXC Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,6 +1544,7 @@ CLAR_TEST_SUITES += u-strvec
CLAR_TEST_SUITES += u-trailer
Copy link
Copy Markdown

@gitgitgadget gitgitgadget Bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Mar 16, 2026 at 03:29:43PM +0000, Aaron Paterson via GitGitGadget wrote:
> From: Aaron Paterson <apaterson@pm.me>
> 
> 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.

> 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

Copy link
Copy Markdown

@gitgitgadget gitgitgadget Bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Mar 16, 2026 at 03:29:43PM +0000, Aaron Paterson via GitGitGadget wrote:
> From: Aaron Paterson <apaterson@pm.me>
> 
> 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.

> 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

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
Expand Down
9 changes: 6 additions & 3 deletions builtin/cat-file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
8 changes: 6 additions & 2 deletions builtin/fast-import.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion builtin/grep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
15 changes: 11 additions & 4 deletions builtin/pack-objects.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
12 changes: 9 additions & 3 deletions loose.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) ?
Expand Down
8 changes: 6 additions & 2 deletions midx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 14 additions & 6 deletions object-file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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);
}

Expand Down
8 changes: 5 additions & 3 deletions object-name.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 14 additions & 0 deletions odb/source-files.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
23 changes: 17 additions & 6 deletions packfile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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;
}
Expand All @@ -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);

Expand Down
14 changes: 10 additions & 4 deletions packfile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
1 change: 1 addition & 0 deletions t/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
25 changes: 25 additions & 0 deletions t/unit-tests/u-odb-source.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
Loading