From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 23 Jan 2026 01:21:55 GMT
Subject: Re: [PATCH v3 12/14] builtin/pack-objects: use `packfile_store_for_each_object()`
Message-ID: <aXLNM+AOpdQtmisC@nand.local>
In-Reply-To: <20260121-pks-odb-for-each-object-v3-12-12c4dfd24227@pks.im>

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

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.

> +		add_cruft_object_entry(oid, OBJ_NONE, oi->u.packed.pack,
> +				       oi->u.packed.offset, NULL, *oi->mtimep);
>  	} else {
>  		add_object_entry(oid, OBJ_NONE, "", 0);
>  	}
> @@ -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?

> +		if (packfile_store_for_each_object(source->packfiles, &oi,
> +						   add_object_in_unpacked_pack, NULL,
> +						   ODB_FOR_EACH_OBJECT_PACK_ORDER |
> +						   ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS |
> +						   ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS))
> +			die(_("cannot open pack index"));
> +	}
>  }
>
>  static int add_loose_object(const struct object_id *oid, const char *path,
>
> --
> 2.53.0.rc0.250.g0ac79233d6.dirty
>
Thanks,
Taylor