Re: [PATCH v3 11/14] odb: introduce mtime fields for object info requests

[Taylor Blau <me@ttaylorr.com>]

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/object-file.c b/object-file.c
> index 65e730684b..c0f896673b 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -409,6 +409,7 @@ static int read_object_info_from_path(struct odb_source *source,
>  	char hdr[MAX_HEADER_LEN];
>  	unsigned long size_scratch;
>  	enum object_type type_scratch;
> +	struct stat st;

I was a little confused why we were declaring a stat struct here...

>  	/*
>  	 * If we don't care about type or size, then we don't
> @@ -421,7 +422,7 @@ static int read_object_info_from_path(struct odb_source *source,
>  	if (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {
>  		struct stat st;
>
> -		if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK)) {
> +		if ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) {
>  			ret = quick_has_loose(source->loose, oid) ? 0 : -1;
>  			goto out;
>  		}
> @@ -431,8 +432,12 @@ static int read_object_info_from_path(struct odb_source *source,
>  			goto out;
>  		}
>
> -		if (oi && oi->disk_sizep)
> -			*oi->disk_sizep = st.st_size;
> +		if (oi) {
> +			if (oi->disk_sizep)
> +				*oi->disk_sizep = st.st_size;

...and then assigning it here without actually calling lstat() between
the two. But the diff context elides the fact that there is another stat
declaration within this block that we *do* lstat() into before reading
it.

That tripped me up a little while reviewing, but not a huge deal. I do
wonder whether or not there is a clearer way to structure all of these
conditionals. I *think* that what you wrote here is right, but the way
that it has grown organically over time (to be clear, not the fault of
your series) makes it a little difficult to follow.

> +			if (oi->mtimep)
> +				*oi->mtimep = st.st_mtime;
> +		}
>
>  		ret = 0;
>  		goto out;
> @@ -446,7 +451,21 @@ static int read_object_info_from_path(struct odb_source *source,
>  		goto out;
>  	}
>
> -	map = map_fd(fd, path, &mapsize);
> +	if (fstat(fd, &st)) {
> +		close(fd);
> +		ret = -1;
> +		goto out;
> +	}

Makes sense. We were previously letting map_fd() take care of stat()-ing
the file to know how large the mmap should be, but now we might need
that information for the mtime as well. So doing what map_fd() is doing
underneath here directly makes sense.

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

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

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

Thanks,
Taylor