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

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 23, 2026, 01:06 UTC
Message-ID: <aXLJoDdoEyKXKtBf@nand.local>
In-Reply-To: <20260121-pks-odb-for-each-object-v3-11-12c4dfd24227@pks.im>

On Wed, Jan 21, 2026 at 01:50:27PM +0100, Patrick Steinhardt wrote:

Show 9 quoted lines

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

Show 9 quoted lines

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

Show 20 quoted lines

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

Show 16 quoted lines

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

Show 10 quoted lines

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

Show 25 quoted lines

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

Show 11 quoted lines

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