[PATCH 0/7] odb: introduce object filters to `odb_for_each_object()`

[PATCH 0/7] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

Hi,

this patch series introduces object filters to `odb_for_each_object()`.
The intent of this is to make `git cat-file --batch-all-objects` work
with pluggable object databases. Right now it doesn't because it reaches
into internals of the "packed" backend to efficiently handle bitmapped
objects.

The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
info fields, 2026-07-02) merged into it.

Thanks!

Patrick

---
Patrick Steinhardt (7):
      odb/source-packed: improve lookup when enumerating objects
      pack-bitmap: mark object filter as `const`
      pack-bitmap: allow aborting iteration of bitmapped objects
      pack-bitmap: iterate object sources when opening bitmaps
      pack-bitmap: introduce function to open bitmap for a single source
      odb: introduce object filters to `odb_for_each_object()`
      builtin/cat-file: filter objects via object database

 builtin/cat-file.c     |  76 +++-----------------------------
 builtin/pack-objects.c |   2 +-
 builtin/rev-list.c     |   2 +-
 odb.h                  |  12 ++++++
 odb/source-packed.c    |  77 ++++++++++++++++++++++++++++++---
 pack-bitmap.c          | 115 ++++++++++++++++++++++++++++---------------------
 pack-bitmap.h          |  10 ++++-
 7 files changed, 164 insertions(+), 130 deletions(-)


---
base-commit: 3c8e2790f2ce15e8b5d4b4e6ced711b12649f32a
change-id: 20260708-pks-odb-for-each-object-filter-13286fa3523d

[PATCH 1/7] odb/source-packed: improve lookup when enumerating objects

[Patrick Steinhardt]

When iterating through packed objects via `odb_for_each_object()` we
do so via two different mechanisms:

  - When a multi-pack index is available we use that one to efficiently
    loop through all objects.

  - We then loop through all packfiles that aren't covered by a
    multi-pack index.

Regardless of which mechanism we use, we then iterate through all the
objects indexed by the respective data structure. Curiously though,
while we use the indices for enumerating the objects, we completely
ignore it for the actual object lookup. Instead, we call into the
generic `odb_source_read_object_info()` function, which will itself
consult the indices to figure out where the object in question even
lives.

This has two consequences:

  - It's inefficient, as we basically have to figure out the position of
    the object a second time.

  - It's subtly wrong, as it may now happen that a specific object will
    be looked up via a different pack in case it exists multiple times.

Fix the issue by using `packed_object_info()` directly. While at it,
rename the `store` variable to `source`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-packed.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0edea5356d..9cfa02b7a2 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -143,7 +143,7 @@ static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_f
 }
 
 static int for_each_prefixed_object_in_midx(
-	struct odb_source_packed *store,
+	struct odb_source_packed *source,
 	struct multi_pack_index *m,
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -170,6 +170,7 @@ static int for_each_prefixed_object_in_midx(
 		 */
 		for (i = first; i < num; i++) {
 			const struct object_id *current = NULL;
+			struct packed_git *pack;
 			struct object_id oid;
 
 			current = nth_midxed_object_oid(&oid, m, i);
@@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
 			if (!match_hash(len, opts->prefix->hash, current->hash))
 				break;
 
-			if (opts->flags) {
+			if (opts->flags || data->request) {
 				uint32_t pack_id = nth_midxed_pack_int_id(m, i);
-				struct packed_git *pack;
 
 				if (prepare_midx_pack(m, pack_id)) {
 					pack_errors = true;
@@ -193,9 +193,9 @@ static int for_each_prefixed_object_in_midx(
 
 			if (data->request) {
 				struct object_info oi = *data->request;
+				off_t offset = nth_midxed_offset(m, i);
 
-				ret = odb_source_read_object_info(&store->base, current,
-								  &oi, 0);
+				ret = packed_object_info(source, pack, offset, &oi);
 				if (ret)
 					goto out;
 
@@ -219,7 +219,7 @@ static int for_each_prefixed_object_in_midx(
 }
 
 static int for_each_prefixed_object_in_pack(
-	struct odb_source_packed *store,
+	struct odb_source_packed *source,
 	struct packed_git *p,
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -246,8 +246,9 @@ static int for_each_prefixed_object_in_pack(
 
 		if (data->request) {
 			struct object_info oi = *data->request;
+			off_t offset = nth_packed_object_offset(p, i);
 
-			ret = odb_source_read_object_info(&store->base, &oid, &oi, 0);
+			ret = packed_object_info(source, p, offset, &oi);
 			if (ret)
 				goto out;
 

-- 
2.55.0.175.ge4962bd3d5.dirty

[PATCH 2/7] pack-bitmap: mark object filter as `const`

[Patrick Steinhardt]

The function `for_each_bitmapped_object()` accepts an optional object
filter. This filter is never modified by the function, but is not
declared as `const`. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 6 +++---
 pack-bitmap.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 35774b6f0c..a47c231632 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1976,7 +1976,7 @@ static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
 static int filter_bitmap(struct bitmap_index *bitmap_git,
 			 struct object_list *tip_objects,
 			 struct bitmap *to_filter,
-			 struct list_objects_filter_options *filter)
+			 const struct list_objects_filter_options *filter)
 {
 	if (!filter || filter->choice == LOFC_DISABLED)
 		return 0;
@@ -2027,7 +2027,7 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int can_filter_bitmap(struct list_objects_filter_options *filter)
+static int can_filter_bitmap(const struct list_objects_filter_options *filter)
 {
 	return !filter_bitmap(NULL, NULL, NULL, filter);
 }
@@ -2058,7 +2058,7 @@ static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
 }
 
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
-			      struct list_objects_filter_options *filter,
+			      const struct list_objects_filter_options *filter,
 			      show_reachable_fn show_reach,
 			      void *payload)
 {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 19a8655457..47935eb24e 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -96,7 +96,7 @@ struct list_objects_filter_options;
  * not supported, `0` otherwise.
  */
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
-			      struct list_objects_filter_options *filter,
+			      const struct list_objects_filter_options *filter,
 			      show_reachable_fn show_reach,
 			      void *payload);
 

-- 
2.55.0.175.ge4962bd3d5.dirty

[PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects

[Patrick Steinhardt]

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.

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()`.

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 <ps@pks.im>
---
 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;
 }
 
 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;
 }
 
 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;
 		}
 	}
 
+	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;
+	}
 
 	ret = 0;
 out:
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 47935eb24e..ae8dc491ac 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -93,7 +93,8 @@ struct list_objects_filter_options;
 /*
  * Filter bitmapped objects and iterate through all resulting objects,
  * executing `show_reach` for each of them. Returns `-1` in case the filter is
- * not supported, `0` otherwise.
+ * not supported, `0` otherwise. Aborts iteration and bubbles up the return
+ * value in case `show_reach()` returns non-zero.
  */
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
 			      const struct list_objects_filter_options *filter,

-- 
2.55.0.175.ge4962bd3d5.dirty

[PATCH 4/7] pack-bitmap: iterate object sources when opening bitmaps

[Patrick Steinhardt]

When opening a bitmap for a repository we perform two steps:

  - We first look for a multi-pack index bitmap in any of the object
    sources connected to the repository.

  - We then look for a packfile bitmap in any of the packfiles of any of
    the object sources.

Both of these steps thus iterate through object sources themselves, one
via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
layout makes it hard to introduce a way to open the bitmap of one
specific object source, which is functionality that we'll require in a
subsequent commit.

Reverse the loop so that we instead loop through all sources in the
outer loop, and then for each source we try to load its bitmap via
either the multi-pack index or via a packfile.

Note that this changes the precedence of bitmaps in one specific edge
case: when an earlier object source only has a packfile bitmap, but a
later source has a multi-pack index bitmap, we now pick the packfile
bitmap of the earlier source. Previously, a multi-pack index bitmap from
any source would have taken precedence over all packfile bitmaps. Given
that object sources are ordered such that the local source comes first,
this arguably is an improvement, as we now prefer local bitmaps over
bitmaps in alternates. Furthermore, we already warn about repositories
that have multiple bitmaps, so this setup is broken and thus arguably
not worth worrying about too much.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 65 ++++++++++++++++++++++++++---------------------------------
 1 file changed, 29 insertions(+), 36 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index eda38a5433..0e3e18a557 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
 	return 0;
 }
 
-static int open_pack_bitmap(struct repository *r,
-			    struct bitmap_index *bitmap_git)
+static int open_bitmap_for_source(struct odb_source_packed *source,
+				  struct bitmap_index *bitmap_git)
 {
-	struct packed_git *p;
+	struct multi_pack_index *midx = get_multi_pack_index(source);
+	struct packfile_list_entry *e;
 	int ret = -1;
 
-	repo_for_each_pack(r, p) {
-		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
-			ret = 0;
-			/*
-			 * The only reason to keep looking is to report
-			 * duplicates.
-			 */
-			if (!trace2_is_enabled())
-				break;
-		}
+	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+		ret = 0;
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		/*
+		 * When tracing is enabled we want to keep looking to report
+		 * duplicates even if we have already found a bitmap.
+		 */
+		if (!ret && !trace2_is_enabled())
+			break;
+
+		if (open_pack_bitmap_1(bitmap_git, e->pack))
+			continue;
+		ret = 0;
 	}
 
 	return ret;
 }
 
-static int open_midx_bitmap(struct repository *r,
-			    struct bitmap_index *bitmap_git)
+static int open_bitmap(struct repository *r,
+		       struct bitmap_index *bitmap_git)
 {
 	struct odb_source *source;
-	int ret = -1;
+	int found = 0;
 
 	assert(!bitmap_git->map);
 
 	odb_prepare_alternates(r->objects);
 	for (source = r->objects->sources; source; source = source->next) {
 		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct multi_pack_index *midx = get_multi_pack_index(files->packed);
-		if (midx && !open_midx_bitmap_1(bitmap_git, midx))
-			ret = 0;
-	}
-	return ret;
-}
-
-static int open_bitmap(struct repository *r,
-		       struct bitmap_index *bitmap_git)
-{
-	int found;
 
-	assert(!bitmap_git->map);
+		found |= !open_bitmap_for_source(files->packed, bitmap_git);
 
-	found = !open_midx_bitmap(r, bitmap_git);
-
-	/*
-	 * these will all be skipped if we opened a midx bitmap; but run it
-	 * anyway if tracing is enabled to report the duplicates
-	 */
-	if (!found || trace2_is_enabled())
-		found |= !open_pack_bitmap(r, bitmap_git);
+		/*
+		 * The only reason to keep looking after having found a bitmap
+		 * is to report duplicates.
+		 */
+		if (found && !trace2_is_enabled())
+			break;
+	}
 
 	return found ? 0 : -1;
 }

-- 
2.55.0.175.ge4962bd3d5.dirty

[PATCH 5/7] pack-bitmap: introduce function to open bitmap for a single source

[Patrick Steinhardt]

The function `prepare_bitmap_git()` opens the first bitmap it can find
in any of the object sources connected to the repository. In a
subsequent commit, the "packed" object database backend will learn to
use bitmaps to answer object filters when enumerating objects. That
backend operates on a single object source though, so using a bitmap
that potentially belongs to a different source would be wrong:

  - The source would yield objects that are not part of the source
    itself.

  - The object source info would be attributed to the wrong source.

  - With multiple sources, each source would enumerate the same bitmap
    another time.

Introduce a new function `prepare_source_bitmap_git()` that only opens
bitmaps belonging to the given object source.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 12 ++++++++++++
 pack-bitmap.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 0e3e18a557..5d2af96e2f 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -753,6 +753,18 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
 	return NULL;
 }
 
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source)
+{
+	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+
+	if (!open_bitmap_for_source(source, bitmap_git) &&
+	    !load_bitmap(source->base.odb->repo, bitmap_git, 0))
+		return bitmap_git;
+
+	free_bitmap_index(bitmap_git);
+	return NULL;
+}
+
 int bitmap_index_contains_pack(struct bitmap_index *bitmap, struct packed_git *pack)
 {
 	for (; bitmap; bitmap = bitmap->base) {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index ae8dc491ac..9f20fb6e56 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -9,6 +9,7 @@
 #include "string-list.h"
 
 struct commit;
+struct odb_source_packed;
 struct repository;
 struct rev_info;
 
@@ -68,6 +69,7 @@ struct bitmapped_pack {
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r);
 struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx);
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source);
 
 /*
  * Given a bitmap index, determine whether it contains the pack either directly

-- 
2.55.0.175.ge4962bd3d5.dirty

[PATCH 6/7] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

The function `for_each_bitmapped_object()` can be used to iterate
through all objects covered by a bitmap. The benefit of this function is
that it allows the caller to efficiently handle some object filters. For
example, this can be used to filter out objects of a specific type with
some simple bitmap operations. But callers are currently required to
manually wire up the use of bitmaps though, and to do so they have to
reach into internals of a given object database source.

Introduce a new `struct odb_for_each_object_options::filter` field so
that the interface becomes generic. When set, then a backend may
optionally use the filter to skip some objects that it would have
otherwise yielded.

Note that the respective backends are free to ignore this field if they
cannot meaningfully optimize for a given filter, and consequently
callers need to verify whether they actually want the returned objects.
While annoying, we cannot easily lift this restriction anyway as the
object filter infrastructure supports some filters that cannot be
answered by the object database alone.

Implement the logic for the "packed" source. Note that we use the new
function `prepare_source_bitmap_git()` to open the bitmap: as the
backend operates on a single object source, we must only use bitmaps
that belong to that specific source. Otherwise we might yield objects
that are not part of the source at all, and with multiple sources we
would enumerate the same bitmap once per source.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.h               | 12 +++++++++++
 odb/source-packed.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 pack-bitmap.c       |  3 +--
 pack-bitmap.h       |  3 +++
 4 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/odb.h b/odb.h
index a1e222f605..67d0b34942 100644
--- a/odb.h
+++ b/odb.h
@@ -8,6 +8,7 @@
 #include "thread-utils.h"
 
 struct cached_object_entry;
+struct list_objects_filter_options;
 struct odb_source_inmemory;
 struct packed_git;
 struct repository;
@@ -490,6 +491,17 @@ struct odb_for_each_object_options {
 	 */
 	const struct object_id *prefix;
 	size_t prefix_hex_len;
+
+	/*
+	 * Optional object filter that allows backends to skip yielding
+	 * objects that are excluded by the filter as an optimization. The
+	 * filter is a best-effort hint: backends may use it to skip
+	 * excluded objects (e.g. by consulting a reachability bitmap), but
+	 * are also free to ignore it entirely and yield every object. As a
+	 * consequence, callers must re-apply the filter on yielded objects
+	 * if they require strict filtering semantics.
+	 */
+	const struct list_objects_filter_options *filter;
 };
 
 /*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 9cfa02b7a2..4777395053 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -3,11 +3,13 @@
 #include "chdir-notify.h"
 #include "dir.h"
 #include "git-zlib.h"
+#include "list-objects-filter-options.h"
 #include "mergesort.h"
 #include "midx.h"
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
 #include "packfile.h"
+#include "pack-bitmap.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -315,6 +317,37 @@ static int odb_source_packed_for_each_prefixed_object(
 	return ret;
 }
 
+struct bitmapped_for_each_object_data {
+	struct odb_source_packed *packed;
+	const struct object_info *request;
+	const struct odb_for_each_object_options *opts;
+	odb_for_each_object_cb cb;
+	void *cb_data;
+};
+
+static int bitmapped_for_each_object(const struct object_id *oid,
+				     enum object_type type UNUSED,
+				     int flags UNUSED,
+				     uint32_t hash UNUSED,
+				     struct packed_git *pack,
+				     off_t offset,
+				     void *cb_data)
+{
+	struct bitmapped_for_each_object_data *data = cb_data;
+
+	if (should_exclude_pack(pack, data->opts->flags))
+		return 0;
+
+	if (data->request) {
+		struct object_info oi = *data->request;
+		if (packed_object_info(data->packed, pack, offset, &oi) < 0)
+			return -1;
+		return data->cb(oid, &oi, data->cb_data);
+	}
+
+	return data->cb(oid, NULL, data->cb_data);
+}
+
 static int odb_source_packed_for_each_object(struct odb_source *source,
 					     const struct object_info *request,
 					     odb_for_each_object_cb cb,
@@ -328,12 +361,33 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 		.cb = cb,
 		.cb_data = cb_data,
 	};
+	struct bitmap_index *bitmap = NULL;
 	struct packfile_list_entry *e;
 	int pack_errors = 0, ret;
 
 	if (opts->prefix)
 		return odb_source_packed_for_each_prefixed_object(packed, opts, &data);
 
+	if (opts->filter &&
+	    opts->filter->choice != LOFC_DISABLED &&
+	    can_filter_bitmap(opts->filter))
+		bitmap = prepare_bitmap_git_for_source(packed);
+	if (bitmap) {
+		struct bitmapped_for_each_object_data bitmap_data = {
+			.packed = packed,
+			.request = request,
+			.opts = opts,
+			.cb = cb,
+			.cb_data = cb_data,
+		};
+
+		ret = for_each_bitmapped_object(bitmap, opts->filter,
+						bitmapped_for_each_object,
+						&bitmap_data);
+		if (ret)
+			goto out;
+	}
+
 	packed->skip_mru_updates = true;
 
 	for (e = packfile_store_get_packs(packed); e; e = e->next) {
@@ -342,6 +396,13 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 		if (should_exclude_pack(p, opts->flags))
 			continue;
 
+		/*
+		 * Objects covered by the bitmap have already been yielded
+		 * above; skip them here to avoid duplicates.
+		 */
+		if (bitmap && bitmap_index_contains_pack(bitmap, p))
+			continue;
+
 		if (open_pack_index(p)) {
 			pack_errors = 1;
 			continue;
@@ -357,6 +418,7 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 
 out:
 	packed->skip_mru_updates = false;
+	free_bitmap_index(bitmap);
 
 	if (!ret && pack_errors)
 		ret = -1;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 5d2af96e2f..ac9da9545f 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2039,12 +2039,11 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int can_filter_bitmap(const struct list_objects_filter_options *filter)
+bool can_filter_bitmap(const struct list_objects_filter_options *filter)
 {
 	return !filter_bitmap(NULL, NULL, NULL, filter);
 }
 
-
 static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
 					      struct bitmap *result)
 {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 9f20fb6e56..1385027c1f 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -92,6 +92,9 @@ int test_bitmap_pseudo_merge_objects(struct repository *r, uint32_t n);
 
 struct list_objects_filter_options;
 
+/* Check whether the filter can be computed via the bitmap. */
+bool can_filter_bitmap(const struct list_objects_filter_options *filter);
+
 /*
  * Filter bitmapped objects and iterate through all resulting objects,
  * executing `show_reach` for each of them. Returns `-1` in case the filter is

-- 
2.55.0.175.ge4962bd3d5.dirty

[PATCH 7/7] builtin/cat-file: filter objects via object database

[Patrick Steinhardt]

Refactor git-cat-file(1) to use the new object filter option when
batching all objects. This significantly simplifies the logic and
ensures that we don't have to reach into internals of the "files" source
anymore.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/cat-file.c | 76 +++++-------------------------------------------------
 1 file changed, 7 insertions(+), 69 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b4b99a73da..1458dd76d6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,7 +20,6 @@
 #include "userdiff.h"
 #include "oid-array.h"
 #include "packfile.h"
-#include "pack-bitmap.h"
 #include "object-file.h"
 #include "object-name.h"
 #include "odb.h"
@@ -844,28 +843,6 @@ static int batch_one_object_oi(const struct object_id *oid,
 	return payload->callback(oid, NULL, 0, payload->payload);
 }
 
-static int batch_one_object_packed(const struct object_id *oid,
-				   struct packed_git *pack,
-				   uint32_t pos,
-				   void *_payload)
-{
-	struct for_each_object_payload *payload = _payload;
-	return payload->callback(oid, pack, nth_packed_object_offset(pack, pos),
-				 payload->payload);
-}
-
-static int batch_one_object_bitmapped(const struct object_id *oid,
-				      enum object_type type UNUSED,
-				      int flags UNUSED,
-				      uint32_t hash UNUSED,
-				      struct packed_git *pack,
-				      off_t offset,
-				      void *_payload)
-{
-	struct for_each_object_payload *payload = _payload;
-	return payload->callback(oid, pack, offset, payload->payload);
-}
-
 static void batch_each_object(struct batch_options *opt,
 			      for_each_object_fn callback,
 			      unsigned flags,
@@ -875,56 +852,17 @@ static void batch_each_object(struct batch_options *opt,
 		.callback = callback,
 		.payload = _payload,
 	};
+	struct odb_source_info source_info;
+	struct object_info oi = {
+		.source_infop = &source_info,
+	};
 	struct odb_for_each_object_options opts = {
 		.flags = flags,
+		.filter = &opt->objects_filter,
 	};
-	struct bitmap_index *bitmap = NULL;
-	struct odb_source *source;
-
-	/*
-	 * TODO: we still need to tap into implementation details of the object
-	 * database sources. Ideally, we should extend `odb_for_each_object()`
-	 * to handle object filters itself so that we can move the filtering
-	 * logic into the individual sources.
-	 */
-	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);
-		int ret = odb_source_for_each_object(&files->loose->base, NULL, batch_one_object_oi,
-						     &payload, &opts);
-		if (ret)
-			break;
-	}
-
-	if (opt->objects_filter.choice != LOFC_DISABLED &&
-	    (bitmap = prepare_bitmap_git(the_repository)) &&
-	    !for_each_bitmapped_object(bitmap, &opt->objects_filter,
-				       batch_one_object_bitmapped, &payload)) {
-		struct packed_git *pack;
-
-		repo_for_each_pack(the_repository, pack) {
-			if (bitmap_index_contains_pack(bitmap, pack) ||
-			    open_pack_index(pack))
-				continue;
-			for_each_object_in_pack(pack, batch_one_object_packed,
-						&payload, flags);
-		}
-	} else {
-		struct odb_source_info source_info;
-		struct object_info oi = {
-			.source_infop = &source_info,
-		};
-
-		for (source = the_repository->objects->sources; source; source = source->next) {
-			struct odb_source_files *files = odb_source_files_downcast(source);
-			int ret = odb_source_for_each_object(&files->packed->base, &oi,
-							     batch_one_object_oi, &payload, &opts);
-			if (ret)
-				break;
-		}
-	}
 
-	free_bitmap_index(bitmap);
+	odb_for_each_object_ext(the_repository->objects, &oi,
+				batch_one_object_oi, &payload, &opts);
 }
 
 static int batch_objects(struct batch_options *opt)

-- 
2.55.0.175.ge4962bd3d5.dirty

Re: [PATCH 7/7] builtin/cat-file: filter objects via object database

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Refactor git-cat-file(1) to use the new object filter option when
> batching all objects. This significantly simplifies the logic and
> ensures that we don't have to reach into internals of the "files" source
> anymore.

This would become more convincing if you spent a few lines before
presenting the solution to give an observation of what the current
code does, e.g.,

    When batching all objects, git-cat-file(1) reaches into the
    internals of the object database and manually manages bitmaps to
    apply object filters. This creates coupling between the command
    and ODB backend internals.

to highlight the perceived problem in it.  That would flow naturally
to the description of your solution.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/cat-file.c | 76 +++++-------------------------------------------------
>  1 file changed, 7 insertions(+), 69 deletions(-)

Very nice.

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index b4b99a73da..1458dd76d6 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -20,7 +20,6 @@
>  #include "userdiff.h"
>  #include "oid-array.h"
>  #include "packfile.h"
> -#include "pack-bitmap.h"
>  #include "object-file.h"
>  #include "object-name.h"
>  #include "odb.h"
> @@ -844,28 +843,6 @@ static int batch_one_object_oi(const struct object_id *oid,
>  	return payload->callback(oid, NULL, 0, payload->payload);
>  }
>  
> -static int batch_one_object_packed(const struct object_id *oid,
> -				   struct packed_git *pack,
> -				   uint32_t pos,
> -				   void *_payload)
> -{
> -	struct for_each_object_payload *payload = _payload;
> -	return payload->callback(oid, pack, nth_packed_object_offset(pack, pos),
> -				 payload->payload);
> -}
> -
> -static int batch_one_object_bitmapped(const struct object_id *oid,
> -				      enum object_type type UNUSED,
> -				      int flags UNUSED,
> -				      uint32_t hash UNUSED,
> -				      struct packed_git *pack,
> -				      off_t offset,
> -				      void *_payload)
> -{
> -	struct for_each_object_payload *payload = _payload;
> -	return payload->callback(oid, pack, offset, payload->payload);
> -}
> -
>  static void batch_each_object(struct batch_options *opt,
>  			      for_each_object_fn callback,
>  			      unsigned flags,
> @@ -875,56 +852,17 @@ static void batch_each_object(struct batch_options *opt,
>  		.callback = callback,
>  		.payload = _payload,
>  	};
> +	struct odb_source_info source_info;
> +	struct object_info oi = {
> +		.source_infop = &source_info,
> +	};
>  	struct odb_for_each_object_options opts = {
>  		.flags = flags,
> +		.filter = &opt->objects_filter,
>  	};
> -	struct bitmap_index *bitmap = NULL;
> -	struct odb_source *source;
> -
> -	/*
> -	 * TODO: we still need to tap into implementation details of the object
> -	 * database sources. Ideally, we should extend `odb_for_each_object()`
> -	 * to handle object filters itself so that we can move the filtering
> -	 * logic into the individual sources.
> -	 */
> -	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);
> -		int ret = odb_source_for_each_object(&files->loose->base, NULL, batch_one_object_oi,
> -						     &payload, &opts);
> -		if (ret)
> -			break;
> -	}
> -
> -	if (opt->objects_filter.choice != LOFC_DISABLED &&
> -	    (bitmap = prepare_bitmap_git(the_repository)) &&
> -	    !for_each_bitmapped_object(bitmap, &opt->objects_filter,
> -				       batch_one_object_bitmapped, &payload)) {
> -		struct packed_git *pack;
> -
> -		repo_for_each_pack(the_repository, pack) {
> -			if (bitmap_index_contains_pack(bitmap, pack) ||
> -			    open_pack_index(pack))
> -				continue;
> -			for_each_object_in_pack(pack, batch_one_object_packed,
> -						&payload, flags);
> -		}
> -	} else {
> -		struct odb_source_info source_info;
> -		struct object_info oi = {
> -			.source_infop = &source_info,
> -		};
> -
> -		for (source = the_repository->objects->sources; source; source = source->next) {
> -			struct odb_source_files *files = odb_source_files_downcast(source);
> -			int ret = odb_source_for_each_object(&files->packed->base, &oi,
> -							     batch_one_object_oi, &payload, &opts);
> -			if (ret)
> -				break;
> -		}
> -	}
>  
> -	free_bitmap_index(bitmap);
> +	odb_for_each_object_ext(the_repository->objects, &oi,
> +				batch_one_object_oi, &payload, &opts);
>  }
>  
>  static int batch_objects(struct batch_options *opt)

Re: [PATCH 1/7] odb/source-packed: improve lookup when enumerating objects

[Justin Tobler]

On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> When iterating through packed objects via `odb_for_each_object()` we
> do so via two different mechanisms:
> 
>   - When a multi-pack index is available we use that one to efficiently
>     loop through all objects.
> 
>   - We then loop through all packfiles that aren't covered by a
>     multi-pack index.

To be specific, we are talking only about the for_each_object callback
for the packed source `odb_source_packed_for_each_object()` correct?
Also, this appears to only matter when we are enumerating OIDs with a
specific prefix.

> Regardless of which mechanism we use, we then iterate through all the
> objects indexed by the respective data structure. Curiously though,
> while we use the indices for enumerating the objects, we completely
> ignore it for the actual object lookup. Instead, we call into the
> generic `odb_source_read_object_info()` function, which will itself
> consult the indices to figure out where the object in question even
> lives.
> 
> This has two consequences:
> 
>   - It's inefficient, as we basically have to figure out the position of
>     the object a second time.

Since we already have the position from the index, there is no need to
start over. Makes sense.

>   - It's subtly wrong, as it may now happen that a specific object will
>     be looked up via a different pack in case it exists multiple times.

Naive question: Is there any real harm in reading the same object, but
from a different packfile here?

Regardless I do think it's a good idea to just reuse the same packfile
to get the same object here.

> Fix the issue by using `packed_object_info()` directly. While at it,
> rename the `store` variable to `source`.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-packed.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 0edea5356d..9cfa02b7a2 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -143,7 +143,7 @@ static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_f
>  }
>  
>  static int for_each_prefixed_object_in_midx(
> -	struct odb_source_packed *store,
> +	struct odb_source_packed *source,
>  	struct multi_pack_index *m,
>  	const struct odb_for_each_object_options *opts,
>  	struct odb_source_packed_for_each_object_wrapper_data *data)
> @@ -170,6 +170,7 @@ static int for_each_prefixed_object_in_midx(
>  		 */
>  		for (i = first; i < num; i++) {
>  			const struct object_id *current = NULL;
> +			struct packed_git *pack;
>  			struct object_id oid;
>  
>  			current = nth_midxed_object_oid(&oid, m, i);
> @@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
>  			if (!match_hash(len, opts->prefix->hash, current->hash))
>  				break;
>  
> -			if (opts->flags) {
> +			if (opts->flags || data->request) {

I'm not sure I follow why the above condition needed to change.

>  				uint32_t pack_id = nth_midxed_pack_int_id(m, i);
> -				struct packed_git *pack;
>  
>  				if (prepare_midx_pack(m, pack_id)) {
>  					pack_errors = true;
> @@ -193,9 +193,9 @@ static int for_each_prefixed_object_in_midx(
>  
>  			if (data->request) {
>  				struct object_info oi = *data->request;
> +				off_t offset = nth_midxed_offset(m, i);
>  
> -				ret = odb_source_read_object_info(&store->base, current,
> -								  &oi, 0);
> +				ret = packed_object_info(source, pack, offset, &oi);

We not longer use the generic function to read object info. This ensures
the exact same object is read.

>  				if (ret)
>  					goto out;
>  
> @@ -219,7 +219,7 @@ static int for_each_prefixed_object_in_midx(
>  }
>  
>  static int for_each_prefixed_object_in_pack(
> -	struct odb_source_packed *store,
> +	struct odb_source_packed *source,
>  	struct packed_git *p,
>  	const struct odb_for_each_object_options *opts,
>  	struct odb_source_packed_for_each_object_wrapper_data *data)
> @@ -246,8 +246,9 @@ static int for_each_prefixed_object_in_pack(
>  
>  		if (data->request) {
>  			struct object_info oi = *data->request;
> +			off_t offset = nth_packed_object_offset(p, i);
>  
> -			ret = odb_source_read_object_info(&store->base, &oid, &oi, 0);
> +			ret = packed_object_info(source, p, offset, &oi);

And we do the same thing here when reading the object from a packfile.

-Justin

Re: [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects

[Justin Tobler]

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.

> 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 <ps@pks.im>
> ---
>  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?

>  }
>  
>  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.

>  }
>  
>  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.

>  		}
>  	}
>  
> +	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

Re: [PATCH 4/7] pack-bitmap: iterate object sources when opening bitmaps

[Justin Tobler]

On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> When opening a bitmap for a repository we perform two steps:
> 
>   - We first look for a multi-pack index bitmap in any of the object
>     sources connected to the repository.
> 
>   - We then look for a packfile bitmap in any of the packfiles of any of
>     the object sources.

So IIUC, we generally stop searching for a bitmap once we find one.

> Both of these steps thus iterate through object sources themselves, one
> via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
> layout makes it hard to introduce a way to open the bitmap of one
> specific object source, which is functionality that we'll require in a
> subsequent commit.
> 
> Reverse the loop so that we instead loop through all sources in the
> outer loop, and then for each source we try to load its bitmap via
> either the multi-pack index or via a packfile.

Conceptually, I think this is a lot easier to follow too which is nice.

> Note that this changes the precedence of bitmaps in one specific edge
> case: when an earlier object source only has a packfile bitmap, but a
> later source has a multi-pack index bitmap, we now pick the packfile
> bitmap of the earlier source. Previously, a multi-pack index bitmap from
> any source would have taken precedence over all packfile bitmaps. Given
> that object sources are ordered such that the local source comes first,
> this arguably is an improvement, as we now prefer local bitmaps over
> bitmaps in alternates. Furthermore, we already warn about repositories
> that have multiple bitmaps, so this setup is broken and thus arguably
> not worth worrying about too much.

I agree that the change in bitmap precedent is probably not a big deal.
Having multiple bitmaps in a repository is already something we warn
against so I think this should be fine.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  pack-bitmap.c | 65 ++++++++++++++++++++++++++---------------------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index eda38a5433..0e3e18a557 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
>  	return 0;
>  }
>  
> -static int open_pack_bitmap(struct repository *r,
> -			    struct bitmap_index *bitmap_git)
> +static int open_bitmap_for_source(struct odb_source_packed *source,
> +				  struct bitmap_index *bitmap_git)
>  {
> -	struct packed_git *p;
> +	struct multi_pack_index *midx = get_multi_pack_index(source);
> +	struct packfile_list_entry *e;
>  	int ret = -1;
>  
> -	repo_for_each_pack(r, p) {
> -		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
> -			ret = 0;
> -			/*
> -			 * The only reason to keep looking is to report
> -			 * duplicates.
> -			 */
> -			if (!trace2_is_enabled())
> -				break;
> -		}
> +	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
> +		ret = 0;

Ok, open_midx_bitmap_1() returns 0 if it find a MIDX and -1 otherwise.
Probably just a matter of preference, but I think writing out like below
is a little bit easier on the eyes:

  if (midx)
    ret = open_midx_bitmap_1(bitmap_git, midx);

it might just be that I find the return values a bit confusing though.
Maybe we could instead use `found` like a bit later in this patch.

> +
> +	for (e = packfile_store_get_packs(source); e; e = e->next) {
> +		/*
> +		 * When tracing is enabled we want to keep looking to report
> +		 * duplicates even if we have already found a bitmap.
> +		 */
> +		if (!ret && !trace2_is_enabled())
> +			break;

So if have already found a bitmap from the MIDX and tracing is not
enabled, we don't continue searching for bitmaps in this source. 

> +
> +		if (open_pack_bitmap_1(bitmap_git, e->pack))
> +			continue;
> +		ret = 0;
>  	}
>  
>  	return ret;
>  }
>  
> -static int open_midx_bitmap(struct repository *r,
> -			    struct bitmap_index *bitmap_git)
> +static int open_bitmap(struct repository *r,
> +		       struct bitmap_index *bitmap_git)
>  {
>  	struct odb_source *source;
> -	int ret = -1;
> +	int found = 0;
>  
>  	assert(!bitmap_git->map);
>  
>  	odb_prepare_alternates(r->objects);
>  	for (source = r->objects->sources; source; source = source->next) {
>  		struct odb_source_files *files = odb_source_files_downcast(source);
> -		struct multi_pack_index *midx = get_multi_pack_index(files->packed);
> -		if (midx && !open_midx_bitmap_1(bitmap_git, midx))
> -			ret = 0;
> -	}
> -	return ret;
> -}
> -
> -static int open_bitmap(struct repository *r,
> -		       struct bitmap_index *bitmap_git)
> -{
> -	int found;
>  
> -	assert(!bitmap_git->map);
> +		found |= !open_bitmap_for_source(files->packed, bitmap_git);
>  
> -	found = !open_midx_bitmap(r, bitmap_git);
> -
> -	/*
> -	 * these will all be skipped if we opened a midx bitmap; but run it
> -	 * anyway if tracing is enabled to report the duplicates
> -	 */
> -	if (!found || trace2_is_enabled())
> -		found |= !open_pack_bitmap(r, bitmap_git);
> +		/*
> +		 * The only reason to keep looking after having found a bitmap
> +		 * is to report duplicates.
> +		 */
> +		if (found && !trace2_is_enabled())
> +			break;
> +	}

Ok, we only advance to the next source if tracing is enabled to print
warnings for multiple bitmaps. Makes sense.

Overall I quite like the direction of this patch.

-Justin

Re: [PATCH 6/7] odb: introduce object filters to `odb_for_each_object()`

[Justin Tobler]

On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> The function `for_each_bitmapped_object()` can be used to iterate
> through all objects covered by a bitmap. The benefit of this function is
> that it allows the caller to efficiently handle some object filters. For
> example, this can be used to filter out objects of a specific type with
> some simple bitmap operations. But callers are currently required to
> manually wire up the use of bitmaps though, and to do so they have to
> reach into internals of a given object database source.
> 
> Introduce a new `struct odb_for_each_object_options::filter` field so
> that the interface becomes generic. When set, then a backend may
> optionally use the filter to skip some objects that it would have
> otherwise yielded.
> 
> Note that the respective backends are free to ignore this field if they
> cannot meaningfully optimize for a given filter, and consequently
> callers need to verify whether they actually want the returned objects.
> While annoying, we cannot easily lift this restriction anyway as the
> object filter infrastructure supports some filters that cannot be
> answered by the object database alone.

Huh, this feels rather awkward. So callers will always still have to
ensure correctness by filtering the result a second time? IIUC, the idea
is that the backend may be able to more efficiently process object
filtering so we would want it to attempt the first pass.

Is there a subset of object filters that we should expect any backend to
be able to answer? If so, maybe we should define a separate list of
object filter options specific to this interface? Any filtering not
supported would have to be deligated to the caller then.

-Justin

Re: [PATCH 1/7] odb/source-packed: improve lookup when enumerating objects

[Patrick Steinhardt]

On Thu, Jul 09, 2026 at 02:54:17PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > When iterating through packed objects via `odb_for_each_object()` we
> > do so via two different mechanisms:
> > 
> >   - When a multi-pack index is available we use that one to efficiently
> >     loop through all objects.
> > 
> >   - We then loop through all packfiles that aren't covered by a
> >     multi-pack index.
> 
> To be specific, we are talking only about the for_each_object callback
> for the packed source `odb_source_packed_for_each_object()` correct?
> Also, this appears to only matter when we are enumerating OIDs with a
> specific prefix.

Yeah, true. I'll clarify this a bit.

> > Regardless of which mechanism we use, we then iterate through all the
> > objects indexed by the respective data structure. Curiously though,
> > while we use the indices for enumerating the objects, we completely
> > ignore it for the actual object lookup. Instead, we call into the
> > generic `odb_source_read_object_info()` function, which will itself
> > consult the indices to figure out where the object in question even
> > lives.
> > 
> > This has two consequences:
> > 
> >   - It's inefficient, as we basically have to figure out the position of
> >     the object a second time.
> 
> Since we already have the position from the index, there is no need to
> start over. Makes sense.
> 
> >   - It's subtly wrong, as it may now happen that a specific object will
> >     be looked up via a different pack in case it exists multiple times.
> 
> Naive question: Is there any real harm in reading the same object, but
> from a different packfile here?

The answer is probably "no". At least I cannot think of any case where
it'd really matter, but semantically it's the wrong thing to do anyway.

> > diff --git a/odb/source-packed.c b/odb/source-packed.c
> > index 0edea5356d..9cfa02b7a2 100644
> > --- a/odb/source-packed.c
> > +++ b/odb/source-packed.c
> > @@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
> >  			if (!match_hash(len, opts->prefix->hash, current->hash))
> >  				break;
> >  
> > -			if (opts->flags) {
> > +			if (opts->flags || data->request) {
> 
> I'm not sure I follow why the above condition needed to change.

This needs to change because we now require access to the pack so that
we can call `packed_object_info()`. Otherwise the pack would be not be
populated if we're invoked without any flags.

Patrick

Re: [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects

[Patrick Steinhardt]

On Thu, Jul 09, 2026 at 03:19:52PM -0500, Justin Tobler wrote:
> > 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?

The function is only called from a single location, and the return value
was completely ignored until this commit. It has always been this way
since the function was originally introduced in 6b8fda2db1
(pack-objects: use bitmaps when packing objects, 2013-12-21), so it
never seemed to have any purpose. The commit message doesn't mention
anything either.

> > 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.

Likewise, this was introduced in aa32939fea (rev-list: add bitmap mode
to speed up object lists, 2013-12-21), and the return value wasn't ever
used for anything.

Patrick

Re: [PATCH 4/7] pack-bitmap: iterate object sources when opening bitmaps

[Patrick Steinhardt]

On Thu, Jul 09, 2026 at 04:08:31PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > When opening a bitmap for a repository we perform two steps:
> > 
> >   - We first look for a multi-pack index bitmap in any of the object
> >     sources connected to the repository.
> > 
> >   - We then look for a packfile bitmap in any of the packfiles of any of
> >     the object sources.
> 
> So IIUC, we generally stop searching for a bitmap once we find one.

Except that we continue searching so that we can print a warning, but
later results are simply being ignored.

> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index eda38a5433..0e3e18a557 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
> >  	return 0;
> >  }
> >  
> > -static int open_pack_bitmap(struct repository *r,
> > -			    struct bitmap_index *bitmap_git)
> > +static int open_bitmap_for_source(struct odb_source_packed *source,
> > +				  struct bitmap_index *bitmap_git)
> >  {
> > -	struct packed_git *p;
> > +	struct multi_pack_index *midx = get_multi_pack_index(source);
> > +	struct packfile_list_entry *e;
> >  	int ret = -1;
> >  
> > -	repo_for_each_pack(r, p) {
> > -		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
> > -			ret = 0;
> > -			/*
> > -			 * The only reason to keep looking is to report
> > -			 * duplicates.
> > -			 */
> > -			if (!trace2_is_enabled())
> > -				break;
> > -		}
> > +	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
> > +		ret = 0;
> 
> Ok, open_midx_bitmap_1() returns 0 if it find a MIDX and -1 otherwise.
> Probably just a matter of preference, but I think writing out like below
> is a little bit easier on the eyes:
> 
>   if (midx)
>     ret = open_midx_bitmap_1(bitmap_git, midx);
> 
> it might just be that I find the return values a bit confusing though.
> Maybe we could instead use `found` like a bit later in this patch.

That's fair, and the result would be both easier to reason about and
more consistent indeed. I'll adapt this accordingly.

Patrick

Re: [PATCH 6/7] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

On Thu, Jul 09, 2026 at 04:43:58PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > The function `for_each_bitmapped_object()` can be used to iterate
> > through all objects covered by a bitmap. The benefit of this function is
> > that it allows the caller to efficiently handle some object filters. For
> > example, this can be used to filter out objects of a specific type with
> > some simple bitmap operations. But callers are currently required to
> > manually wire up the use of bitmaps though, and to do so they have to
> > reach into internals of a given object database source.
> > 
> > Introduce a new `struct odb_for_each_object_options::filter` field so
> > that the interface becomes generic. When set, then a backend may
> > optionally use the filter to skip some objects that it would have
> > otherwise yielded.
> > 
> > Note that the respective backends are free to ignore this field if they
> > cannot meaningfully optimize for a given filter, and consequently
> > callers need to verify whether they actually want the returned objects.
> > While annoying, we cannot easily lift this restriction anyway as the
> > object filter infrastructure supports some filters that cannot be
> > answered by the object database alone.
> 
> Huh, this feels rather awkward. So callers will always still have to
> ensure correctness by filtering the result a second time? IIUC, the idea
> is that the backend may be able to more efficiently process object
> filtering so we would want it to attempt the first pass.
> 
> Is there a subset of object filters that we should expect any backend to
> be able to answer? If so, maybe we should define a separate list of
> object filter options specific to this interface? Any filtering not
> supported would have to be deligated to the caller then.

It's a bit awkward, but it's also similar to how we handle this for
example in the reference backends with the exclude patterns. I don't
really think it makes sense to enforce that backends may only handle a
subset of object filters that we know the current backends support, as
that would artificially limit us.

For example, the "loose" backend already cannot efficiently handle many
of the filters that the "packed" backend can handle, like for example
filtering by type. So ultimately, the subset of filters that can be
handled efficiently by both backends is empty. And as the "files"
backend always combines both of these backends we wouldn't be able to
ever use the object filter at all there.

The same could be true for any future backend: we cannot assume how they
store their objects, so they might be able to efficiently handle filters
that the current backends cannot.

An alternative going forward could be to perform filtering of yielded
objects inside `odb_for_each_object()` itself so that it will filter out
any objects that the backends themselves couldn't filter efficiently.
But I'm not sure I want to go there as part of this series -- we only
have a single caller anyway that iterates with a filter, and that caller
already knows to manually filter references.

I'll add a bit of an explanation to the commit message.

Patrick

Re: [PATCH 7/7] builtin/cat-file: filter objects via object database

[Patrick Steinhardt]

On Thu, Jul 09, 2026 at 11:59:29AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Refactor git-cat-file(1) to use the new object filter option when
> > batching all objects. This significantly simplifies the logic and
> > ensures that we don't have to reach into internals of the "files" source
> > anymore.
> 
> This would become more convincing if you spent a few lines before
> presenting the solution to give an observation of what the current
> code does, e.g.,
> 
>     When batching all objects, git-cat-file(1) reaches into the
>     internals of the object database and manually manages bitmaps to
>     apply object filters. This creates coupling between the command
>     and ODB backend internals.
> 
> to highlight the perceived problem in it.  That would flow naturally
> to the description of your solution.

Good point, will add.

Patrick

[PATCH v2 0/8] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

Hi,

this patch series introduces object filters to `odb_for_each_object()`.
The intent of this is to make `git cat-file --batch-all-objects` work
with pluggable object databases. Right now it doesn't because it reaches
into internals of the "packed" backend to efficiently handle bitmapped
objects.

The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
info fields, 2026-07-02) merged into it.

Changes in v2:
  - Add another patch to drop the `_1()` prefixes that aren't required
    anymore.
  - Change the approach in `open_bitmap_for_source()` to also use a
    `found` boolean instead of a confusing integer.
  - Add some more explanations to commit messages.
  - Link to v1: https://patch.msgid.link/20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (8):
      odb/source-packed: improve lookup when enumerating objects
      pack-bitmap: mark object filter as `const`
      pack-bitmap: allow aborting iteration of bitmapped objects
      pack-bitmap: iterate object sources when opening bitmaps
      pack-bitmap: drop `_1` suffix from functions that open bitmaps
      pack-bitmap: introduce function to open bitmap for a single source
      odb: introduce object filters to `odb_for_each_object()`
      builtin/cat-file: filter objects via object database

 builtin/cat-file.c     |  76 +++--------------------------
 builtin/pack-objects.c |   2 +-
 builtin/rev-list.c     |   2 +-
 odb.h                  |  12 +++++
 odb/source-packed.c    |  77 ++++++++++++++++++++++++++---
 pack-bitmap.c          | 129 +++++++++++++++++++++++++++----------------------
 pack-bitmap.h          |  10 +++-
 7 files changed, 171 insertions(+), 137 deletions(-)

Range-diff versus v1:

1:  7a1a92acbe ! 1:  b675967b78 odb/source-packed: improve lookup when enumerating objects
    @@ Metadata
      ## Commit message ##
         odb/source-packed: improve lookup when enumerating objects
     
    -    When iterating through packed objects via `odb_for_each_object()` we
    -    do so via two different mechanisms:
    +    When iterating through objects of a packed source that have a specific
    +    prefix we do so via two different methods:
     
           - When a multi-pack index is available we use that one to efficiently
             loop through all objects.
    @@ Commit message
     
           - It's subtly wrong, as it may now happen that a specific object will
             be looked up via a different pack in case it exists multiple times.
    +        This is unlikely to have any real-world consequences, but it's still
    +        the wrong thing to do.
     
         Fix the issue by using `packed_object_info()` directly. While at it,
         rename the `store` variable to `source`.
2:  a6c8bd7a61 = 2:  d3f9b2f781 pack-bitmap: mark object filter as `const`
3:  c38b06636b = 3:  825920205a pack-bitmap: allow aborting iteration of bitmapped objects
4:  450cdd13b7 ! 4:  a33ca8fa3b pack-bitmap: iterate object sources when opening bitmaps
    @@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
     +				  struct bitmap_index *bitmap_git)
      {
     -	struct packed_git *p;
    +-	int ret = -1;
     +	struct multi_pack_index *midx = get_multi_pack_index(source);
     +	struct packfile_list_entry *e;
    - 	int ret = -1;
    ++	bool found = false;
      
     -	repo_for_each_pack(r, p) {
     -		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
    @@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
     -				break;
     -		}
     +	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
    -+		ret = 0;
    ++		found = true;
     +
     +	for (e = packfile_store_get_packs(source); e; e = e->next) {
     +		/*
     +		 * When tracing is enabled we want to keep looking to report
     +		 * duplicates even if we have already found a bitmap.
     +		 */
    -+		if (!ret && !trace2_is_enabled())
    ++		if (found && !trace2_is_enabled())
     +			break;
     +
    -+		if (open_pack_bitmap_1(bitmap_git, e->pack))
    -+			continue;
    -+		ret = 0;
    ++		if (!open_pack_bitmap_1(bitmap_git, e->pack))
    ++			found = true;
      	}
      
    - 	return ret;
    +-	return ret;
    ++	return found ? 0 : -1;
      }
      
     -static int open_midx_bitmap(struct repository *r,
    @@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
      {
      	struct odb_source *source;
     -	int ret = -1;
    -+	int found = 0;
    ++	bool found = false;
      
      	assert(!bitmap_git->map);
      
    @@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index
     -	int found;
      
     -	assert(!bitmap_git->map);
    -+		found |= !open_bitmap_for_source(files->packed, bitmap_git);
    ++		if (!open_bitmap_for_source(files->packed, bitmap_git))
    ++			found = true;
      
     -	found = !open_midx_bitmap(r, bitmap_git);
     -
-:  ---------- > 5:  b890ed7163 pack-bitmap: drop `_1` suffix from functions that open bitmaps
5:  26b1957f8b = 6:  f7e466217b pack-bitmap: introduce function to open bitmap for a single source
6:  722727c76d ! 7:  27ecc0802f odb: introduce object filters to `odb_for_each_object()`
    @@ Commit message
         object filter infrastructure supports some filters that cannot be
         answered by the object database alone.
     
    +    An alternative might be to limit the filters to only those that _can_ be
    +    answered by backends. But ultimately, the filters that can be answered
    +    efficiently by the "packed" backend are completely disjunct from those
    +    that can be answered by the "loose" backend, and consequently the set of
    +    filters supported by all backends would be empty. Furthermore, it would
    +    require us to make assumptions about capabilities of future backends,
    +    which may be able to efficiently handle more filters than current ones.
    +    So in the end, this alternative would only limit us artificially.
    +
         Implement the logic for the "packed" source. Note that we use the new
         function `prepare_source_bitmap_git()` to open the bitmap: as the
         backend operates on a single object source, we must only use bitmaps
7:  90be28e904 ! 8:  e4c6aeab0a builtin/cat-file: filter objects via object database
    @@ Metadata
      ## Commit message ##
         builtin/cat-file: filter objects via object database
     
    +    When batching all objects, git-cat-file(1) reaches into the internals of
    +    the object database and manually manages bitmaps to apply object
    +    filters. This creates coupling between the command and the internals of
    +    the respective backend.
    +
         Refactor git-cat-file(1) to use the new object filter option when
         batching all objects. This significantly simplifies the logic and
         ensures that we don't have to reach into internals of the "files" source

---
base-commit: 3c8e2790f2ce15e8b5d4b4e6ced711b12649f32a
change-id: 20260708-pks-odb-for-each-object-filter-13286fa3523d

[PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects

[Patrick Steinhardt]

When iterating through objects of a packed source that have a specific
prefix we do so via two different methods:

  - When a multi-pack index is available we use that one to efficiently
    loop through all objects.

  - We then loop through all packfiles that aren't covered by a
    multi-pack index.

Regardless of which mechanism we use, we then iterate through all the
objects indexed by the respective data structure. Curiously though,
while we use the indices for enumerating the objects, we completely
ignore it for the actual object lookup. Instead, we call into the
generic `odb_source_read_object_info()` function, which will itself
consult the indices to figure out where the object in question even
lives.

This has two consequences:

  - It's inefficient, as we basically have to figure out the position of
    the object a second time.

  - It's subtly wrong, as it may now happen that a specific object will
    be looked up via a different pack in case it exists multiple times.
    This is unlikely to have any real-world consequences, but it's still
    the wrong thing to do.

Fix the issue by using `packed_object_info()` directly. While at it,
rename the `store` variable to `source`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-packed.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0edea5356d..9cfa02b7a2 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -143,7 +143,7 @@ static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_f
 }
 
 static int for_each_prefixed_object_in_midx(
-	struct odb_source_packed *store,
+	struct odb_source_packed *source,
 	struct multi_pack_index *m,
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -170,6 +170,7 @@ static int for_each_prefixed_object_in_midx(
 		 */
 		for (i = first; i < num; i++) {
 			const struct object_id *current = NULL;
+			struct packed_git *pack;
 			struct object_id oid;
 
 			current = nth_midxed_object_oid(&oid, m, i);
@@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
 			if (!match_hash(len, opts->prefix->hash, current->hash))
 				break;
 
-			if (opts->flags) {
+			if (opts->flags || data->request) {
 				uint32_t pack_id = nth_midxed_pack_int_id(m, i);
-				struct packed_git *pack;
 
 				if (prepare_midx_pack(m, pack_id)) {
 					pack_errors = true;
@@ -193,9 +193,9 @@ static int for_each_prefixed_object_in_midx(
 
 			if (data->request) {
 				struct object_info oi = *data->request;
+				off_t offset = nth_midxed_offset(m, i);
 
-				ret = odb_source_read_object_info(&store->base, current,
-								  &oi, 0);
+				ret = packed_object_info(source, pack, offset, &oi);
 				if (ret)
 					goto out;
 
@@ -219,7 +219,7 @@ static int for_each_prefixed_object_in_midx(
 }
 
 static int for_each_prefixed_object_in_pack(
-	struct odb_source_packed *store,
+	struct odb_source_packed *source,
 	struct packed_git *p,
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -246,8 +246,9 @@ static int for_each_prefixed_object_in_pack(
 
 		if (data->request) {
 			struct object_info oi = *data->request;
+			off_t offset = nth_packed_object_offset(p, i);
 
-			ret = odb_source_read_object_info(&store->base, &oid, &oi, 0);
+			ret = packed_object_info(source, p, offset, &oi);
 			if (ret)
 				goto out;
 

-- 
2.55.0.229.g6434b31f56.dirty

[PATCH v2 2/8] pack-bitmap: mark object filter as `const`

[Patrick Steinhardt]

The function `for_each_bitmapped_object()` accepts an optional object
filter. This filter is never modified by the function, but is not
declared as `const`. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 6 +++---
 pack-bitmap.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 35774b6f0c..a47c231632 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1976,7 +1976,7 @@ static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
 static int filter_bitmap(struct bitmap_index *bitmap_git,
 			 struct object_list *tip_objects,
 			 struct bitmap *to_filter,
-			 struct list_objects_filter_options *filter)
+			 const struct list_objects_filter_options *filter)
 {
 	if (!filter || filter->choice == LOFC_DISABLED)
 		return 0;
@@ -2027,7 +2027,7 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int can_filter_bitmap(struct list_objects_filter_options *filter)
+static int can_filter_bitmap(const struct list_objects_filter_options *filter)
 {
 	return !filter_bitmap(NULL, NULL, NULL, filter);
 }
@@ -2058,7 +2058,7 @@ static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
 }
 
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
-			      struct list_objects_filter_options *filter,
+			      const struct list_objects_filter_options *filter,
 			      show_reachable_fn show_reach,
 			      void *payload)
 {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 19a8655457..47935eb24e 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -96,7 +96,7 @@ struct list_objects_filter_options;
  * not supported, `0` otherwise.
  */
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
-			      struct list_objects_filter_options *filter,
+			      const struct list_objects_filter_options *filter,
 			      show_reachable_fn show_reach,
 			      void *payload);
 

-- 
2.55.0.229.g6434b31f56.dirty

[PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects

[Patrick Steinhardt]

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.

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()`.

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 <ps@pks.im>
---
 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;
 }
 
 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;
 }
 
 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;
 		}
 	}
 
+	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;
+	}
 
 	ret = 0;
 out:
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 47935eb24e..ae8dc491ac 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -93,7 +93,8 @@ struct list_objects_filter_options;
 /*
  * Filter bitmapped objects and iterate through all resulting objects,
  * executing `show_reach` for each of them. Returns `-1` in case the filter is
- * not supported, `0` otherwise.
+ * not supported, `0` otherwise. Aborts iteration and bubbles up the return
+ * value in case `show_reach()` returns non-zero.
  */
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
 			      const struct list_objects_filter_options *filter,

-- 
2.55.0.229.g6434b31f56.dirty

[PATCH v2 4/8] pack-bitmap: iterate object sources when opening bitmaps

[Patrick Steinhardt]

When opening a bitmap for a repository we perform two steps:

  - We first look for a multi-pack index bitmap in any of the object
    sources connected to the repository.

  - We then look for a packfile bitmap in any of the packfiles of any of
    the object sources.

Both of these steps thus iterate through object sources themselves, one
via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
layout makes it hard to introduce a way to open the bitmap of one
specific object source, which is functionality that we'll require in a
subsequent commit.

Reverse the loop so that we instead loop through all sources in the
outer loop, and then for each source we try to load its bitmap via
either the multi-pack index or via a packfile.

Note that this changes the precedence of bitmaps in one specific edge
case: when an earlier object source only has a packfile bitmap, but a
later source has a multi-pack index bitmap, we now pick the packfile
bitmap of the earlier source. Previously, a multi-pack index bitmap from
any source would have taken precedence over all packfile bitmaps. Given
that object sources are ordered such that the local source comes first,
this arguably is an improvement, as we now prefer local bitmaps over
bitmaps in alternates. Furthermore, we already warn about repositories
that have multiple bitmaps, so this setup is broken and thus arguably
not worth worrying about too much.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 69 +++++++++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 38 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index eda38a5433..e32795a595 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
 	return 0;
 }
 
-static int open_pack_bitmap(struct repository *r,
-			    struct bitmap_index *bitmap_git)
+static int open_bitmap_for_source(struct odb_source_packed *source,
+				  struct bitmap_index *bitmap_git)
 {
-	struct packed_git *p;
-	int ret = -1;
+	struct multi_pack_index *midx = get_multi_pack_index(source);
+	struct packfile_list_entry *e;
+	bool found = false;
 
-	repo_for_each_pack(r, p) {
-		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
-			ret = 0;
-			/*
-			 * The only reason to keep looking is to report
-			 * duplicates.
-			 */
-			if (!trace2_is_enabled())
-				break;
-		}
+	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+		found = true;
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		/*
+		 * When tracing is enabled we want to keep looking to report
+		 * duplicates even if we have already found a bitmap.
+		 */
+		if (found && !trace2_is_enabled())
+			break;
+
+		if (!open_pack_bitmap_1(bitmap_git, e->pack))
+			found = true;
 	}
 
-	return ret;
+	return found ? 0 : -1;
 }
 
-static int open_midx_bitmap(struct repository *r,
-			    struct bitmap_index *bitmap_git)
+static int open_bitmap(struct repository *r,
+		       struct bitmap_index *bitmap_git)
 {
 	struct odb_source *source;
-	int ret = -1;
+	bool found = false;
 
 	assert(!bitmap_git->map);
 
 	odb_prepare_alternates(r->objects);
 	for (source = r->objects->sources; source; source = source->next) {
 		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct multi_pack_index *midx = get_multi_pack_index(files->packed);
-		if (midx && !open_midx_bitmap_1(bitmap_git, midx))
-			ret = 0;
-	}
-	return ret;
-}
-
-static int open_bitmap(struct repository *r,
-		       struct bitmap_index *bitmap_git)
-{
-	int found;
 
-	assert(!bitmap_git->map);
+		if (!open_bitmap_for_source(files->packed, bitmap_git))
+			found = true;
 
-	found = !open_midx_bitmap(r, bitmap_git);
-
-	/*
-	 * these will all be skipped if we opened a midx bitmap; but run it
-	 * anyway if tracing is enabled to report the duplicates
-	 */
-	if (!found || trace2_is_enabled())
-		found |= !open_pack_bitmap(r, bitmap_git);
+		/*
+		 * The only reason to keep looking after having found a bitmap
+		 * is to report duplicates.
+		 */
+		if (found && !trace2_is_enabled())
+			break;
+	}
 
 	return found ? 0 : -1;
 }

-- 
2.55.0.229.g6434b31f56.dirty

[PATCH v2 5/8] pack-bitmap: drop `_1` suffix from functions that open bitmaps

[Patrick Steinhardt]

In the preceding commit we've refactored how we open bitmaps. As part of
the refactoring we have consolidated `open_pack_bitmap()` as well as
`open_midx_bitmap()` into `open_bitmap_for_source()`. Consequently, we
only have their `open_pack_bitmap_1()` and `open_midx_bitmap_1()`
variants left over, where the `_1` suffix doesn't really make much sense
anymore.

Drop the suffix.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index e32795a595..72c8ae3228 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -460,8 +460,8 @@ char *pack_bitmap_filename(struct packed_git *p)
 	return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
 }
 
-static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
-			      struct multi_pack_index *midx)
+static int open_midx_bitmap(struct bitmap_index *bitmap_git,
+			    struct multi_pack_index *midx)
 {
 	struct stat st;
 	char *bitmap_name = midx_bitmap_filename(midx);
@@ -539,7 +539,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
+static int open_pack_bitmap(struct bitmap_index *bitmap_git, struct packed_git *packfile)
 {
 	int fd;
 	struct stat st;
@@ -603,7 +603,7 @@ static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_
 
 		/*
 		 * The multi-pack-index's .rev file is already loaded via
-		 * open_pack_bitmap_1().
+		 * open_pack_bitmap().
 		 *
 		 * But we still need to open the individual pack .rev files,
 		 * since we will need to make use of them in pack-objects.
@@ -687,7 +687,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
 	struct packfile_list_entry *e;
 	bool found = false;
 
-	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+	if (midx && !open_midx_bitmap(bitmap_git, midx))
 		found = true;
 
 	for (e = packfile_store_get_packs(source); e; e = e->next) {
@@ -698,7 +698,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
 		if (found && !trace2_is_enabled())
 			break;
 
-		if (!open_pack_bitmap_1(bitmap_git, e->pack))
+		if (!open_pack_bitmap(bitmap_git, e->pack))
 			found = true;
 	}
 
@@ -746,7 +746,7 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
 {
 	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
 
-	if (!open_midx_bitmap_1(bitmap_git, midx))
+	if (!open_midx_bitmap(bitmap_git, midx))
 		return bitmap_git;
 
 	free_bitmap_index(bitmap_git);

-- 
2.55.0.229.g6434b31f56.dirty

[PATCH v2 6/8] pack-bitmap: introduce function to open bitmap for a single source

[Patrick Steinhardt]

The function `prepare_bitmap_git()` opens the first bitmap it can find
in any of the object sources connected to the repository. In a
subsequent commit, the "packed" object database backend will learn to
use bitmaps to answer object filters when enumerating objects. That
backend operates on a single object source though, so using a bitmap
that potentially belongs to a different source would be wrong:

  - The source would yield objects that are not part of the source
    itself.

  - The object source info would be attributed to the wrong source.

  - With multiple sources, each source would enumerate the same bitmap
    another time.

Introduce a new function `prepare_source_bitmap_git()` that only opens
bitmaps belonging to the given object source.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 12 ++++++++++++
 pack-bitmap.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 72c8ae3228..09ba15d26b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -753,6 +753,18 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
 	return NULL;
 }
 
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source)
+{
+	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+
+	if (!open_bitmap_for_source(source, bitmap_git) &&
+	    !load_bitmap(source->base.odb->repo, bitmap_git, 0))
+		return bitmap_git;
+
+	free_bitmap_index(bitmap_git);
+	return NULL;
+}
+
 int bitmap_index_contains_pack(struct bitmap_index *bitmap, struct packed_git *pack)
 {
 	for (; bitmap; bitmap = bitmap->base) {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index ae8dc491ac..9f20fb6e56 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -9,6 +9,7 @@
 #include "string-list.h"
 
 struct commit;
+struct odb_source_packed;
 struct repository;
 struct rev_info;
 
@@ -68,6 +69,7 @@ struct bitmapped_pack {
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r);
 struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx);
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source);
 
 /*
  * Given a bitmap index, determine whether it contains the pack either directly

-- 
2.55.0.229.g6434b31f56.dirty

[PATCH v2 7/8] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

The function `for_each_bitmapped_object()` can be used to iterate
through all objects covered by a bitmap. The benefit of this function is
that it allows the caller to efficiently handle some object filters. For
example, this can be used to filter out objects of a specific type with
some simple bitmap operations. But callers are currently required to
manually wire up the use of bitmaps though, and to do so they have to
reach into internals of a given object database source.

Introduce a new `struct odb_for_each_object_options::filter` field so
that the interface becomes generic. When set, then a backend may
optionally use the filter to skip some objects that it would have
otherwise yielded.

Note that the respective backends are free to ignore this field if they
cannot meaningfully optimize for a given filter, and consequently
callers need to verify whether they actually want the returned objects.
While annoying, we cannot easily lift this restriction anyway as the
object filter infrastructure supports some filters that cannot be
answered by the object database alone.

An alternative might be to limit the filters to only those that _can_ be
answered by backends. But ultimately, the filters that can be answered
efficiently by the "packed" backend are completely disjunct from those
that can be answered by the "loose" backend, and consequently the set of
filters supported by all backends would be empty. Furthermore, it would
require us to make assumptions about capabilities of future backends,
which may be able to efficiently handle more filters than current ones.
So in the end, this alternative would only limit us artificially.

Implement the logic for the "packed" source. Note that we use the new
function `prepare_source_bitmap_git()` to open the bitmap: as the
backend operates on a single object source, we must only use bitmaps
that belong to that specific source. Otherwise we might yield objects
that are not part of the source at all, and with multiple sources we
would enumerate the same bitmap once per source.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.h               | 12 +++++++++++
 odb/source-packed.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 pack-bitmap.c       |  3 +--
 pack-bitmap.h       |  3 +++
 4 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/odb.h b/odb.h
index a1e222f605..67d0b34942 100644
--- a/odb.h
+++ b/odb.h
@@ -8,6 +8,7 @@
 #include "thread-utils.h"
 
 struct cached_object_entry;
+struct list_objects_filter_options;
 struct odb_source_inmemory;
 struct packed_git;
 struct repository;
@@ -490,6 +491,17 @@ struct odb_for_each_object_options {
 	 */
 	const struct object_id *prefix;
 	size_t prefix_hex_len;
+
+	/*
+	 * Optional object filter that allows backends to skip yielding
+	 * objects that are excluded by the filter as an optimization. The
+	 * filter is a best-effort hint: backends may use it to skip
+	 * excluded objects (e.g. by consulting a reachability bitmap), but
+	 * are also free to ignore it entirely and yield every object. As a
+	 * consequence, callers must re-apply the filter on yielded objects
+	 * if they require strict filtering semantics.
+	 */
+	const struct list_objects_filter_options *filter;
 };
 
 /*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 9cfa02b7a2..4777395053 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -3,11 +3,13 @@
 #include "chdir-notify.h"
 #include "dir.h"
 #include "git-zlib.h"
+#include "list-objects-filter-options.h"
 #include "mergesort.h"
 #include "midx.h"
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
 #include "packfile.h"
+#include "pack-bitmap.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -315,6 +317,37 @@ static int odb_source_packed_for_each_prefixed_object(
 	return ret;
 }
 
+struct bitmapped_for_each_object_data {
+	struct odb_source_packed *packed;
+	const struct object_info *request;
+	const struct odb_for_each_object_options *opts;
+	odb_for_each_object_cb cb;
+	void *cb_data;
+};
+
+static int bitmapped_for_each_object(const struct object_id *oid,
+				     enum object_type type UNUSED,
+				     int flags UNUSED,
+				     uint32_t hash UNUSED,
+				     struct packed_git *pack,
+				     off_t offset,
+				     void *cb_data)
+{
+	struct bitmapped_for_each_object_data *data = cb_data;
+
+	if (should_exclude_pack(pack, data->opts->flags))
+		return 0;
+
+	if (data->request) {
+		struct object_info oi = *data->request;
+		if (packed_object_info(data->packed, pack, offset, &oi) < 0)
+			return -1;
+		return data->cb(oid, &oi, data->cb_data);
+	}
+
+	return data->cb(oid, NULL, data->cb_data);
+}
+
 static int odb_source_packed_for_each_object(struct odb_source *source,
 					     const struct object_info *request,
 					     odb_for_each_object_cb cb,
@@ -328,12 +361,33 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 		.cb = cb,
 		.cb_data = cb_data,
 	};
+	struct bitmap_index *bitmap = NULL;
 	struct packfile_list_entry *e;
 	int pack_errors = 0, ret;
 
 	if (opts->prefix)
 		return odb_source_packed_for_each_prefixed_object(packed, opts, &data);
 
+	if (opts->filter &&
+	    opts->filter->choice != LOFC_DISABLED &&
+	    can_filter_bitmap(opts->filter))
+		bitmap = prepare_bitmap_git_for_source(packed);
+	if (bitmap) {
+		struct bitmapped_for_each_object_data bitmap_data = {
+			.packed = packed,
+			.request = request,
+			.opts = opts,
+			.cb = cb,
+			.cb_data = cb_data,
+		};
+
+		ret = for_each_bitmapped_object(bitmap, opts->filter,
+						bitmapped_for_each_object,
+						&bitmap_data);
+		if (ret)
+			goto out;
+	}
+
 	packed->skip_mru_updates = true;
 
 	for (e = packfile_store_get_packs(packed); e; e = e->next) {
@@ -342,6 +396,13 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 		if (should_exclude_pack(p, opts->flags))
 			continue;
 
+		/*
+		 * Objects covered by the bitmap have already been yielded
+		 * above; skip them here to avoid duplicates.
+		 */
+		if (bitmap && bitmap_index_contains_pack(bitmap, p))
+			continue;
+
 		if (open_pack_index(p)) {
 			pack_errors = 1;
 			continue;
@@ -357,6 +418,7 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 
 out:
 	packed->skip_mru_updates = false;
+	free_bitmap_index(bitmap);
 
 	if (!ret && pack_errors)
 		ret = -1;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 09ba15d26b..f55a0859ea 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2039,12 +2039,11 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int can_filter_bitmap(const struct list_objects_filter_options *filter)
+bool can_filter_bitmap(const struct list_objects_filter_options *filter)
 {
 	return !filter_bitmap(NULL, NULL, NULL, filter);
 }
 
-
 static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
 					      struct bitmap *result)
 {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 9f20fb6e56..1385027c1f 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -92,6 +92,9 @@ int test_bitmap_pseudo_merge_objects(struct repository *r, uint32_t n);
 
 struct list_objects_filter_options;
 
+/* Check whether the filter can be computed via the bitmap. */
+bool can_filter_bitmap(const struct list_objects_filter_options *filter);
+
 /*
  * Filter bitmapped objects and iterate through all resulting objects,
  * executing `show_reach` for each of them. Returns `-1` in case the filter is

-- 
2.55.0.229.g6434b31f56.dirty

[PATCH v2 8/8] builtin/cat-file: filter objects via object database

[Patrick Steinhardt]

When batching all objects, git-cat-file(1) reaches into the internals of
the object database and manually manages bitmaps to apply object
filters. This creates coupling between the command and the internals of
the respective backend.

Refactor git-cat-file(1) to use the new object filter option when
batching all objects. This significantly simplifies the logic and
ensures that we don't have to reach into internals of the "files" source
anymore.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/cat-file.c | 76 +++++-------------------------------------------------
 1 file changed, 7 insertions(+), 69 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b4b99a73da..1458dd76d6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,7 +20,6 @@
 #include "userdiff.h"
 #include "oid-array.h"
 #include "packfile.h"
-#include "pack-bitmap.h"
 #include "object-file.h"
 #include "object-name.h"
 #include "odb.h"
@@ -844,28 +843,6 @@ static int batch_one_object_oi(const struct object_id *oid,
 	return payload->callback(oid, NULL, 0, payload->payload);
 }
 
-static int batch_one_object_packed(const struct object_id *oid,
-				   struct packed_git *pack,
-				   uint32_t pos,
-				   void *_payload)
-{
-	struct for_each_object_payload *payload = _payload;
-	return payload->callback(oid, pack, nth_packed_object_offset(pack, pos),
-				 payload->payload);
-}
-
-static int batch_one_object_bitmapped(const struct object_id *oid,
-				      enum object_type type UNUSED,
-				      int flags UNUSED,
-				      uint32_t hash UNUSED,
-				      struct packed_git *pack,
-				      off_t offset,
-				      void *_payload)
-{
-	struct for_each_object_payload *payload = _payload;
-	return payload->callback(oid, pack, offset, payload->payload);
-}
-
 static void batch_each_object(struct batch_options *opt,
 			      for_each_object_fn callback,
 			      unsigned flags,
@@ -875,56 +852,17 @@ static void batch_each_object(struct batch_options *opt,
 		.callback = callback,
 		.payload = _payload,
 	};
+	struct odb_source_info source_info;
+	struct object_info oi = {
+		.source_infop = &source_info,
+	};
 	struct odb_for_each_object_options opts = {
 		.flags = flags,
+		.filter = &opt->objects_filter,
 	};
-	struct bitmap_index *bitmap = NULL;
-	struct odb_source *source;
-
-	/*
-	 * TODO: we still need to tap into implementation details of the object
-	 * database sources. Ideally, we should extend `odb_for_each_object()`
-	 * to handle object filters itself so that we can move the filtering
-	 * logic into the individual sources.
-	 */
-	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);
-		int ret = odb_source_for_each_object(&files->loose->base, NULL, batch_one_object_oi,
-						     &payload, &opts);
-		if (ret)
-			break;
-	}
-
-	if (opt->objects_filter.choice != LOFC_DISABLED &&
-	    (bitmap = prepare_bitmap_git(the_repository)) &&
-	    !for_each_bitmapped_object(bitmap, &opt->objects_filter,
-				       batch_one_object_bitmapped, &payload)) {
-		struct packed_git *pack;
-
-		repo_for_each_pack(the_repository, pack) {
-			if (bitmap_index_contains_pack(bitmap, pack) ||
-			    open_pack_index(pack))
-				continue;
-			for_each_object_in_pack(pack, batch_one_object_packed,
-						&payload, flags);
-		}
-	} else {
-		struct odb_source_info source_info;
-		struct object_info oi = {
-			.source_infop = &source_info,
-		};
-
-		for (source = the_repository->objects->sources; source; source = source->next) {
-			struct odb_source_files *files = odb_source_files_downcast(source);
-			int ret = odb_source_for_each_object(&files->packed->base, &oi,
-							     batch_one_object_oi, &payload, &opts);
-			if (ret)
-				break;
-		}
-	}
 
-	free_bitmap_index(bitmap);
+	odb_for_each_object_ext(the_repository->objects, &oi,
+				batch_one_object_oi, &payload, &opts);
 }
 
 static int batch_objects(struct batch_options *opt)

-- 
2.55.0.229.g6434b31f56.dirty

Re: [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects

[Taylor Blau]

On Fri, Jul 10, 2026 at 10:48:53AM +0200, Patrick Steinhardt wrote:
> Fix the issue by using `packed_object_info()` directly.

What you wrote here makes sense to me insofar as I understand the
pluggable ODB code.

However, I am confused by the way this function is written in general.
We use `bsearch_one_midx()` to locate the first possible MIDX position
in which an object matching the given prefix may exist, which is
sensible. However, we go from that position up to "num", where "num" is
the total number of objects in the MIDX!

Functionally this is not incorrect as we will happily discard objects
that do not match the prefix. But it causes us to waste CPU cycles
repeatedly calling `match_hash()` (at least for the first byte of the
prefix) for objects that we know will match.

How often do we call this function with a prefix longer than a
single byte? I have no idea, but I would suspect that it makes up the
majority of calls. If we read the OID fanout chunk, we could narrow the
range that we enumerate through, and only compare the second byte
onwards of the given prefix, if one exists. In the single-byte prefix
case, this means that we shouldn't have to do any memory comparisons at
all.

> While at it, rename the `store` variable to `source`.

Unrelated, but please keep these to a minimum, as they make the patch
more difficult to read than is necessary.

Thanks,
Taylor

Re: [PATCH v2 2/8] pack-bitmap: mark object filter as `const`

[Taylor Blau]

On Fri, Jul 10, 2026 at 10:48:54AM +0200, Patrick Steinhardt wrote:
> The function `for_each_bitmapped_object()` accepts an optional object
> filter. This filter is never modified by the function, but is not
> declared as `const`. Fix this.

Makes sense. "Fix" this seems to imply that the existing behavior was
broken or otherwise incorrect, but I think this is fine.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  pack-bitmap.c | 6 +++---
>  pack-bitmap.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)

Thanks,
Taylor

Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects

[Taylor Blau]

On Fri, Jul 10, 2026 at 10:48:55AM +0200, 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.
>
> 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.

Makes sense.

> 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 was initially rather surprised to read this diff. I suspected that
this was a "we used to return non-zero to indicate success but now
return zero to match the project conventions", but was stumped by the
unchanged "return 0" in the context above.

But I suppose that is demonstrating the thing that you're trying to fix
here, which is that the caller doesn't actually care what is returned
from the callback, so the change here (and analogous ones below) make
sense to me.

> -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;

This has a broader scope than is strictly necessary, but I think that is
OK.

>  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;
> +	}

OK. So now we call this function in a loop instead of the unrolled
version, presumably because we want to propagate a failure from any one
of these before falling through to the remaining object types.

That makes sense, and I think the clean-up is well justified here.

However, the remaining `show_objects_for_type()` callers from within
`traverse_bitmap_commit_list()` do *not* bother to inspect the return
value, despite taking in an arbitrary 'show_reachable_fn', which itself
may return a non-zero value.

I guess this must be effectively OK in practice with respect to the
existing code for the same reason you indicate in the commit message
above, but we should change this function to *also* propagate non-zero
return values to eliminate the foot-gun completely.

Thanks,
Taylor

Re: [PATCH v2 4/8] pack-bitmap: iterate object sources when opening bitmaps

[Taylor Blau]

On Fri, Jul 10, 2026 at 10:48:56AM +0200, Patrick Steinhardt wrote:
> When opening a bitmap for a repository we perform two steps:
>
>   - We first look for a multi-pack index bitmap in any of the object
>     sources connected to the repository.
>
>   - We then look for a packfile bitmap in any of the packfiles of any of
>     the object sources.
>
> Both of these steps thus iterate through object sources themselves, one
> via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
> layout makes it hard to introduce a way to open the bitmap of one
> specific object source, which is functionality that we'll require in a
> subsequent commit.
>
> Reverse the loop so that we instead loop through all sources in the
> outer loop, and then for each source we try to load its bitmap via
> either the multi-pack index or via a packfile.

This makes sense. An individual object store should be considered to
have a bitmap in the abstract sense if it provides either a multi-pack
bitmap (or an incremental multi-pack bitmap ), or a single-pack
bitmap.

> Note that this changes the precedence of bitmaps in one specific edge
> case: when an earlier object source only has a packfile bitmap, but a
> later source has a multi-pack index bitmap, we now pick the packfile
> bitmap of the earlier source. Previously, a multi-pack index bitmap from
> any source would have taken precedence over all packfile bitmaps. Given
> that object sources are ordered such that the local source comes first,
> this arguably is an improvement, as we now prefer local bitmaps over
> bitmaps in alternates. Furthermore, we already warn about repositories
> that have multiple bitmaps, so this setup is broken and thus arguably
> not worth worrying about too much.

Yeah, I think the existing behavior should be considered broken, so I
think that this behavior change is a positive one.

Thanks,
Taylor

Re: [PATCH v2 5/8] pack-bitmap: drop `_1` suffix from functions that open bitmaps

[Taylor Blau]

On Fri, Jul 10, 2026 at 10:48:57AM +0200, Patrick Steinhardt wrote:
> In the preceding commit we've refactored how we open bitmaps. As part of
> the refactoring we have consolidated `open_pack_bitmap()` as well as
> `open_midx_bitmap()` into `open_bitmap_for_source()`. Consequently, we
> only have their `open_pack_bitmap_1()` and `open_midx_bitmap_1()`
> variants left over, where the `_1` suffix doesn't really make much sense
> anymore.
>
> Drop the suffix.

Makes sense. Thanks for keeping this in a separate commit.

Thanks,
Taylor

Re: [PATCH v2 7/8] odb: introduce object filters to `odb_for_each_object()`

[Taylor Blau]

On Fri, Jul 10, 2026 at 10:48:59AM +0200, Patrick Steinhardt wrote:
> ---
>  odb.h               | 12 +++++++++++
>  odb/source-packed.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  pack-bitmap.c       |  3 +--
>  pack-bitmap.h       |  3 +++
>  4 files changed, 78 insertions(+), 2 deletions(-)

This all looks about as expected to me. As mentioned earlier in this
thread, I am not as familiar with the pluggable ODB code as I'd like to
be, but the patch looks plausibly correct to me.

Thanks,
Taylor

Re: [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects

[Jeff King]

On Fri, Jul 10, 2026 at 09:08:44AM +0200, Patrick Steinhardt wrote:

> On Thu, Jul 09, 2026 at 03:19:52PM -0500, Justin Tobler wrote:
> > > 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?
> 
> The function is only called from a single location, and the return value
> was completely ignored until this commit. It has always been this way
> since the function was originally introduced in 6b8fda2db1
> (pack-objects: use bitmaps when packing objects, 2013-12-21), so it
> never seemed to have any purpose. The commit message doesn't mention
> anything either.

I think it was copying the semantics of its non-bitmap counterpart,
add_object_entry(). Of course nobody looks at that return value either!

Long ago there were callers that cared about whether we actually created
an entry, but I think the last one went away in 5379a5c5ee (Thin pack
generation: optimization., 2006-04-05), which was quite some time ago.

So I think we could probably drop the return value from
add_object_entry() entirely (but of course we can't do the same for the
bitmap variant, because of its use as a callback).

I mention this mostly as answering Justin's "I wonder why...", but it
might be worth cleaning up add_object_entry() here, as its return value
semantics have diverged from add_object_entry_from_bitmap().

-Peff

[PATCH v2 9/8?] pack-objects: drop unused return value from add_object_entry()

[Jeff King]

On Fri, Jul 10, 2026 at 10:48:52AM +0200, Patrick Steinhardt wrote:

> The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
> 2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
> info fields, 2026-07-02) merged into it.

Here's a patch doing the cleanup I proposed upthread.

-- >8 --
Subject: pack-objects: drop unused return value from add_object_entry()

This function returns 0/1 to its caller to tell them whether we actually
added a new entry (or if we considered it redundant). But nobody has
relied on that behavior since 5379a5c5ee (Thin pack generation:
optimization., 2006-04-05).

The extra return does not hurt much, but it recently became a bit more
confusing. We have a sister function, add_object_entry_from_bitmap(),
which had the same return value semantics. That function recently
changed to always return 0 (not void, because it must conform to a
callback function interface). So now we have two related functions which
both return an "int" but with different semantics.

Let's drop the unused "int" return from add_object_entry() entirely,
which makes it more clear that the two functions have diverged.

Signed-off-by: Jeff King <peff@peff.net>
---
I couldn't reference the commit by its id, since Junio has not yet
picked up the v2 sent a few hours ago. ;)

 builtin/pack-objects.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8ff92c5272..3673b14b89 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1867,16 +1867,16 @@ static const char no_closure_warning[] = N_(
 "disabling bitmap writing, as some objects are not being packed"
 );
 
-static int add_object_entry(const struct object_id *oid, enum object_type type,
-			    const char *name, int exclude)
+static void add_object_entry(const struct object_id *oid, enum object_type type,
+			     const char *name, int exclude)
 {
 	struct packed_git *found_pack = NULL;
 	off_t found_offset = 0;
 
 	display_progress(progress_state, ++nr_seen);
 
 	if (have_duplicate_entry(oid, exclude))
-		return 0;
+		return;
 
 	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
 		/* The pack is missing an object, so it will not have closure */
@@ -1885,13 +1885,12 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 				warning(_(no_closure_warning));
 			write_bitmap_index = 0;
 		}
-		return 0;
+		return;
 	}
 
 	create_object_entry(oid, type, pack_name_hash_fn(name),
 			    exclude, name && no_try_delta(name),
 			    found_pack, found_offset);
-	return 1;
 }
 
 static int add_object_entry_from_bitmap(const struct object_id *oid,
-- 
2.55.0.580.gbbcb530e9e

Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects

[Jeff King]

On Fri, Jul 10, 2026 at 03:34:53PM -0700, Taylor Blau wrote:

> However, the remaining `show_objects_for_type()` callers from within
> `traverse_bitmap_commit_list()` do *not* bother to inspect the return
> value, despite taking in an arbitrary 'show_reachable_fn', which itself
> may return a non-zero value.
> 
> I guess this must be effectively OK in practice with respect to the
> existing code for the same reason you indicate in the commit message
> above, but we should change this function to *also* propagate non-zero
> return values to eliminate the foot-gun completely.

The matching non-bitmap traverse_commit_list() does not allow aborting
based on callback returns, either. In fact, its callbacks return void!

Whichever direction we go, those two should probably stay in sync (so
either both should allow aborting early with a non-zero return, or both
should return void).

-Peff

Re: [PATCH v2 9/8?] pack-objects: drop unused return value from add_object_entry()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 10, 2026 at 10:48:52AM +0200, Patrick Steinhardt wrote:
>
>> The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
>> 2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
>> info fields, 2026-07-02) merged into it.
>
> Here's a patch doing the cleanup I proposed upthread.
>
> -- >8 --
> Subject: pack-objects: drop unused return value from add_object_entry()
>
> This function returns 0/1 to its caller to tell them whether we actually
> added a new entry (or if we considered it redundant). But nobody has
> relied on that behavior since 5379a5c5ee (Thin pack generation:
> optimization., 2006-04-05).
>
> The extra return does not hurt much, but it recently became a bit more
> confusing. We have a sister function, add_object_entry_from_bitmap(),
> which had the same return value semantics. That function recently
> changed to always return 0 (not void, because it must conform to a
> callback function interface). So now we have two related functions which
> both return an "int" but with different semantics.
>
> Let's drop the unused "int" return from add_object_entry() entirely,
> which makes it more clear that the two functions have diverged.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I couldn't reference the commit by its id, since Junio has not yet
> picked up the v2 sent a few hours ago. ;)

Heh, if you do intend to make this a part of the series as 9/8, you
can just say "earlier in the series" with its title, and that should
be sufficient to identify which patch, as I never make a
fast-forward merge when merging topics into integration branches
(which means that Michael's "git when-merged" works well).

And if we ever see v3 of this series, you and Patrick can work
together to see if it makes sense to squash it in, or move it
earlier in a series to as preliminary clean-up, etc.

Thanks.  I agree with the reasoning upthread that led to this
change.

Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects

[Patrick Steinhardt]

On Sat, Jul 11, 2026 at 04:01:14AM -0400, Jeff King wrote:
> On Fri, Jul 10, 2026 at 03:34:53PM -0700, Taylor Blau wrote:
> 
> > However, the remaining `show_objects_for_type()` callers from within
> > `traverse_bitmap_commit_list()` do *not* bother to inspect the return
> > value, despite taking in an arbitrary 'show_reachable_fn', which itself
> > may return a non-zero value.
> > 
> > I guess this must be effectively OK in practice with respect to the
> > existing code for the same reason you indicate in the commit message
> > above, but we should change this function to *also* propagate non-zero
> > return values to eliminate the foot-gun completely.
> 
> The matching non-bitmap traverse_commit_list() does not allow aborting
> based on callback returns, either. In fact, its callbacks return void!
> 
> Whichever direction we go, those two should probably stay in sync (so
> either both should allow aborting early with a non-zero return, or both
> should return void).

That's fair. But adapting `traverse_commit_list()` requires tons of
changes all over the tree, so I'm inclined to rather leave both
`traverse_bitmap_commit_list()` and `traverse_commit_list()` as-is.
Does that work for both of you?

Patrick

Re: [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects

[Patrick Steinhardt]

On Fri, Jul 10, 2026 at 03:25:10PM -0700, Taylor Blau wrote:
> On Fri, Jul 10, 2026 at 10:48:53AM +0200, Patrick Steinhardt wrote:
> > Fix the issue by using `packed_object_info()` directly.
> 
> What you wrote here makes sense to me insofar as I understand the
> pluggable ODB code.
> 
> However, I am confused by the way this function is written in general.
> We use `bsearch_one_midx()` to locate the first possible MIDX position
> in which an object matching the given prefix may exist, which is
> sensible. However, we go from that position up to "num", where "num" is
> the total number of objects in the MIDX!
> 
> Functionally this is not incorrect as we will happily discard objects
> that do not match the prefix. But it causes us to waste CPU cycles
> repeatedly calling `match_hash()` (at least for the first byte of the
> prefix) for objects that we know will match.

That's not quite true though, as we abort iteration as soon as
`match_hash()` tells us that the prefix doesn't match anymore.

Or do you mean that `num` should only be `m->num_objects` instead of
also iterating through `num_objects_in_base`? I have to admit that I'm
alwas struggling with the chained MIDX. It's never quite clear to me
whether a given function cares about the complete chain or whether it
really only cares about a single MIDX.

In any case, this code ultimately derives from 3f5f1cff92 (midx:
introduce `bsearch_one_midx()`, 2024-08-06). If one squints a bit you
can see that it's still roughly in the same shape.

> How often do we call this function with a prefix longer than a
> single byte? I have no idea, but I would suspect that it makes up the
> majority of calls. If we read the OID fanout chunk, we could narrow the
> range that we enumerate through, and only compare the second byte
> onwards of the given prefix, if one exists. In the single-byte prefix
> case, this means that we shouldn't have to do any memory comparisons at
> all.

The function is currently used to find unique prefixes and to
disambiguate object names. So whenever we either want to abbreviate a
object ID or in case we cant to figure out whether a given object ID
prefix is unique we'll end up calling it.

If this logic is currently wrong (or at least wasteful) though I'd
propose to fix this in a separate series, as it's been this way for
quite a while.

Thanks!

Patrick

[PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

Hi,

this patch series introduces object filters to `odb_for_each_object()`.
The intent of this is to make `git cat-file --batch-all-objects` work
with pluggable object databases. Right now it doesn't because it reaches
into internals of the "packed" backend to efficiently handle bitmapped
objects.

The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
info fields, 2026-07-02) merged into it.

Changes in v3:
  - Weave Peff's patch into the patch series.
  - Link to v2: https://patch.msgid.link/20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im

Changes in v2:
  - Add another patch to drop the `_1()` prefixes that aren't required
    anymore.
  - Change the approach in `open_bitmap_for_source()` to also use a
    `found` boolean instead of a confusing integer.
  - Add some more explanations to commit messages.
  - Link to v1: https://patch.msgid.link/20260709-pks-odb-for-each-object-filter-v1-0-82fe014b12b3@pks.im

Thanks!

Patrick

---
Jeff King (1):
      pack-objects: drop unused return value from add_object_entry()

Patrick Steinhardt (8):
      odb/source-packed: improve lookup when enumerating objects
      pack-bitmap: mark object filter as `const`
      pack-bitmap: allow aborting iteration of bitmapped objects
      pack-bitmap: iterate object sources when opening bitmaps
      pack-bitmap: drop `_1` suffix from functions that open bitmaps
      pack-bitmap: introduce function to open bitmap for a single source
      odb: introduce object filters to `odb_for_each_object()`
      builtin/cat-file: filter objects via object database

 builtin/cat-file.c     |  76 +++--------------------------
 builtin/pack-objects.c |  11 ++---
 builtin/rev-list.c     |   2 +-
 odb.h                  |  12 +++++
 odb/source-packed.c    |  77 ++++++++++++++++++++++++++---
 pack-bitmap.c          | 129 +++++++++++++++++++++++++++----------------------
 pack-bitmap.h          |  10 +++-
 7 files changed, 175 insertions(+), 142 deletions(-)

Range-diff versus v2:

 1:  baf2adb012 =  1:  7c0dc1be0d odb/source-packed: improve lookup when enumerating objects
 2:  57eecf3031 =  2:  2e5908c9c3 pack-bitmap: mark object filter as `const`
 -:  ---------- >  3:  f4d66ccfc6 pack-objects: drop unused return value from add_object_entry()
 3:  92dd6a6f6e =  4:  af475654b8 pack-bitmap: allow aborting iteration of bitmapped objects
 4:  92fe41577d =  5:  6ca42587c9 pack-bitmap: iterate object sources when opening bitmaps
 5:  e5d59959e3 =  6:  f62c3bbc81 pack-bitmap: drop `_1` suffix from functions that open bitmaps
 6:  ab3547ac2b =  7:  b2d25b6e9b pack-bitmap: introduce function to open bitmap for a single source
 7:  026f21f522 =  8:  a5bf309bec odb: introduce object filters to `odb_for_each_object()`
 8:  534b25c817 =  9:  600b15a907 builtin/cat-file: filter objects via object database

---
base-commit: 3c8e2790f2ce15e8b5d4b4e6ced711b12649f32a
change-id: 20260708-pks-odb-for-each-object-filter-13286fa3523d

[PATCH v3 1/9] odb/source-packed: improve lookup when enumerating objects

[Patrick Steinhardt]

When iterating through objects of a packed source that have a specific
prefix we do so via two different methods:

  - When a multi-pack index is available we use that one to efficiently
    loop through all objects.

  - We then loop through all packfiles that aren't covered by a
    multi-pack index.

Regardless of which mechanism we use, we then iterate through all the
objects indexed by the respective data structure. Curiously though,
while we use the indices for enumerating the objects, we completely
ignore it for the actual object lookup. Instead, we call into the
generic `odb_source_read_object_info()` function, which will itself
consult the indices to figure out where the object in question even
lives.

This has two consequences:

  - It's inefficient, as we basically have to figure out the position of
    the object a second time.

  - It's subtly wrong, as it may now happen that a specific object will
    be looked up via a different pack in case it exists multiple times.
    This is unlikely to have any real-world consequences, but it's still
    the wrong thing to do.

Fix the issue by using `packed_object_info()` directly. While at it,
rename the `store` variable to `source`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-packed.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0edea5356d..9cfa02b7a2 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -143,7 +143,7 @@ static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_f
 }
 
 static int for_each_prefixed_object_in_midx(
-	struct odb_source_packed *store,
+	struct odb_source_packed *source,
 	struct multi_pack_index *m,
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -170,6 +170,7 @@ static int for_each_prefixed_object_in_midx(
 		 */
 		for (i = first; i < num; i++) {
 			const struct object_id *current = NULL;
+			struct packed_git *pack;
 			struct object_id oid;
 
 			current = nth_midxed_object_oid(&oid, m, i);
@@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
 			if (!match_hash(len, opts->prefix->hash, current->hash))
 				break;
 
-			if (opts->flags) {
+			if (opts->flags || data->request) {
 				uint32_t pack_id = nth_midxed_pack_int_id(m, i);
-				struct packed_git *pack;
 
 				if (prepare_midx_pack(m, pack_id)) {
 					pack_errors = true;
@@ -193,9 +193,9 @@ static int for_each_prefixed_object_in_midx(
 
 			if (data->request) {
 				struct object_info oi = *data->request;
+				off_t offset = nth_midxed_offset(m, i);
 
-				ret = odb_source_read_object_info(&store->base, current,
-								  &oi, 0);
+				ret = packed_object_info(source, pack, offset, &oi);
 				if (ret)
 					goto out;
 
@@ -219,7 +219,7 @@ static int for_each_prefixed_object_in_midx(
 }
 
 static int for_each_prefixed_object_in_pack(
-	struct odb_source_packed *store,
+	struct odb_source_packed *source,
 	struct packed_git *p,
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
@@ -246,8 +246,9 @@ static int for_each_prefixed_object_in_pack(
 
 		if (data->request) {
 			struct object_info oi = *data->request;
+			off_t offset = nth_packed_object_offset(p, i);
 
-			ret = odb_source_read_object_info(&store->base, &oid, &oi, 0);
+			ret = packed_object_info(source, p, offset, &oi);
 			if (ret)
 				goto out;
 

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 2/9] pack-bitmap: mark object filter as `const`

[Patrick Steinhardt]

The function `for_each_bitmapped_object()` accepts an optional object
filter. This filter is never modified by the function, but is not
declared as `const`. Fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 6 +++---
 pack-bitmap.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 35774b6f0c..a47c231632 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1976,7 +1976,7 @@ static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
 static int filter_bitmap(struct bitmap_index *bitmap_git,
 			 struct object_list *tip_objects,
 			 struct bitmap *to_filter,
-			 struct list_objects_filter_options *filter)
+			 const struct list_objects_filter_options *filter)
 {
 	if (!filter || filter->choice == LOFC_DISABLED)
 		return 0;
@@ -2027,7 +2027,7 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int can_filter_bitmap(struct list_objects_filter_options *filter)
+static int can_filter_bitmap(const struct list_objects_filter_options *filter)
 {
 	return !filter_bitmap(NULL, NULL, NULL, filter);
 }
@@ -2058,7 +2058,7 @@ static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
 }
 
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
-			      struct list_objects_filter_options *filter,
+			      const struct list_objects_filter_options *filter,
 			      show_reachable_fn show_reach,
 			      void *payload)
 {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 19a8655457..47935eb24e 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -96,7 +96,7 @@ struct list_objects_filter_options;
  * not supported, `0` otherwise.
  */
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
-			      struct list_objects_filter_options *filter,
+			      const struct list_objects_filter_options *filter,
 			      show_reachable_fn show_reach,
 			      void *payload);
 

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 3/9] pack-objects: drop unused return value from add_object_entry()

[Patrick Steinhardt]

From: Jeff King <peff@peff.net>

This function returns 0/1 to its caller to tell them whether we actually
added a new entry (or if we considered it redundant). But nobody has
relied on that behavior since 5379a5c5ee (Thin pack generation:
optimization., 2006-04-05).

The extra return does not hurt much, but it is a bit confusing. We have
a sister function, add_object_entry_from_bitmap(), which has the same
return value semantics. That function is about to change to always return
0 (not void, because it must conform to a callback function interface).
So with that change, we'd have two related functions which both return
an "int" but with different semantics.

Let's drop the unused "int" return from add_object_entry() entirely,
which makes it more clear that the two functions have diverged.

Signed-off-by: Jeff King <peff@peff.net>
[ps: slightly massaged the commit message]
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-objects.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ea5eab4cf8..188c4f6d4b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1867,8 +1867,8 @@ static const char no_closure_warning[] = N_(
 "disabling bitmap writing, as some objects are not being packed"
 );
 
-static int add_object_entry(const struct object_id *oid, enum object_type type,
-			    const char *name, int exclude)
+static void add_object_entry(const struct object_id *oid, enum object_type type,
+			     const char *name, int exclude)
 {
 	struct packed_git *found_pack = NULL;
 	off_t found_offset = 0;
@@ -1876,7 +1876,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 	display_progress(progress_state, ++nr_seen);
 
 	if (have_duplicate_entry(oid, exclude))
-		return 0;
+		return;
 
 	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
 		/* The pack is missing an object, so it will not have closure */
@@ -1885,13 +1885,12 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 				warning(_(no_closure_warning));
 			write_bitmap_index = 0;
 		}
-		return 0;
+		return;
 	}
 
 	create_object_entry(oid, type, pack_name_hash_fn(name),
 			    exclude, name && no_try_delta(name),
 			    found_pack, found_offset);
-	return 1;
 }
 
 static int add_object_entry_from_bitmap(const struct object_id *oid,

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 4/9] pack-bitmap: allow aborting iteration of bitmapped objects

[Patrick Steinhardt]

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.

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()`.

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 <ps@pks.im>
---
 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 188c4f6d4b..3673b14b89 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1908,7 +1908,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;
 }
 
 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;
 }
 
 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;
 		}
 	}
 
+	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;
+	}
 
 	ret = 0;
 out:
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 47935eb24e..ae8dc491ac 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -93,7 +93,8 @@ struct list_objects_filter_options;
 /*
  * Filter bitmapped objects and iterate through all resulting objects,
  * executing `show_reach` for each of them. Returns `-1` in case the filter is
- * not supported, `0` otherwise.
+ * not supported, `0` otherwise. Aborts iteration and bubbles up the return
+ * value in case `show_reach()` returns non-zero.
  */
 int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
 			      const struct list_objects_filter_options *filter,

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 5/9] pack-bitmap: iterate object sources when opening bitmaps

[Patrick Steinhardt]

When opening a bitmap for a repository we perform two steps:

  - We first look for a multi-pack index bitmap in any of the object
    sources connected to the repository.

  - We then look for a packfile bitmap in any of the packfiles of any of
    the object sources.

Both of these steps thus iterate through object sources themselves, one
via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
layout makes it hard to introduce a way to open the bitmap of one
specific object source, which is functionality that we'll require in a
subsequent commit.

Reverse the loop so that we instead loop through all sources in the
outer loop, and then for each source we try to load its bitmap via
either the multi-pack index or via a packfile.

Note that this changes the precedence of bitmaps in one specific edge
case: when an earlier object source only has a packfile bitmap, but a
later source has a multi-pack index bitmap, we now pick the packfile
bitmap of the earlier source. Previously, a multi-pack index bitmap from
any source would have taken precedence over all packfile bitmaps. Given
that object sources are ordered such that the local source comes first,
this arguably is an improvement, as we now prefer local bitmaps over
bitmaps in alternates. Furthermore, we already warn about repositories
that have multiple bitmaps, so this setup is broken and thus arguably
not worth worrying about too much.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 69 +++++++++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 38 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index eda38a5433..e32795a595 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -680,60 +680,53 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
 	return 0;
 }
 
-static int open_pack_bitmap(struct repository *r,
-			    struct bitmap_index *bitmap_git)
+static int open_bitmap_for_source(struct odb_source_packed *source,
+				  struct bitmap_index *bitmap_git)
 {
-	struct packed_git *p;
-	int ret = -1;
+	struct multi_pack_index *midx = get_multi_pack_index(source);
+	struct packfile_list_entry *e;
+	bool found = false;
 
-	repo_for_each_pack(r, p) {
-		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
-			ret = 0;
-			/*
-			 * The only reason to keep looking is to report
-			 * duplicates.
-			 */
-			if (!trace2_is_enabled())
-				break;
-		}
+	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+		found = true;
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		/*
+		 * When tracing is enabled we want to keep looking to report
+		 * duplicates even if we have already found a bitmap.
+		 */
+		if (found && !trace2_is_enabled())
+			break;
+
+		if (!open_pack_bitmap_1(bitmap_git, e->pack))
+			found = true;
 	}
 
-	return ret;
+	return found ? 0 : -1;
 }
 
-static int open_midx_bitmap(struct repository *r,
-			    struct bitmap_index *bitmap_git)
+static int open_bitmap(struct repository *r,
+		       struct bitmap_index *bitmap_git)
 {
 	struct odb_source *source;
-	int ret = -1;
+	bool found = false;
 
 	assert(!bitmap_git->map);
 
 	odb_prepare_alternates(r->objects);
 	for (source = r->objects->sources; source; source = source->next) {
 		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct multi_pack_index *midx = get_multi_pack_index(files->packed);
-		if (midx && !open_midx_bitmap_1(bitmap_git, midx))
-			ret = 0;
-	}
-	return ret;
-}
-
-static int open_bitmap(struct repository *r,
-		       struct bitmap_index *bitmap_git)
-{
-	int found;
 
-	assert(!bitmap_git->map);
+		if (!open_bitmap_for_source(files->packed, bitmap_git))
+			found = true;
 
-	found = !open_midx_bitmap(r, bitmap_git);
-
-	/*
-	 * these will all be skipped if we opened a midx bitmap; but run it
-	 * anyway if tracing is enabled to report the duplicates
-	 */
-	if (!found || trace2_is_enabled())
-		found |= !open_pack_bitmap(r, bitmap_git);
+		/*
+		 * The only reason to keep looking after having found a bitmap
+		 * is to report duplicates.
+		 */
+		if (found && !trace2_is_enabled())
+			break;
+	}
 
 	return found ? 0 : -1;
 }

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 6/9] pack-bitmap: drop `_1` suffix from functions that open bitmaps

[Patrick Steinhardt]

In the preceding commit we've refactored how we open bitmaps. As part of
the refactoring we have consolidated `open_pack_bitmap()` as well as
`open_midx_bitmap()` into `open_bitmap_for_source()`. Consequently, we
only have their `open_pack_bitmap_1()` and `open_midx_bitmap_1()`
variants left over, where the `_1` suffix doesn't really make much sense
anymore.

Drop the suffix.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index e32795a595..72c8ae3228 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -460,8 +460,8 @@ char *pack_bitmap_filename(struct packed_git *p)
 	return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
 }
 
-static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
-			      struct multi_pack_index *midx)
+static int open_midx_bitmap(struct bitmap_index *bitmap_git,
+			    struct multi_pack_index *midx)
 {
 	struct stat st;
 	char *bitmap_name = midx_bitmap_filename(midx);
@@ -539,7 +539,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile)
+static int open_pack_bitmap(struct bitmap_index *bitmap_git, struct packed_git *packfile)
 {
 	int fd;
 	struct stat st;
@@ -603,7 +603,7 @@ static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_
 
 		/*
 		 * The multi-pack-index's .rev file is already loaded via
-		 * open_pack_bitmap_1().
+		 * open_pack_bitmap().
 		 *
 		 * But we still need to open the individual pack .rev files,
 		 * since we will need to make use of them in pack-objects.
@@ -687,7 +687,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
 	struct packfile_list_entry *e;
 	bool found = false;
 
-	if (midx && !open_midx_bitmap_1(bitmap_git, midx))
+	if (midx && !open_midx_bitmap(bitmap_git, midx))
 		found = true;
 
 	for (e = packfile_store_get_packs(source); e; e = e->next) {
@@ -698,7 +698,7 @@ static int open_bitmap_for_source(struct odb_source_packed *source,
 		if (found && !trace2_is_enabled())
 			break;
 
-		if (!open_pack_bitmap_1(bitmap_git, e->pack))
+		if (!open_pack_bitmap(bitmap_git, e->pack))
 			found = true;
 	}
 
@@ -746,7 +746,7 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
 {
 	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
 
-	if (!open_midx_bitmap_1(bitmap_git, midx))
+	if (!open_midx_bitmap(bitmap_git, midx))
 		return bitmap_git;
 
 	free_bitmap_index(bitmap_git);

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 7/9] pack-bitmap: introduce function to open bitmap for a single source

[Patrick Steinhardt]

The function `prepare_bitmap_git()` opens the first bitmap it can find
in any of the object sources connected to the repository. In a
subsequent commit, the "packed" object database backend will learn to
use bitmaps to answer object filters when enumerating objects. That
backend operates on a single object source though, so using a bitmap
that potentially belongs to a different source would be wrong:

  - The source would yield objects that are not part of the source
    itself.

  - The object source info would be attributed to the wrong source.

  - With multiple sources, each source would enumerate the same bitmap
    another time.

Introduce a new function `prepare_source_bitmap_git()` that only opens
bitmaps belonging to the given object source.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 pack-bitmap.c | 12 ++++++++++++
 pack-bitmap.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 72c8ae3228..09ba15d26b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -753,6 +753,18 @@ struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
 	return NULL;
 }
 
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source)
+{
+	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
+
+	if (!open_bitmap_for_source(source, bitmap_git) &&
+	    !load_bitmap(source->base.odb->repo, bitmap_git, 0))
+		return bitmap_git;
+
+	free_bitmap_index(bitmap_git);
+	return NULL;
+}
+
 int bitmap_index_contains_pack(struct bitmap_index *bitmap, struct packed_git *pack)
 {
 	for (; bitmap; bitmap = bitmap->base) {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index ae8dc491ac..9f20fb6e56 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -9,6 +9,7 @@
 #include "string-list.h"
 
 struct commit;
+struct odb_source_packed;
 struct repository;
 struct rev_info;
 
@@ -68,6 +69,7 @@ struct bitmapped_pack {
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r);
 struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx);
+struct bitmap_index *prepare_bitmap_git_for_source(struct odb_source_packed *source);
 
 /*
  * Given a bitmap index, determine whether it contains the pack either directly

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 8/9] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

The function `for_each_bitmapped_object()` can be used to iterate
through all objects covered by a bitmap. The benefit of this function is
that it allows the caller to efficiently handle some object filters. For
example, this can be used to filter out objects of a specific type with
some simple bitmap operations. But callers are currently required to
manually wire up the use of bitmaps though, and to do so they have to
reach into internals of a given object database source.

Introduce a new `struct odb_for_each_object_options::filter` field so
that the interface becomes generic. When set, then a backend may
optionally use the filter to skip some objects that it would have
otherwise yielded.

Note that the respective backends are free to ignore this field if they
cannot meaningfully optimize for a given filter, and consequently
callers need to verify whether they actually want the returned objects.
While annoying, we cannot easily lift this restriction anyway as the
object filter infrastructure supports some filters that cannot be
answered by the object database alone.

An alternative might be to limit the filters to only those that _can_ be
answered by backends. But ultimately, the filters that can be answered
efficiently by the "packed" backend are completely disjunct from those
that can be answered by the "loose" backend, and consequently the set of
filters supported by all backends would be empty. Furthermore, it would
require us to make assumptions about capabilities of future backends,
which may be able to efficiently handle more filters than current ones.
So in the end, this alternative would only limit us artificially.

Implement the logic for the "packed" source. Note that we use the new
function `prepare_source_bitmap_git()` to open the bitmap: as the
backend operates on a single object source, we must only use bitmaps
that belong to that specific source. Otherwise we might yield objects
that are not part of the source at all, and with multiple sources we
would enumerate the same bitmap once per source.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.h               | 12 +++++++++++
 odb/source-packed.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 pack-bitmap.c       |  3 +--
 pack-bitmap.h       |  3 +++
 4 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/odb.h b/odb.h
index a1e222f605..67d0b34942 100644
--- a/odb.h
+++ b/odb.h
@@ -8,6 +8,7 @@
 #include "thread-utils.h"
 
 struct cached_object_entry;
+struct list_objects_filter_options;
 struct odb_source_inmemory;
 struct packed_git;
 struct repository;
@@ -490,6 +491,17 @@ struct odb_for_each_object_options {
 	 */
 	const struct object_id *prefix;
 	size_t prefix_hex_len;
+
+	/*
+	 * Optional object filter that allows backends to skip yielding
+	 * objects that are excluded by the filter as an optimization. The
+	 * filter is a best-effort hint: backends may use it to skip
+	 * excluded objects (e.g. by consulting a reachability bitmap), but
+	 * are also free to ignore it entirely and yield every object. As a
+	 * consequence, callers must re-apply the filter on yielded objects
+	 * if they require strict filtering semantics.
+	 */
+	const struct list_objects_filter_options *filter;
 };
 
 /*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 9cfa02b7a2..4777395053 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -3,11 +3,13 @@
 #include "chdir-notify.h"
 #include "dir.h"
 #include "git-zlib.h"
+#include "list-objects-filter-options.h"
 #include "mergesort.h"
 #include "midx.h"
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
 #include "packfile.h"
+#include "pack-bitmap.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -315,6 +317,37 @@ static int odb_source_packed_for_each_prefixed_object(
 	return ret;
 }
 
+struct bitmapped_for_each_object_data {
+	struct odb_source_packed *packed;
+	const struct object_info *request;
+	const struct odb_for_each_object_options *opts;
+	odb_for_each_object_cb cb;
+	void *cb_data;
+};
+
+static int bitmapped_for_each_object(const struct object_id *oid,
+				     enum object_type type UNUSED,
+				     int flags UNUSED,
+				     uint32_t hash UNUSED,
+				     struct packed_git *pack,
+				     off_t offset,
+				     void *cb_data)
+{
+	struct bitmapped_for_each_object_data *data = cb_data;
+
+	if (should_exclude_pack(pack, data->opts->flags))
+		return 0;
+
+	if (data->request) {
+		struct object_info oi = *data->request;
+		if (packed_object_info(data->packed, pack, offset, &oi) < 0)
+			return -1;
+		return data->cb(oid, &oi, data->cb_data);
+	}
+
+	return data->cb(oid, NULL, data->cb_data);
+}
+
 static int odb_source_packed_for_each_object(struct odb_source *source,
 					     const struct object_info *request,
 					     odb_for_each_object_cb cb,
@@ -328,12 +361,33 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 		.cb = cb,
 		.cb_data = cb_data,
 	};
+	struct bitmap_index *bitmap = NULL;
 	struct packfile_list_entry *e;
 	int pack_errors = 0, ret;
 
 	if (opts->prefix)
 		return odb_source_packed_for_each_prefixed_object(packed, opts, &data);
 
+	if (opts->filter &&
+	    opts->filter->choice != LOFC_DISABLED &&
+	    can_filter_bitmap(opts->filter))
+		bitmap = prepare_bitmap_git_for_source(packed);
+	if (bitmap) {
+		struct bitmapped_for_each_object_data bitmap_data = {
+			.packed = packed,
+			.request = request,
+			.opts = opts,
+			.cb = cb,
+			.cb_data = cb_data,
+		};
+
+		ret = for_each_bitmapped_object(bitmap, opts->filter,
+						bitmapped_for_each_object,
+						&bitmap_data);
+		if (ret)
+			goto out;
+	}
+
 	packed->skip_mru_updates = true;
 
 	for (e = packfile_store_get_packs(packed); e; e = e->next) {
@@ -342,6 +396,13 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 		if (should_exclude_pack(p, opts->flags))
 			continue;
 
+		/*
+		 * Objects covered by the bitmap have already been yielded
+		 * above; skip them here to avoid duplicates.
+		 */
+		if (bitmap && bitmap_index_contains_pack(bitmap, p))
+			continue;
+
 		if (open_pack_index(p)) {
 			pack_errors = 1;
 			continue;
@@ -357,6 +418,7 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 
 out:
 	packed->skip_mru_updates = false;
+	free_bitmap_index(bitmap);
 
 	if (!ret && pack_errors)
 		ret = -1;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 09ba15d26b..f55a0859ea 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2039,12 +2039,11 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
 	return -1;
 }
 
-static int can_filter_bitmap(const struct list_objects_filter_options *filter)
+bool can_filter_bitmap(const struct list_objects_filter_options *filter)
 {
 	return !filter_bitmap(NULL, NULL, NULL, filter);
 }
 
-
 static void filter_packed_objects_from_bitmap(struct bitmap_index *bitmap_git,
 					      struct bitmap *result)
 {
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 9f20fb6e56..1385027c1f 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -92,6 +92,9 @@ int test_bitmap_pseudo_merge_objects(struct repository *r, uint32_t n);
 
 struct list_objects_filter_options;
 
+/* Check whether the filter can be computed via the bitmap. */
+bool can_filter_bitmap(const struct list_objects_filter_options *filter);
+
 /*
  * Filter bitmapped objects and iterate through all resulting objects,
  * executing `show_reach` for each of them. Returns `-1` in case the filter is

-- 
2.55.0.313.g8d093f411d.dirty

[PATCH v3 9/9] builtin/cat-file: filter objects via object database

[Patrick Steinhardt]

When batching all objects, git-cat-file(1) reaches into the internals of
the object database and manually manages bitmaps to apply object
filters. This creates coupling between the command and the internals of
the respective backend.

Refactor git-cat-file(1) to use the new object filter option when
batching all objects. This significantly simplifies the logic and
ensures that we don't have to reach into internals of the "files" source
anymore.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/cat-file.c | 76 +++++-------------------------------------------------
 1 file changed, 7 insertions(+), 69 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b4b99a73da..1458dd76d6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -20,7 +20,6 @@
 #include "userdiff.h"
 #include "oid-array.h"
 #include "packfile.h"
-#include "pack-bitmap.h"
 #include "object-file.h"
 #include "object-name.h"
 #include "odb.h"
@@ -844,28 +843,6 @@ static int batch_one_object_oi(const struct object_id *oid,
 	return payload->callback(oid, NULL, 0, payload->payload);
 }
 
-static int batch_one_object_packed(const struct object_id *oid,
-				   struct packed_git *pack,
-				   uint32_t pos,
-				   void *_payload)
-{
-	struct for_each_object_payload *payload = _payload;
-	return payload->callback(oid, pack, nth_packed_object_offset(pack, pos),
-				 payload->payload);
-}
-
-static int batch_one_object_bitmapped(const struct object_id *oid,
-				      enum object_type type UNUSED,
-				      int flags UNUSED,
-				      uint32_t hash UNUSED,
-				      struct packed_git *pack,
-				      off_t offset,
-				      void *_payload)
-{
-	struct for_each_object_payload *payload = _payload;
-	return payload->callback(oid, pack, offset, payload->payload);
-}
-
 static void batch_each_object(struct batch_options *opt,
 			      for_each_object_fn callback,
 			      unsigned flags,
@@ -875,56 +852,17 @@ static void batch_each_object(struct batch_options *opt,
 		.callback = callback,
 		.payload = _payload,
 	};
+	struct odb_source_info source_info;
+	struct object_info oi = {
+		.source_infop = &source_info,
+	};
 	struct odb_for_each_object_options opts = {
 		.flags = flags,
+		.filter = &opt->objects_filter,
 	};
-	struct bitmap_index *bitmap = NULL;
-	struct odb_source *source;
-
-	/*
-	 * TODO: we still need to tap into implementation details of the object
-	 * database sources. Ideally, we should extend `odb_for_each_object()`
-	 * to handle object filters itself so that we can move the filtering
-	 * logic into the individual sources.
-	 */
-	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);
-		int ret = odb_source_for_each_object(&files->loose->base, NULL, batch_one_object_oi,
-						     &payload, &opts);
-		if (ret)
-			break;
-	}
-
-	if (opt->objects_filter.choice != LOFC_DISABLED &&
-	    (bitmap = prepare_bitmap_git(the_repository)) &&
-	    !for_each_bitmapped_object(bitmap, &opt->objects_filter,
-				       batch_one_object_bitmapped, &payload)) {
-		struct packed_git *pack;
-
-		repo_for_each_pack(the_repository, pack) {
-			if (bitmap_index_contains_pack(bitmap, pack) ||
-			    open_pack_index(pack))
-				continue;
-			for_each_object_in_pack(pack, batch_one_object_packed,
-						&payload, flags);
-		}
-	} else {
-		struct odb_source_info source_info;
-		struct object_info oi = {
-			.source_infop = &source_info,
-		};
-
-		for (source = the_repository->objects->sources; source; source = source->next) {
-			struct odb_source_files *files = odb_source_files_downcast(source);
-			int ret = odb_source_for_each_object(&files->packed->base, &oi,
-							     batch_one_object_oi, &payload, &opts);
-			if (ret)
-				break;
-		}
-	}
 
-	free_bitmap_index(bitmap);
+	odb_for_each_object_ext(the_repository->objects, &oi,
+				batch_one_object_oi, &payload, &opts);
 }
 
 static int batch_objects(struct batch_options *opt)

-- 
2.55.0.313.g8d093f411d.dirty

Re: [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects

[Taylor Blau]

On Mon, Jul 13, 2026 at 11:54:43AM +0200, Patrick Steinhardt wrote:
> On Fri, Jul 10, 2026 at 03:25:10PM -0700, Taylor Blau wrote:
> > On Fri, Jul 10, 2026 at 10:48:53AM +0200, Patrick Steinhardt wrote:
> > > Fix the issue by using `packed_object_info()` directly.
> >
> > What you wrote here makes sense to me insofar as I understand the
> > pluggable ODB code.
> >
> > However, I am confused by the way this function is written in general.
> > We use `bsearch_one_midx()` to locate the first possible MIDX position
> > in which an object matching the given prefix may exist, which is
> > sensible. However, we go from that position up to "num", where "num" is
> > the total number of objects in the MIDX!
> >
> > Functionally this is not incorrect as we will happily discard objects
> > that do not match the prefix. But it causes us to waste CPU cycles
> > repeatedly calling `match_hash()` (at least for the first byte of the
> > prefix) for objects that we know will match.
>
> That's not quite true though, as we abort iteration as soon as
> `match_hash()` tells us that the prefix doesn't match anymore.

Right, we neither iterate through more objects than necessary once we
know that `match_hash()` will stop returning true, nor do we emit
objects that don't actually match the prefix.

What I was trying to say above is that in the special case where our
prefix is a single byte long, we don't have to call `match_hash()` at
*all*, since we can enumerate just the portion of the fanout for that
specific byte, and we know that all such entries will match.

> Or do you mean that `num` should only be `m->num_objects` instead of
> also iterating through `num_objects_in_base`? I have to admit that I'm
> alwas struggling with the chained MIDX. It's never quite clear to me
> whether a given function cares about the complete chain or whether it
> really only cares about a single MIDX.

If the goal is to yield all such objects that match the prefix, then we
need to enumerate each layer. The analogy that I have had in my head
while working on these is that they are the same conceptually as the
incremental commit-graph format.

Thanks,
Taylor

Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects

[Taylor Blau]

On Mon, Jul 13, 2026 at 11:53:50AM +0200, Patrick Steinhardt wrote:
> On Sat, Jul 11, 2026 at 04:01:14AM -0400, Jeff King wrote:
> > On Fri, Jul 10, 2026 at 03:34:53PM -0700, Taylor Blau wrote:
> >
> > > However, the remaining `show_objects_for_type()` callers from within
> > > `traverse_bitmap_commit_list()` do *not* bother to inspect the return
> > > value, despite taking in an arbitrary 'show_reachable_fn', which itself
> > > may return a non-zero value.
> > >
> > > I guess this must be effectively OK in practice with respect to the
> > > existing code for the same reason you indicate in the commit message
> > > above, but we should change this function to *also* propagate non-zero
> > > return values to eliminate the foot-gun completely.
> >
> > The matching non-bitmap traverse_commit_list() does not allow aborting
> > based on callback returns, either. In fact, its callbacks return void!
> >
> > Whichever direction we go, those two should probably stay in sync (so
> > either both should allow aborting early with a non-zero return, or both
> > should return void).
>
> That's fair. But adapting `traverse_commit_list()` requires tons of
> changes all over the tree, so I'm inclined to rather leave both
> `traverse_bitmap_commit_list()` and `traverse_commit_list()` as-is.
> Does that work for both of you?

I think that it's fine to leave it as-is for the purpose of this series,
though I would like to address it.

I don't think we need to adapt `traverse_commit_list()`, though. We can
go in the other direction Peff suggested, which would be to split the
callback type used by `for_each_bitmapped_object()` from
`show_reachable_fn`, keep the former abortable, and make the latter
return void.

That keeps `traverse_bitmap_commit_list()` in sync with
`traverse_commit_list()` without changing the non-bitmap traversal
machinery. I have a small two-patch follow-up on top of v3 that does
this, which I'll send separately.

Thanks,
Taylor

Re: [PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`

[Taylor Blau]

On Mon, Jul 13, 2026 at 04:41:24PM +0200, Patrick Steinhardt wrote:
> Range-diff versus v2:
>
>  1:  baf2adb012 =  1:  7c0dc1be0d odb/source-packed: improve lookup when enumerating objects
>  2:  57eecf3031 =  2:  2e5908c9c3 pack-bitmap: mark object filter as `const`
>  -:  ---------- >  3:  f4d66ccfc6 pack-objects: drop unused return value from add_object_entry()
>  3:  92dd6a6f6e =  4:  af475654b8 pack-bitmap: allow aborting iteration of bitmapped objects
>  4:  92fe41577d =  5:  6ca42587c9 pack-bitmap: iterate object sources when opening bitmaps
>  5:  e5d59959e3 =  6:  f62c3bbc81 pack-bitmap: drop `_1` suffix from functions that open bitmaps
>  6:  ab3547ac2b =  7:  b2d25b6e9b pack-bitmap: introduce function to open bitmap for a single source
>  7:  026f21f522 =  8:  a5bf309bec odb: introduce object filters to `odb_for_each_object()`
>  8:  534b25c817 =  9:  600b15a907 builtin/cat-file: filter objects via object database

Thanks, this version looks good to me.

Thanks,
Taylor

Re: [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects

[Patrick Steinhardt]

On Mon, Jul 13, 2026 at 08:49:43PM -0700, Taylor Blau wrote:
> On Mon, Jul 13, 2026 at 11:54:43AM +0200, Patrick Steinhardt wrote:
> > On Fri, Jul 10, 2026 at 03:25:10PM -0700, Taylor Blau wrote:
> > > On Fri, Jul 10, 2026 at 10:48:53AM +0200, Patrick Steinhardt wrote:
> > > > Fix the issue by using `packed_object_info()` directly.
> > >
> > > What you wrote here makes sense to me insofar as I understand the
> > > pluggable ODB code.
> > >
> > > However, I am confused by the way this function is written in general.
> > > We use `bsearch_one_midx()` to locate the first possible MIDX position
> > > in which an object matching the given prefix may exist, which is
> > > sensible. However, we go from that position up to "num", where "num" is
> > > the total number of objects in the MIDX!
> > >
> > > Functionally this is not incorrect as we will happily discard objects
> > > that do not match the prefix. But it causes us to waste CPU cycles
> > > repeatedly calling `match_hash()` (at least for the first byte of the
> > > prefix) for objects that we know will match.
> >
> > That's not quite true though, as we abort iteration as soon as
> > `match_hash()` tells us that the prefix doesn't match anymore.
> 
> Right, we neither iterate through more objects than necessary once we
> know that `match_hash()` will stop returning true, nor do we emit
> objects that don't actually match the prefix.
> 
> What I was trying to say above is that in the special case where our
> prefix is a single byte long, we don't have to call `match_hash()` at
> *all*, since we can enumerate just the portion of the fanout for that
> specific byte, and we know that all such entries will match.

Oh, now that's what you're getting at. I don't think that this case ever
happens at all right now. I think the shortest prefix that we're ever
using should be at least 2 bytes, as we don't treat anything shorter
than 4 hex characters as an abbreviated object ID.

Thanks for clarifying!

Patrick

Re: [PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`

[Patrick Steinhardt]

On Mon, Jul 13, 2026 at 08:59:39PM -0700, Taylor Blau wrote:
> On Mon, Jul 13, 2026 at 04:41:24PM +0200, Patrick Steinhardt wrote:
> > Range-diff versus v2:
> >
> >  1:  baf2adb012 =  1:  7c0dc1be0d odb/source-packed: improve lookup when enumerating objects
> >  2:  57eecf3031 =  2:  2e5908c9c3 pack-bitmap: mark object filter as `const`
> >  -:  ---------- >  3:  f4d66ccfc6 pack-objects: drop unused return value from add_object_entry()
> >  3:  92dd6a6f6e =  4:  af475654b8 pack-bitmap: allow aborting iteration of bitmapped objects
> >  4:  92fe41577d =  5:  6ca42587c9 pack-bitmap: iterate object sources when opening bitmaps
> >  5:  e5d59959e3 =  6:  f62c3bbc81 pack-bitmap: drop `_1` suffix from functions that open bitmaps
> >  6:  ab3547ac2b =  7:  b2d25b6e9b pack-bitmap: introduce function to open bitmap for a single source
> >  7:  026f21f522 =  8:  a5bf309bec odb: introduce object filters to `odb_for_each_object()`
> >  8:  534b25c817 =  9:  600b15a907 builtin/cat-file: filter objects via object database
> 
> Thanks, this version looks good to me.

Thanks for your review!

Patrick

Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects

[Jeff King]

On Mon, Jul 13, 2026 at 08:58:54PM -0700, Taylor Blau wrote:

> > That's fair. But adapting `traverse_commit_list()` requires tons of
> > changes all over the tree, so I'm inclined to rather leave both
> > `traverse_bitmap_commit_list()` and `traverse_commit_list()` as-is.
> > Does that work for both of you?
> 
> I think that it's fine to leave it as-is for the purpose of this series,
> though I would like to address it.

Me too.

> I don't think we need to adapt `traverse_commit_list()`, though. We can
> go in the other direction Peff suggested, which would be to split the
> callback type used by `for_each_bitmapped_object()` from
> `show_reachable_fn`, keep the former abortable, and make the latter
> return void.
> 
> That keeps `traverse_bitmap_commit_list()` in sync with
> `traverse_commit_list()` without changing the non-bitmap traversal
> machinery. I have a small two-patch follow-up on top of v3 that does
> this, which I'll send separately.

That would be a nice cleanup if it's possible, but I wondered if you
would find that one or more of the callbacks actually rely on this abort
feature. Only one way to find out. :)

-Peff

Re: [PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`

[Jeff King]

On Mon, Jul 13, 2026 at 04:41:24PM +0200, Patrick Steinhardt wrote:

> Changes in v3:
>   - Weave Peff's patch into the patch series.
>   - Link to v2: https://patch.msgid.link/20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im

Yay, thank you. :)

-Peff