From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 23 Jan 2026 09:43:07 GMT
Subject: Re: [PATCH v3 11/14] odb: introduce mtime fields for object info requests
Message-ID: <aXNCq8h94i2Z6uSa@pks.im>
In-Reply-To: <aXLJoDdoEyKXKtBf@nand.local>

On Thu, Jan 22, 2026 at 08:06:40PM -0500, Taylor Blau wrote:
> On Wed, Jan 21, 2026 at 01:50:27PM +0100, Patrick Steinhardt wrote:
> > There are some use cases where we need to figure out the mtime for
> > objects. Most importantly, this is the case when we want to prune
> > unreachable objects. But getting at that data requires users to manually
> > derive the info either via the loose object's mtime, the packfiles'
> > mtime or via the ".mtimes" file.
> >
> > Introduce a new `struct object_info::mtimep` pointer that allows callers
> > to request an object's mtime. This new field will be used in a
> > subsequent commit.
> 
> The goal seems reasonable to me, but I am a little unsure about whether
> or not this is the right place to expose this information. I have some
> more thoughts below...
> 
> > diff --git a/odb.c b/odb.c
> > index 65f0447aa5..67decd3908 100644
> > --- a/odb.c
> > +++ b/odb.c
> > @@ -702,6 +702,8 @@ static int do_oid_object_info_extended(struct object_database *odb,
> >  				oidclr(oi->delta_base_oid, odb->repo->hash_algo);
> >  			if (oi->contentp)
> >  				*oi->contentp = xmemdupz(co->buf, co->size);
> > +			if (oi->mtimep)
> > +				*oi->mtimep = 0;
> 
> Assuming that you do not change the object_info request/response
> semantics, I wonder if it might make sense to zero out the entirety of
> the response section as a belt-and-suspenders mechanism in case future
> contributors forget to assign zero to the new fields themselves.

Splitting up the request/response structure as you proposed in a
previous patch could definitely help with this. I'd prefer to rather do
such a bigger change as a follow-up though as it would lead to a lot of
churn.

> > @@ -1619,16 +1620,34 @@ int packed_object_info(struct packed_git *p,
> >  		}
> >  	}
> >
> > -	if (oi->disk_sizep) {
> > -		uint32_t pos;
> > -		if (offset_to_pack_pos(p, obj_offset, &pos) < 0) {
> > +	if (oi->disk_sizep || (oi->mtimep && p->is_cruft)) {
> > +		if (offset_to_pack_pos(p, obj_offset, &pack_pos) < 0) {
> >  			error("could not find object at offset %"PRIuMAX" "
> >  			      "in pack %s", (uintmax_t)obj_offset, p->pack_name);
> >  			ret = -1;
> >  			goto out;
> >  		}
> > +	}
> > +
> > +	if (oi->disk_sizep)
> > +		*oi->disk_sizep = pack_pos_to_offset(p, pack_pos + 1) - obj_offset;
> > +
> > +	if (oi->mtimep) {
> > +		if (p->is_cruft) {
> > +			uint32_t index_pos;
> > +
> > +			if (load_pack_mtimes(p) < 0)
> > +				die(_("could not load cruft pack .mtimes"));
> 
> Do you think it would be worth doing instead:
> 
>     die(_("could not load .mtimes for cruft pack '%s'"), pack_basename(p));
> 
> ? Most repositories should only ever have one cruft pack in practice
> (even so, there should still be some value in identifying it by its
> checksum in case someone is repacking underneath us). But some
> repositories will have >1 cruft pack, so knowing which one is busted may
> be useful in that case.

Yup, makes sense.

> > +
> > +			if (maybe_index_pos)
> > +				index_pos = *maybe_index_pos;
> > +			else
> > +				index_pos = pack_pos_to_index(p, pack_pos);
> >
> > -		*oi->disk_sizep = pack_pos_to_offset(p, pos + 1) - obj_offset;
> > +			*oi->mtimep = nth_packed_mtime(p, index_pos);
> > +		} else {
> > +			*oi->mtimep = p->mtime;
> > +		}
> 
> I am a little stuck here on whether or not this is the right layer to
> determine an object's mtime. On the one hand, it makes sense to me that
> callers would want to know the mtime of an object, either by the mtime
> of the loose object on disk, or the mtime of the contain pack otherwise.
> 
> But I'm not sure whether the GC-specific definition of "mtime" is what
> the caller would always want. For GC uses, yes, having mtime be aware of
> cruft packs makes total sense to me. But for non-GC uses, would there
> ever be a scenario where the caller would want to know the mtime of an
> object's containing pack, regardless of whether or not that pack is
> cruft?
> 
> I suppose they could get around that today by doing something like:
> 
>     if (oi->whence == OI_PACKED) {
>         struct packed_git *p = oi->u.packed.p;
>         if (p->is_cruft) {
>             /* reinterpret the meaning of mtime... */
>             *oi->mtimep = p->mtime;
>         }
>     }
> 
> , but that feels a little clunky. I dunno, maybe this hypothetical
> doesn't really exist and I'm overthinking this. But I have this nagging
> feeling that we are exposing this information at too low of a level as
> to make the object store aware of cruft pack/GC-specific mechanics.

I'll answer on your next mail, where you also talk about this.

Patrick