From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 23 Jan 2026 09:42:50 GMT
Subject: Re: [PATCH v3 08/14] builtin/fsck: refactor to use `odb_for_each_object()`
Message-ID: <aXNCmnDysVmrMVJ-@pks.im>
In-Reply-To: <aXLBoFnqbnCcylCD@nand.local>

On Thu, Jan 22, 2026 at 07:32:32PM -0500, Taylor Blau wrote:
> On Wed, Jan 21, 2026 at 01:50:24PM +0100, Patrick Steinhardt wrote:
> > 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/ ?

Well spotted.

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

Fair point, will mention.

Patrick