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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jan 23, 2026 at 12:48:32PM -0500, Taylor Blau wrote:
> On Fri, Jan 23, 2026 at 10:43:07AM +0100, Patrick Steinhardt wrote:
> > > > 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.
> 
> I'm OK with pushing the larger change down the road, but I am a little
> uncomfortable with the interim state being introduced here. Perhaps a
> compromise here would be to have the caller supply a pointer to an
> object_info struct, whose request fields we honor. The response fields
> would then be written into a separate object_info struct via an
> out-parameter.
> 
> I don't know. I think that ^ this suggestion is kind of ugly, but I'm
> trying to come up with something that doesn't introduce the risk I
> described above in the interim between this patch series and the one
> you're proposing later on.

I think it's actually not _that_ ugly, and I like the additional safety
that it brings us.

Thanks!

Patrick

diff --git a/object-file.c b/object-file.c
index bc5209f2fe..6785821c8c 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1804,7 +1804,7 @@ int for_each_loose_file_in_source(struct odb_source *source,
 
 struct for_each_object_wrapper_data {
 	struct odb_source *source;
-	struct object_info *oi;
+	const struct object_info *request;
 	odb_for_each_object_cb cb;
 	void *cb_data;
 };
@@ -1814,21 +1814,28 @@ static int for_each_object_wrapper_cb(const struct object_id *oid,
 				      void *cb_data)
 {
 	struct for_each_object_wrapper_data *data = cb_data;
-	if (data->oi &&
-	    read_object_info_from_path(data->source, path, oid, data->oi, 0) < 0)
+
+	if (data->request) {
+		struct object_info oi = *data->request;
+
+		if (read_object_info_from_path(data->source, path, oid, &oi, 0) < 0)
 			return -1;
-	return data->cb(oid, data->oi, data->cb_data);
+
+		return data->cb(oid, &oi, data->cb_data);
+	} else {
+		return data->cb(oid, NULL, data->cb_data);
+	}
 }
 
 int odb_source_loose_for_each_object(struct odb_source *source,
-				     struct object_info *oi,
+				     const struct object_info *request,
 				     odb_for_each_object_cb cb,
 				     void *cb_data,
 				     unsigned flags)
 {
 	struct for_each_object_wrapper_data data = {
 		.source = source,
-		.oi = oi,
+		.request = request,
 		.cb = cb,
 		.cb_data = cb_data,
 	};
diff --git a/object-file.h b/object-file.h
index af7f57d2a1..d9979baea8 100644
--- a/object-file.h
+++ b/object-file.h
@@ -128,12 +128,13 @@ int for_each_loose_file_in_source(struct odb_source *source,
 
 /*
  * Iterate through all loose objects in the given object database source and
- * invoke the callback function for each of them. If given, the object info
- * will be populated with the object's data as if you had called
- * `odb_source_loose_read_object_info()` on the object.
+ * invoke the callback function for each of them. If an object info request is
+ * given, then the object info will be read for every individual object and
+ * passed to the callback as if `odb_source_loose_read_object_info()` was
+ * called for the object.
  */
 int odb_source_loose_for_each_object(struct odb_source *source,
-				     struct object_info *oi,
+				     const struct object_info *request,
 				     odb_for_each_object_cb cb,
 				     void *cb_data,
 				     unsigned flags);
diff --git a/odb.c b/odb.c
index 67decd3908..9d9a3fad62 100644
--- a/odb.c
+++ b/odb.c
@@ -998,7 +998,7 @@ int odb_freshen_object(struct object_database *odb,
 }
 
 int odb_for_each_object(struct object_database *odb,
-			struct object_info *oi,
+			const struct object_info *request,
 			odb_for_each_object_cb cb,
 			void *cb_data,
 			unsigned flags)
@@ -1011,12 +1011,14 @@ int odb_for_each_object(struct object_database *odb,
 			continue;
 
 		if (!(flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY)) {
-			ret = odb_source_loose_for_each_object(source, oi, cb, cb_data, flags);
+			ret = odb_source_loose_for_each_object(source, request,
+							       cb, cb_data, flags);
 			if (ret)
 				return ret;
 		}
 
-		ret = packfile_store_for_each_object(source->packfiles, oi, cb, cb_data, flags);
+		ret = packfile_store_for_each_object(source->packfiles, request,
+						     cb, cb_data, flags);
 		if (ret)
 			return ret;
 	}
diff --git a/odb.h b/odb.h
index 72d69ffcb3..8ad0fcc02f 100644
--- a/odb.h
+++ b/odb.h
@@ -492,6 +492,9 @@ typedef int (*odb_for_each_object_cb)(const struct object_id *oid,
  * Iterate through all objects contained in the object database. Note that
  * objects may be iterated over multiple times in case they are either stored
  * in different backends or in case they are stored in multiple sources.
+ * If an object info request is given, then the object info will be read and
+ * passed to the callback as if `odb_read_object_info()` was called for the
+ * object.
  *
  * Returning a non-zero error code from the callback function will cause
  * iteration to abort. The error code will be propagated.
@@ -500,7 +503,7 @@ typedef int (*odb_for_each_object_cb)(const struct object_id *oid,
  * an arbitrary non-zero error code returned by the callback itself.
  */
 int odb_for_each_object(struct object_database *odb,
-			struct object_info *oi,
+			const struct object_info *request,
 			odb_for_each_object_cb cb,
 			void *cb_data,
 			unsigned flags);
diff --git a/packfile.c b/packfile.c
index e455150d65..57fbf51876 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2329,7 +2329,7 @@ int for_each_object_in_pack(struct packed_git *p,
 
 struct packfile_store_for_each_object_wrapper_data {
 	struct packfile_store *store;
-	struct object_info *oi;
+	const struct object_info *request;
 	odb_for_each_object_cb cb;
 	void *cb_data;
 };
@@ -2341,28 +2341,31 @@ static int packfile_store_for_each_object_wrapper(const struct object_id *oid,
 {
 	struct packfile_store_for_each_object_wrapper_data *data = cb_data;
 
-	if (data->oi) {
+	if (data->request) {
 		off_t offset = nth_packed_object_offset(pack, index_pos);
+		struct object_info oi = *data->request;
 
 		if (packed_object_info_with_index_pos(pack, offset,
-						      &index_pos, data->oi) < 0) {
+						      &index_pos, &oi) < 0) {
 			mark_bad_packed_object(pack, oid);
 			return -1;
 		}
-	}
 
-	return data->cb(oid, data->oi, data->cb_data);
+		return data->cb(oid, &oi, data->cb_data);
+	} else {
+		return data->cb(oid, NULL, data->cb_data);
+	}
 }
 
 int packfile_store_for_each_object(struct packfile_store *store,
-				   struct object_info *oi,
+				   const struct object_info *request,
 				   odb_for_each_object_cb cb,
 				   void *cb_data,
 				   unsigned flags)
 {
 	struct packfile_store_for_each_object_wrapper_data data = {
 		.store = store,
-		.oi = oi,
+		.request = request,
 		.cb = cb,
 		.cb_data = cb_data,
 	};
diff --git a/packfile.h b/packfile.h
index 8e0d2b7661..1a1b720764 100644
--- a/packfile.h
+++ b/packfile.h
@@ -343,14 +343,15 @@ int for_each_object_in_pack(struct packed_git *p,
 
 /*
  * Iterate through all packed objects in the given packfile store and invoke
- * the callback function for each of them. If given, the object info will be
- * populated with the object's data as if you had called
- * `packfile_store_read_object_info()` on the object.
+ * the callback function for each of them. If an object info request is given,
+ * then the object info will be read for every individual object and passed to
+ * the callback as if `packfile_store_read_object_info()` was called for the
+ * object.
  *
  * The flags parameter is a combination of `odb_for_each_object_flags`.
  */
 int packfile_store_for_each_object(struct packfile_store *store,
-				   struct object_info *oi,
+				   const struct object_info *request,
 				   odb_for_each_object_cb cb,
 				   void *cb_data,
 				   unsigned flags);