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

[Taylor Blau <ttaylorr@openai.com>]

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