[PATCH v2 4/5] odb/source: allow `read_object_info()` to bubble up error messages

When reading an object fails even though it exists, the sources know
best what exactly went wrong and where the corrupt object is located.
This information is lost though when bubbling up the error to the object
database layer, which forces that layer to reconstruct it after the
fact. This is exactly what `do_oid_object_info_extended()` does via
`has_packed_and_bad()`, but that function only really knows to handle
the "files" backend by reaching into its internals.

Introduce a new `errmsg` parameter for the `read_object_info()` callback
that sources are expected to populate with a human-readable message in
case reading the object has failed. Adapt the packed and loose sources
to populate the buffer with the messages that we ultimately want to
surface to the user.

For now, all callers are adapted to pass a `NULL` pointer. We will add a
user of this new infrastructure in a subsequent commit.

Signed-off-by: Patrick Steinhardt <[email protected]>
---
 builtin/pack-objects.c        |  6 +++---
 odb.c                         |  7 ++++---
 odb/source-files.c            |  9 ++++++---
 odb/source-inmemory.c         |  3 ++-
 odb/source-loose.c            | 23 +++++++++++++++--------
 odb/source-packed.c           | 34 ++++++++++++++++++++++++++--------
 odb/source.h                  | 18 ++++++++++++++----
 packfile.c                    |  2 +-
 t/unit-tests/u-odb-inmemory.c |  4 ++--
 9 files changed, 73 insertions(+), 33 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 10c2471024..399acd0f22 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1759,7 +1759,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
 		struct odb_source *source = the_repository->objects->sources->next;
 		for (; source; source = source->next) {
 			struct odb_source_files *files = odb_source_files_downcast(source);
-			if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0))
+			if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL))
 				return 0;
 		}
 	}
@@ -4171,7 +4171,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type
 
 			for (; !found && source; source = source->next) {
 				struct odb_source_files *files = odb_source_files_downcast(source);
-				if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0))
+				if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL))
 					found = 1;
 			}
 
@@ -4637,7 +4637,7 @@ static int force_object_loose(struct odb_source *source,
 
 	for (struct odb_source *s = source->odb->sources; s; s = s->next) {
 		struct odb_source_files *files = odb_source_files_downcast(s);
-		if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0))
+		if (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL))
 			return 0;
 	}
 
diff --git a/odb.c b/odb.c
index 1b37b26376..83a53f7f6b 100644
--- a/odb.c
+++ b/odb.c
@@ -560,7 +560,7 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 	if (is_null_oid(real))
 		return -1;
 
-	if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags))
+	if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags, NULL))
 		return 0;
 
 	odb_prepare_alternates(odb);
@@ -569,7 +569,7 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 		struct odb_source *source;
 
 		for (source = odb->sources; source; source = source->next)
-			if (!odb_source_read_object_info(source, real, oi, flags))
+			if (!odb_source_read_object_info(source, real, oi, flags, NULL))
 				return 0;
 
 		/*
@@ -580,7 +580,8 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 		if (!(flags & OBJECT_INFO_QUICK)) {
 			for (source = odb->sources; source; source = source->next)
 				if (!odb_source_read_object_info(source, real, oi,
-								 flags | OBJECT_INFO_SECOND_READ))
+								 flags | OBJECT_INFO_SECOND_READ,
+								 NULL))
 					return 0;
 		}
 
diff --git a/odb/source-files.c b/odb/source-files.c
index e88fd1d399..aafba358e4 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -62,16 +62,19 @@ static void odb_source_files_prepare(struct odb_source *source,
 static enum odb_read_status odb_source_files_read_object_info(struct odb_source *source,
 							      const struct object_id *oid,
 							      struct object_info *oi,
-							      enum object_info_flags flags)
+							      enum object_info_flags flags,
+							      struct strbuf *errmsg)
 {
 	struct odb_source_files *files = odb_source_files_downcast(source);
 	enum odb_read_status ret_packed, ret_loose;
 
-	ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags);
+	ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi,
+						 flags, errmsg);
 	if (!ret_packed)
 		return 0;
 
-	ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags);
+	ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags,
+						ret_packed == ODB_READ_NOT_FOUND ? errmsg : NULL);
 	if (!ret_loose)
 		return 0;
 
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 3f3bd12de3..12f91e594a 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -59,7 +59,8 @@ static void populate_object_info(struct odb_source_inmemory *source,
 static enum odb_read_status odb_source_inmemory_read_object_info(struct odb_source *source,
 								 const struct object_id *oid,
 								 struct object_info *oi,
-								 enum object_info_flags flags UNUSED)
+								 enum object_info_flags flags UNUSED,
+								 struct strbuf *errmsg UNUSED)
 {
 	struct odb_source_inmemory *inmemory = odb_source_inmemory_downcast(source);
 	const struct inmemory_object *object;
diff --git a/odb/source-loose.c b/odb/source-loose.c
index 3c942a1069..b57ee2701a 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -67,7 +67,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 				      const char *path,
 				      const struct object_id *oid,
 				      struct object_info *oi,
-				      enum object_info_flags flags)
+				      enum object_info_flags flags,
+				      struct strbuf *errmsg)
 {
 	int ret;
 	int fd;
@@ -191,9 +192,14 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 	ret = 0;
 
 out:
-	if (ret && ret != ODB_READ_NOT_FOUND && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
-		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(oid), path);
+	if (ret && ret != ODB_READ_NOT_FOUND) {
+		if ((flags & OBJECT_INFO_DIE_IF_CORRUPT))
+			die(_("loose object %s (stored in %s) is corrupt"),
+			    oid_to_hex(oid), path);
+		if (errmsg)
+			strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"),
+				    oid_to_hex(oid), path);
+	}
 
 	if (stream_to_end)
 		git_inflate_end(stream_to_end);
@@ -216,7 +222,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 static enum odb_read_status odb_source_loose_read_object_info(struct odb_source *source,
 							      const struct object_id *oid,
 							      struct object_info *oi,
-							      enum object_info_flags flags)
+							      enum object_info_flags flags,
+							      struct strbuf *errmsg)
 {
 	struct odb_source_loose *loose = odb_source_loose_downcast(source);
 	static struct strbuf buf = STRBUF_INIT;
@@ -231,7 +238,7 @@ static enum odb_read_status odb_source_loose_read_object_info(struct odb_source
 		return ODB_READ_NOT_FOUND;
 
 	odb_loose_path(loose, &buf, oid);
-	return read_object_info_from_path(loose, buf.buf, oid, oi, flags);
+	return read_object_info_from_path(loose, buf.buf, oid, oi, flags, errmsg);
 }
 
 /*
@@ -428,7 +435,7 @@ static int for_each_object_wrapper_cb(const struct object_id *oid,
 	if (data->request) {
 		struct object_info oi = *data->request;
 
-		if (read_object_info_from_path(data->loose, path, oid, &oi, 0) < 0)
+		if (read_object_info_from_path(data->loose, path, oid, &oi, 0, NULL) < 0)
 			return -1;
 
 		return data->cb(oid, &oi, data->cb_data);
@@ -446,7 +453,7 @@ static int for_each_prefixed_object_wrapper_cb(const struct object_id *oid,
 		struct object_info oi = *data->request;
 
 		if (odb_source_read_object_info(&data->loose->base,
-						oid, &oi, 0) < 0)
+						oid, &oi, 0, NULL) < 0)
 			return -1;
 
 		return data->cb(oid, &oi, data->cb_data);
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 9b19405380..1a12a605db 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -2,7 +2,9 @@
 #include "abspath.h"
 #include "chdir-notify.h"
 #include "dir.h"
+#include "gettext.h"
 #include "git-zlib.h"
+#include "hex.h"
 #include "list-objects-filter-options.h"
 #include "mergesort.h"
 #include "midx.h"
@@ -10,6 +12,7 @@
 #include "odb/streaming.h"
 #include "packfile.h"
 #include "pack-bitmap.h"
+#include "strbuf.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -38,7 +41,8 @@ static int find_pack_entry(struct odb_source_packed *store,
 static enum odb_read_status odb_source_packed_read_object_info(struct odb_source *source,
 							       const struct object_id *oid,
 							       struct object_info *oi,
-							       enum object_info_flags flags)
+							       enum object_info_flags flags,
+							       struct strbuf *errmsg)
 {
 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
 	struct packed_git *bad_pack = NULL;
@@ -59,25 +63,39 @@ static enum odb_read_status odb_source_packed_read_object_info(struct odb_source
 		 * corrupt in one of the packfiles. Report the object as
 		 * corrupt instead of missing in that case.
 		 */
-		if (bad_pack)
-			return -1;
-		return ODB_READ_NOT_FOUND;
+		if (bad_pack) {
+			ret = -1;
+			goto out;
+		}
+
+		ret = ODB_READ_NOT_FOUND;
+		goto out;
 	}
 
 	/*
 	 * We know that the caller doesn't actually need the
 	 * information below, so return early.
 	 */
-	if (!oi)
-		return 0;
+	if (!oi) {
+		ret = 0;
+		goto out;
+	}
 
 	ret = packed_object_info(packed, e.p, e.offset, oi);
 	if (ret < 0) {
+		bad_pack = e.p;
 		mark_bad_packed_object(e.p, oid);
-		return -1;
+		goto out;
 	}
 
-	return 0;
+	ret = 0;
+
+out:
+	if (ret < 0 && bad_pack && errmsg)
+		strbuf_addf(errmsg, _("packed object %s (stored in %s) is corrupt"),
+			    oid_to_hex(oid), bad_pack->pack_name);
+
+	return ret;
 }
 
 static int odb_source_packed_read_object_stream(struct odb_read_stream **out,
diff --git a/odb/source.h b/odb/source.h
index 7b8ff3d19d..4d13e4cfaf 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -27,6 +27,7 @@ enum odb_source_type {
 
 struct object_id;
 struct odb_read_stream;
+struct strbuf;
 struct strvec;
 
 /*
@@ -111,12 +112,16 @@ struct odb_source {
 	 *     already surfaced the object without reloading any on-disk state.
 	 *
 	 * The callback is expected to return an `enum odb_read_status`. Please
-	 * refer to the individual values that can be returned.
+	 * refer to the individual values that can be returned. In case reading
+	 * the object has failed with a generic error and `errmsg` is non-NULL,
+	 * the callback is expected to populate it with a human-readable
+	 * message that describes the failure.
 	 */
 	enum odb_read_status (*read_object_info)(struct odb_source *source,
 						 const struct object_id *oid,
 						 struct object_info *oi,
-						 enum object_info_flags flags);
+						 enum object_info_flags flags,
+						 struct strbuf *errmsg);
 
 	/*
 	 * This callback is expected to create a new read stream that can be
@@ -341,13 +346,18 @@ static inline void odb_source_prepare(struct odb_source *source,
 /*
  * Read an object from the object database source identified by its object ID.
  * Please refer to `enum odb_read_status` for the individual error codes.
+ *
+ * In case reading the object has failed with a generic error and `errmsg` is
+ * non-NULL it will be populated with a human-readable message that describes
+ * the failure.
  */
 static inline enum odb_read_status odb_source_read_object_info(struct odb_source *source,
 							       const struct object_id *oid,
 							       struct object_info *oi,
-							       enum object_info_flags flags)
+							       enum object_info_flags flags,
+							       struct strbuf *errmsg)
 {
-	return source->read_object_info(source, oid, oi, flags);
+	return source->read_object_info(source, oid, oi, flags, errmsg);
 }
 
 /*
diff --git a/packfile.c b/packfile.c
index 34e2f9bb8b..3cde39a01c 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1945,7 +1945,7 @@ int has_object_pack(struct repository *r, const struct object_id *oid)
 	odb_prepare_alternates(r->objects);
 	for (source = r->objects->sources; source; source = source->next) {
 		struct odb_source_files *files = odb_source_files_downcast(source);
-		if (!odb_source_read_object_info(&files->packed->base, oid, NULL, 0))
+		if (!odb_source_read_object_info(&files->packed->base, oid, NULL, 0, NULL))
 			return 1;
 	}
 
diff --git a/t/unit-tests/u-odb-inmemory.c b/t/unit-tests/u-odb-inmemory.c
index 3e5068080c..095c20ba91 100644
--- a/t/unit-tests/u-odb-inmemory.c
+++ b/t/unit-tests/u-odb-inmemory.c
@@ -29,7 +29,7 @@ static void cl_assert_object_info(struct odb_source_inmemory *source,
 		.contentp = &actual_content,
 	};
 
-	cl_must_pass(odb_source_read_object_info(&source->base, oid, &oi, 0));
+	cl_must_pass(odb_source_read_object_info(&source->base, oid, &oi, 0, NULL));
 	cl_assert_equal_u(actual_size, strlen(expected_content));
 	cl_assert_equal_u(actual_type, expected_type);
 	cl_assert_equal_s((char *) actual_content, expected_content);
@@ -72,7 +72,7 @@ void test_odb_inmemory__read_missing_object(void)
 	const char *end;
 
 	cl_must_pass(parse_oid_hex_algop(RANDOM_OID, &oid, &end, repo.hash_algo));
-	cl_assert_equal_i(odb_source_read_object_info(&source->base, &oid, NULL, 0),
+	cl_assert_equal_i(odb_source_read_object_info(&source->base, &oid, NULL, 0, NULL),
 			  ODB_READ_NOT_FOUND);
 
 	odb_source_free(&source->base);

-- 
2.55.0.822.g20453c30eb.dirty