Re: [PATCH v3 12/14] builtin/pack-objects: use `packfile_store_for_each_object()`

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jan 22, 2026 at 08:21:55PM -0500, Taylor Blau wrote:
> On Wed, Jan 21, 2026 at 01:50:28PM +0100, Patrick Steinhardt wrote:
> >  static int add_object_in_unpacked_pack(const struct object_id *oid,
> > -				       struct packed_git *pack,
> > -				       uint32_t pos,
> > +				       struct object_info *oi,
> >  				       void *data UNUSED)
> >  {
> >  	if (cruft) {
> > -		off_t offset;
> > -		time_t mtime;
> > -
> > -		if (pack->is_cruft) {
> > -			if (load_pack_mtimes(pack) < 0)
> > -				die(_("could not load cruft pack .mtimes"));
> > -			mtime = nth_packed_mtime(pack, pos);
> > -		} else {
> > -			mtime = pack->mtime;
> > -		}
> > -		offset = nth_packed_object_offset(pack, pos);
> > -
> > -		add_cruft_object_entry(oid, OBJ_NONE, pack, offset,
> > -				       NULL, mtime);
> 
> OK, here's where we see the existing logic for determining the mtime of
> an object in the GC sense. I see there's a subsequent patch that also
> makes use of the object_info->mtimep field, and my guess is (not having
> completely read that patch yet) that having the same notion of mtime
> between the two callsites is desirable.
> 
> I still wonder whether imposing that notion of mtime at the object_info
> layer is the right choice. I wonder if it would make more sense to allow
> the caller to have a "statp" pointer filled out (or alternatively stick
> a "struct stat" in both the packed union type as well as the loose one,
> though the latter doesn't yet exist).

The problem with filling out a `struct stat` though is that it will only
apply to backends that actually have a path to stat. There may be other
backends that don't. You could of course pretend that there was a file
and fill in the `st_mtime` field. But I don't really see the benefit
over having a standalone mtime field.

> Then the caller could do something like:
> 
> static time_t object_info_gc_mtime(const struct object_info *oi)
> {
>     if (!oi->statp)
>         BUG("oops!");
> 
>     switch (oi->whence) {
>     case OI_CACHED:
>         return 0;
>     case OI_LOOSE:
>         return oi->statp->st_mtime;
>     case OI_PACKED:
>         struct packed_git *p = oi->u.packed.pack;
>         if (p->is_cruft) {
>             uint32_t pack_pos;
> 
>             if (load_pack_mtimes(p) < 0)
>                 die(_("could not load cruft pack .mtimes for '%s'"),
>                     pack_basename(p));
>             if (offset_to_pack_pos(p, oi->u.packed.offset, &pack_pos) < 0)
>                 die(_("could not find offset for object '%s' in cruft pack '%s'"),
>                     oid_to_hex(&oi->oid),
>                     pack_basename(p));
> 
>             return nth_packed_mtime(p, pack_pos_to_index(p, pack_pos));
>         } else {
>             return p->mtime; /* or oi->statp->st_mtime */
>         }
>     default:
>         BUG("unknown oi->whence: %d", oi->whence);
>     }
> }
> 
> I like the above because it encapsulates the GC-specific interpretation
> of an object's mtime outside of the object_info layer, while adding
> information (namely statp) that is generic enough to be potentially
> useful to other callers who may not be interested in the GC-specific
> interpretation.

This isn't achieving the goal of making the logic pluggable though, as
you now have backend-specific logic outside of the backends. Also, isn't
the end result basically the same as what I have proposed, except that
my version _is_ fully pluggable because the logic is entirely contained
in the backend?

> > @@ -4341,14 +4328,24 @@ static int add_object_in_unpacked_pack(const struct object_id *oid,
> >
> >  static void add_objects_in_unpacked_packs(void)
> >  {
> > -	if (for_each_packed_object(to_pack.repo,
> > -				   add_object_in_unpacked_pack,
> > -				   NULL,
> > -				   ODB_FOR_EACH_OBJECT_PACK_ORDER |
> > -				   ODB_FOR_EACH_OBJECT_LOCAL_ONLY |
> > -				   ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS |
> > -				   ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS))
> > -		die(_("cannot open pack index"));
> > +	struct odb_source *source;
> > +	time_t mtime;
> > +	struct object_info oi = {
> > +		.mtimep = &mtime,
> > +	};
> > +
> > +	odb_prepare_alternates(to_pack.repo->objects);
> > +	for (source = to_pack.repo->objects->sources; source; source = source->next) {
> > +		if (!source->local)
> > +			continue;
> 
> OK, we dropped the ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag when dispatching
> to the packfile_store iterator, but that's OK, since it's handled above
> here.
> 
> Interestingly, packfile_store_for_each_object_internal() has a similar
> check:
> 
>     if ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
>         continue;
> 
> , but I'm wondering whether these are subtly different. Would a
> non-local source ever have packs for which the p->pack_local bit is set?
> Or is the locality of a pack determined relative to the source
> containing it, in which case we'd need to make the check here?

To the best of my knowledge we may only ever end up adding a non-local
pack to the source, but not the other way round. This can for example
happen in git-index-pack(1).

But you know, there isn't any good reason to not continue passing this
flag. Better be safe than sorry.

Thanks!

Patrick