Re: [PATCH v3 08/14] builtin/fsck: refactor to use `odb_for_each_object()`

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jan 21, 2026 at 01:50:24PM +0100, Patrick Steinhardt wrote:
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c | 57 ++++++++++++---------------------------------------------
>  1 file changed, 12 insertions(+), 45 deletions(-)

This patch was a really pleasant read. It's really great to see both
pairs of functions collapse into a single one that acts the same over
loose/packed objects as opposed to two functions which do the same thing
but need to have different signatures to be able to plug into the
iterators for loose vs. packed objects.

> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 4979bc795e..96107695ae 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -218,15 +218,17 @@ static int mark_used(struct object *obj, enum object_type type UNUSED,
>  	return 0;
>  }
>
> -static void mark_unreachable_referents(const struct object_id *oid)
> +static int mark_unreachable_referents(const struct object_id *oid,
> +				      struct object_info *io UNUSED,

s/io/oi/ ?

> +				      void *data UNUSED)
>  {

The transformation here makes sense (and I think aloud through the
similar mark_object_for_connectivity() transformation below). One
thought that I had while reading, though, was how this function behaves
when it is passed the same object more than once, since you mention that
as a possibility in the commit which introduces odb_for_each_object().

I think that this is OK, since we already likely send the same object to
this function multiple times if, e.g., we freshened an object from the
cruft pack, in which case we'd see it both when iterating packed
objects as well as when iterating loose ones.

As far as I can tell, that's OK, but it might be nice to provide a brief
analysis of that in the commit message, just to be sure and to help
future readers.

> @@ -848,26 +831,12 @@ static void fsck_index(struct index_state *istate, const char *index_path,
>  	fsck_resolve_undo(istate, index_path);
>  }
>
> -static void mark_object_for_connectivity(const struct object_id *oid)
> +static int mark_object_for_connectivity(const struct object_id *oid,
> +					struct object_info *oi UNUSED,
> +					void *cb_data UNUSED)
>  {
>  	struct object *obj = lookup_unknown_object(the_repository, oid);
>  	obj->flags |= HAS_OBJ;
> -}
> -
> -static int mark_loose_for_connectivity(const struct object_id *oid,
> -				       const char *path UNUSED,
> -				       void *data UNUSED)
> -{
> -	mark_object_for_connectivity(oid);
> -	return 0;
> -}
> -
> -static int mark_packed_for_connectivity(const struct object_id *oid,
> -					struct packed_git *pack UNUSED,
> -					uint32_t pos UNUSED,
> -					void *data UNUSED)
> -{
> -	mark_object_for_connectivity(oid);
>  	return 0;
>  }

This is really nice, and everything here makes sense. Both of the old
callback functions merely call mark_object_for_connectivity() but are
different in order to accommodate the different function signatures
required. The new function uses the common interface and does the exact
same thing. Looking great.

> @@ -1001,10 +970,8 @@ int cmd_fsck(int argc,
>  		fsck_refs(the_repository);
>
>  	if (connectivity_only) {
> -		for_each_loose_object(the_repository->objects,
> -				      mark_loose_for_connectivity, NULL, 0);
> -		for_each_packed_object(the_repository,
> -				       mark_packed_for_connectivity, NULL, 0);
> +		odb_for_each_object(the_repository->objects, NULL,
> +				    mark_object_for_connectivity, NULL, 0);

Makes sense.

Thanks,
Taylor