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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jan 23, 2026 at 10:43:16AM +0100, Patrick Steinhardt wrote:
> 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.

I understand what you're saying, but I don't think that this is unique
to stat. Looking through the object_info struct, there are a handful of
fields on the request side that are coupled to the objects themselves,
not their representation, such as typep, sizep, and contentp.

But there are a handful of fields in the request section which are *not*
properties of the objects themselves, but rather properties of the way
those objects are represented as part of the backend-specific
implementation.

For example, disk_sizep suggests that all objects are stored on disk and
have a clear notion of how much space they occupy. I could imagine a
backend implementation where perhaps the contents of objects are divvied
up into smaller chunks and deduplicated across many objects. I don't
think there is a clear answer to how much "disk size" an object occupies
in that case.

delta_base_oid is another field that I'd argue is not a property of the
object itself, but rather its representation. Of course, objects stored
in packfiles may or may not be stored as a delta against some other
object, and thus being able to ask what that object is makes sense. But
loose objects don't have the same property as a result of how they are
stored.

To me this seems like an example where implementation-specific details
are already leaking through the object_info struct. So in that sense I
don't think that adding a "struct stat" here is meaningfully changing
anything.

But I think the proposed mtimep field is a special case not only for the
reasons stated above, but because an object's mtime has multiple
interpretations already. For example, if I'm asking about an object's
mtime, and that object happens to be stored in a cruft pack, which mtime
am I referring to? Packed objects inherit their mtime from the mtime of
the *.pack itself, but cruft objects have an additional interpretation
which is read from the *.mtimes file corresponding to the cruft pack.

I don't love bolting another leaky abstraction onto the object_info
interface, but my broader concern is that the information here is not
just leaky but ambiguous. By adding a statp pointer, I think the
information is less ambiguous since the GC-specific interpretation of
mtime is done at a layer above stat(2).

> > 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?

Yes, the end result is the same, both your patch and what I wrote here
implement the same GC-specific definition of an object's "mtime". I am
not following the argument about pluggability, though. The concern I
have above is that we are pushing domain-specific logic into the object
storage backend, not the other way around.

Thanks,
Taylor