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

On Wed, Jan 21, 2026 at 01:50:28PM +0100, Patrick Steinhardt wrote:

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

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.

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?

Thanks, Taylor