Re: [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects
From: Justin Tobler <hidden>
Date: 2026-07-09 20:19:56
On 26/07/09 10:35AM, Patrick Steinhardt wrote:
In a subsequent commit we'll lift iteration of bitmapped objects into the "packed" backend and make it accessible via `odb_for_each_object()`. The calling convention for that function is that the callback may return a non-zero exit code, and if so we'll abort iteration. This is currently impossible to realize though, as `for_each_bitmapped_object()` will ignore any return value and just churn through all objects completely.
Ok.
This doesn't matter to the callers of `for_each_bitmapped_object()`, as there's only one of them in git-cat-file(1), and the callbacks we pass always return zero. But once we move the logic into the generic infrastructure it becomes a latent bug waiting to happen. Refactor the code so that the return value of the `show_reach` callback is not ignored anymore. Instead, returning a non-zero value will cause us to abort iteration in both `show_objects_for_type()` and in `for_each_bitmapped_object()`.
Make sense. We want to ensure that the `show_reach` callback can properly signal back to `for_each_bitmapped_object()` to abort.
quoted hunk ↗ jump to hunk
Note though that there's a second user of `show_objects_for_type()` with `traverse_bitmap_commit_list()`, and that function does indeed invoke callbacks that may return non-zero. This non-zero return value never had any effect at all though, and the callbacks that return non-zero values are only ever invoked via `traverse_bitmap_commit_list()`. Consequently, we adapt them to always return 0. Signed-off-by: Patrick Steinhardt <redacted> --- builtin/pack-objects.c | 2 +- builtin/rev-list.c | 2 +- pack-bitmap.c | 31 +++++++++++++++++++++---------- pack-bitmap.h | 3 ++- 4 files changed, 25 insertions(+), 13 deletions(-)diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index ea5eab4cf8..8ff92c5272 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c@@ -1909,7 +1909,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid, return 0; create_object_entry(oid, type, name_hash, 0, 0, pack, offset); - return 1; + return 0;
I wonder why this was even returning 1 to begin with? As you mentioned, the return value appears to be ignored anyways. I'm assuming it was signal that an object entry was created?
quoted hunk ↗ jump to hunk
} struct pbase_tree_cache {diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 8f63003709..02818b81c6 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c@@ -486,7 +486,7 @@ static int show_object_fast( void *payload UNUSED) { fprintf(stdout, "%s\n", oid_to_hex(oid)); - return 1; + return 0;
Also curious about this one too. It probably doesn't matter though.
quoted hunk ↗ jump to hunk
} static void print_disk_usage(off_t size)diff --git a/pack-bitmap.c b/pack-bitmap.c index a47c231632..eda38a5433 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c@@ -1695,7 +1695,7 @@ static void init_type_iterator(struct ewah_or_iterator *it, } } -static void show_objects_for_type( +static int show_objects_for_type( struct bitmap_index *bitmap_git, struct bitmap *objects, enum object_type object_type,@@ -1704,6 +1704,7 @@ static void show_objects_for_type( { size_t i = 0; uint32_t offset; + int ret; struct ewah_or_iterator it; eword_t filter;@@ -1749,11 +1750,17 @@ static void show_objects_for_type( hash = bitmap_name_hash(bitmap_git, index_pos); - show_reach(&oid, object_type, 0, hash, pack, ofs, payload); + ret = show_reach(&oid, object_type, 0, hash, pack, ofs, payload); + if (ret) + goto out;
The show_reach callback now wires back its return code.
quoted hunk ↗ jump to hunk
} } + ret = 0; + +out: ewah_or_iterator_release(&it); + return ret; } static int in_bitmapped_pack(struct bitmap_index *bitmap_git,@@ -2062,6 +2069,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git, show_reachable_fn show_reach, void *payload) { + const enum object_type types[] = { + OBJ_COMMIT, + OBJ_TREE, + OBJ_BLOB, + OBJ_TAG, + }; struct bitmap *filtered_bitmap = NULL; uint32_t objects_nr; size_t full_word_count;@@ -2086,14 +2099,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git, goto out; } - show_objects_for_type(bitmap_git, filtered_bitmap, - OBJ_COMMIT, show_reach, payload); - show_objects_for_type(bitmap_git, filtered_bitmap, - OBJ_TREE, show_reach, payload); - show_objects_for_type(bitmap_git, filtered_bitmap, - OBJ_BLOB, show_reach, payload); - show_objects_for_type(bitmap_git, filtered_bitmap, - OBJ_TAG, show_reach, payload); + for (size_t i = 0; i < ARRAY_SIZE(types); i++) { + ret = show_objects_for_type(bitmap_git, filtered_bitmap, + types[i], show_reach, payload); + if (ret) + goto out; + }
`for_each_bitmapped_object()` now has access to the underlying return code and can abort. Looks good. -Justin