From: Patrick Steinhardt Date: Fri, 23 Jan 2026 09:43:16 GMT Subject: Re: [PATCH v3 12/14] builtin/pack-objects: use `packfile_store_for_each_object()` Message-ID: In-Reply-To: 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