Re: [PATCH v3 12/14] builtin/pack-objects: use `packfile_store_for_each_object()`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jan 23, 2026, 18:35 UTC
- Message-ID
- <aXO/YLzRlDXD5IPY@nand.local>
- In-Reply-To
- <aXNCtCZwP57Tfu60@pks.im>
On Fri, Jan 23, 2026 at 10:43:16AM +0100, Patrick Steinhardt wrote:
Show 41 quoted lines
> 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).
Show 45 quoted lines
> > 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