Re: [PATCH v3 08/14] builtin/fsck: refactor to use `odb_for_each_object()`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jan 23, 2026, 00:32 UTC
- Message-ID
- <aXLBoFnqbnCcylCD@nand.local>
- In-Reply-To
- <20260121-pks-odb-for-each-object-v3-8-12c4dfd24227@pks.im>
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.
Show 11 quoted lines
> 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.
Show 29 quoted lines
> @@ -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.
Show 10 quoted lines
> @@ -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