[PATCH 0/7] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically

[PATCH 0/7] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically

[Patrick Steinhardt]

Hi,

when looking up an object with `OBJECT_INFO_DIE_IF_CORRUPT` fails we
want to die in case the object exists but is corrupted. This flag is
handled in two different spots right now:

  - `do_oid_object_info_extended()` calls `has_packed_and_bad()` to
    check whether the object is known to be corrupt in any packfile.
    This function reaches into the internals of the packed source and
    thus breaks the abstraction provided by our object sources.

  - The loose source handles the flag itself and dies directly in
    `read_object_info_from_path()`, which means that we die even in
    cases where another source may still have a good copy of the
    object.

Besides being inconsistent, it also ties us to the specific backend used
by the database sources because `has_packed_and_bad()` assumes that they
use the "files" backend. Any other backend will instead cause us to die
when calling `odb_source_files_downcast()`, even if the object was
simply nonexistent.

This series fixes these issues and makes the check backend-agnostic by
extending semantics of `odb_source_read_object_info()`: on the one hand
it now distinguishes whether an object is missing or corrput, and on the
other hand it starts to return an error message to the caller.

Thanks!

Patrick

---
Patrick Steinhardt (7):
      odb/source: discern missing and corrupt objects
      odb/source-inmemory: signal missing objects via positive return
      odb/source-packed: flag known-bad objects as corrupt and not missing
      odb/source-loose: distinguish missing and corrupt objects
      odb/source-files: signal mark objects via positive return
      odb/source: allow `read_object_info()` to bubble up error messages
      odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically

 builtin/pack-objects.c        |  8 +++----
 midx.c                        | 10 +++++---
 midx.h                        |  3 ++-
 odb.c                         | 47 ++++++++++++++++++++++++++------------
 odb/source-files.c            | 25 ++++++++++++++++----
 odb/source-inmemory.c         |  5 ++--
 odb/source-loose.c            | 46 +++++++++++++++++++++----------------
 odb/source-packed.c           | 53 +++++++++++++++++++++++++++++++++----------
 odb/source.h                  | 33 ++++++++++++++++++++++-----
 packfile.c                    | 29 +++++++----------------
 packfile.h                    |  4 ++--
 t/helper/test-read-midx.c     |  2 +-
 t/t1060-object-corruption.sh  | 18 +++++++++++++++
 t/unit-tests/u-odb-inmemory.c |  4 ++--
 14 files changed, 196 insertions(+), 91 deletions(-)


---
base-commit: 18e66859d87fb4b76599f73460b54f0848c76b16
change-id: 20260818-pks-odb-generic-corrupt-objects-52a47d6214d9

[PATCH 1/7] odb/source: discern missing and corrupt objects

[Patrick Steinhardt]

The `read_object_info()` callback of `struct odb_source` is documented
to return a negative error code in case reading the object has failed,
and zero otherwise. This is overly broad though, as there are two very
different kinds of failures:

  - The object may not exist in the source at all.

  - The object exists, but reading it has failed, for example because
    its on-disk state is corrupt.

This distinction matters to callers: when an object is corrupt in one
source we may still find a good copy of it in another source, so we may
still be able to proceed with a given operation.

The "packed" source already distinguishes these cases by returning a
positive value for missing objects and a negative value in case reading
the object has failed. But all the other sources conflate them into a
single negative return value.

Adapt the documentation to explicitly require the semantics of the
"packed" backend, where we return a positive value for missing objects
and a negative value for corrupt ones. Subsequent commits will adapt all
the other implementations to respect those new semantics.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source.h | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/odb/source.h b/odb/source.h
index d69f8e2d1c..4ae6cc160e 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -110,8 +110,17 @@ struct odb_source {
 	 *     second read in case they know that the first read would have
 	 *     already surfaced the object without reloading any on-disk state.
 	 *
-	 * The callback is expected to return a negative error code in case
-	 * reading the object has failed, 0 otherwise.
+	 * The callback is expected to return one of the following values:
+	 *
+	 *   - Zero in case the object has been found and its object info has
+	 *     been read successfully.
+	 *
+	 *   - A positive value in case the object does not exist in this
+	 *     source.
+	 *
+	 *   - A negative value in case the object exists in this source, but
+	 *     reading its object info has failed, for example because its
+	 *     on-disk state is corrupt.
 	 */
 	int (*read_object_info)(struct odb_source *source,
 				const struct object_id *oid,
@@ -340,7 +349,9 @@ static inline void odb_source_prepare(struct odb_source *source,
 
 /*
  * Read an object from the object database source identified by its object ID.
- * Returns 0 on success, a negative error code otherwise.
+ * Returns 0 on success, a positive value in case the object is missing in the
+ * source and a negative value in case the object exists, but reading it has
+ * failed.
  */
 static inline int odb_source_read_object_info(struct odb_source *source,
 					      const struct object_id *oid,

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 2/7] odb/source-inmemory: signal missing objects via positive return

[Patrick Steinhardt]

The in-memory source returns a negative value from its
`read_object_info()` callback when the object in question does not
exist. Adapt the callback to return a positive value for missing objects
according to the new calling convention.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-inmemory.c         | 2 +-
 t/unit-tests/u-odb-inmemory.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 3e71611b8e..57183daf4d 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -66,7 +66,7 @@ static int odb_source_inmemory_read_object_info(struct odb_source *source,
 
 	object = find_cached_object(inmemory, oid);
 	if (!object)
-		return -1;
+		return 1;
 
 	populate_object_info(inmemory, oi, object);
 	return 0;
diff --git a/t/unit-tests/u-odb-inmemory.c b/t/unit-tests/u-odb-inmemory.c
index ddf2db5c81..93b3f38dab 100644
--- a/t/unit-tests/u-odb-inmemory.c
+++ b/t/unit-tests/u-odb-inmemory.c
@@ -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_must_fail(odb_source_read_object_info(&source->base, &oid, NULL, 0));
+	cl_assert(odb_source_read_object_info(&source->base, &oid, NULL, 0) > 0);
 
 	odb_source_free(&source->base);
 }

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing

[Patrick Steinhardt]

When reading a packed object that doesn't verify we mark it as bad and
indicate to the caller that we failed reading the object despite the
fact that it supposedly exists. This matches the semantics we have now
established in a preceding commit, where we discern failure to read a
corrupt object from a missing object.

What doesn't work yet though is when a call tries to read an object that
has already been marked as corrupt in a previous call. In that case,
`find_pack_entry()` will tell us that the object in question does not
exist, and consequently we'll not flag the object as corrupt but as
missing.

Fix this issue by bubbling up whether the object is corrupt and, if so,
which packfile contains the corrupted object. We don't yet need the
latter information about the specific packfile, so we could've just as
well made this a `bool *corrupted` pointer. But we'll need information
about the containing packfile in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-objects.c    |  2 +-
 midx.c                    | 10 +++++++---
 midx.h                    |  3 ++-
 odb/source-packed.c       | 23 +++++++++++++++++------
 packfile.c                | 10 +++++++---
 packfile.h                |  3 ++-
 t/helper/test-read-midx.c |  2 +-
 7 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1ec5b6f206..10c2471024 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
 		struct multi_pack_index *m = get_multi_pack_index(files->packed);
 		struct pack_entry e;
 
-		if (m && fill_midx_entry(m, oid, &e)) {
+		if (m && fill_midx_entry(m, oid, &e, NULL)) {
 			want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
 			if (want != -1)
 				return want;
diff --git a/midx.c b/midx.c
index 76c3f92cc3..37f082dbdd 100644
--- a/midx.c
+++ b/midx.c
@@ -591,7 +591,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)
 
 int fill_midx_entry(struct multi_pack_index *m,
 		    const struct object_id *oid,
-		    struct pack_entry *e)
+		    struct pack_entry *e,
+		    struct packed_git **bad_pack)
 {
 	uint32_t pos;
 	uint32_t pack_int_id;
@@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,
 		return 0;
 
 	if (oidset_size(&p->bad_objects) &&
-	    oidset_contains(&p->bad_objects, oid))
+	    oidset_contains(&p->bad_objects, oid)) {
+		if (bad_pack && !*bad_pack)
+			*bad_pack = p;
 		return 0;
+	}
 
 	e->offset = nth_midxed_offset(m, pos);
 	e->p = p;
@@ -1028,7 +1032,7 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags)
 
 		nth_midxed_object_oid(&oid, m, pairs[i].pos);
 
-		if (!fill_midx_entry(m, &oid, &e)) {
+		if (!fill_midx_entry(m, &oid, &e, NULL)) {
 			midx_report(_("failed to load pack entry for oid[%d] = %s"),
 				    pairs[i].pos, oid_to_hex(&oid));
 			continue;
diff --git a/midx.h b/midx.h
index 939c18e588..1f2f2d5321 100644
--- a/midx.h
+++ b/midx.h
@@ -117,7 +117,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos);
 struct object_id *nth_midxed_object_oid(struct object_id *oid,
 					struct multi_pack_index *m,
 					uint32_t n);
-int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid, struct pack_entry *e);
+int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid,
+		    struct pack_entry *e, struct packed_git **bad_pack);
 int midx_contains_pack(struct multi_pack_index *m,
 		       const char *idx_or_pack_name);
 int midx_layer_contains_pack(struct multi_pack_index *m,
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0890704e76..50e9be3b4c 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -13,18 +13,19 @@
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
-			   struct pack_entry *e)
+			   struct pack_entry *e,
+			   struct packed_git **bad_pack)
 {
 	struct packfile_list_entry *l;
 
 	odb_source_prepare(&store->base, 0);
-	if (store->midx && fill_midx_entry(store->midx, oid, e))
+	if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))
 		return 1;
 
 	for (l = store->packs.head; l; l = l->next) {
 		struct packed_git *p = l->pack;
 
-		if (!p->multi_pack_index && packfile_fill_entry(p, oid, e)) {
+		if (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) {
 			if (!store->skip_mru_updates)
 				packfile_list_prepend(&store->packs, p);
 			return 1;
@@ -40,6 +41,7 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
 					      enum object_info_flags flags)
 {
 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
+	struct packed_git *bad_pack = NULL;
 	struct pack_entry e;
 	int ret;
 
@@ -51,8 +53,17 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
 	if (flags & OBJECT_INFO_SECOND_READ)
 		odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);
 
-	if (!find_pack_entry(packed, oid, &e))
+	if (!find_pack_entry(packed, oid, &e, &bad_pack)) {
+		/*
+		 * The lookup may have failed because the object is known to
+		 * be corrupt in one of our packfiles, in which case the
+		 * corresponding pack entries are skipped. Report the object
+		 * as corrupt instead of as missing in that case.
+		 */
+		if (bad_pack)
+			return -1;
 		return 1;
+	}
 
 	/*
 	 * We know that the caller doesn't actually need the
@@ -77,7 +88,7 @@ static int odb_source_packed_read_object_stream(struct odb_read_stream **out,
 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
 	struct pack_entry e;
 
-	if (!find_pack_entry(packed, oid, &e))
+	if (!find_pack_entry(packed, oid, &e, NULL))
 		return -1;
 
 	return packfile_read_object_stream(out, oid, e.p, e.offset);
@@ -583,7 +594,7 @@ static int odb_source_packed_freshen_object(struct odb_source *source,
 		timesp = &times;
 	}
 
-	if (!find_pack_entry(packed, oid, &e))
+	if (!find_pack_entry(packed, oid, &e, NULL))
 		return 0;
 	if (e.p->is_cruft)
 		return 0;
diff --git a/packfile.c b/packfile.c
index 0eee45055f..34e2f9bb8b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1859,13 +1859,17 @@ int is_pack_valid(struct packed_git *p)
 
 int packfile_fill_entry(struct packed_git *p,
 			const struct object_id *oid,
-			struct pack_entry *e)
+			struct pack_entry *e,
+			struct packed_git **bad_pack)
 {
 	off_t offset;
 
 	if (oidset_size(&p->bad_objects) &&
-	    oidset_contains(&p->bad_objects, oid))
+	    oidset_contains(&p->bad_objects, oid)) {
+		if (bad_pack && !*bad_pack)
+			*bad_pack = p;
 		return 0;
+	}
 
 	offset = find_pack_entry_one(oid, p);
 	if (!offset)
@@ -1962,7 +1966,7 @@ int has_object_kept_pack(struct repository *r, const struct object_id *oid,
 
 		for (; *cache; cache++) {
 			struct packed_git *p = *cache;
-			if (packfile_fill_entry(p, oid, &e))
+			if (packfile_fill_entry(p, oid, &e, NULL))
 				return 1;
 		}
 	}
diff --git a/packfile.h b/packfile.h
index e1f77152b5..3229a6ed47 100644
--- a/packfile.h
+++ b/packfile.h
@@ -294,7 +294,8 @@ off_t find_pack_entry_one(const struct object_id *oid, struct packed_git *);
 
 int packfile_fill_entry(struct packed_git *p,
 			const struct object_id *oid,
-			struct pack_entry *e);
+			struct pack_entry *e,
+			struct packed_git **bad_pack);
 
 int is_pack_valid(struct packed_git *);
 void *unpack_entry(struct repository *r, struct packed_git *, off_t,
diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
index fb16ec0176..27a05da957 100644
--- a/t/helper/test-read-midx.c
+++ b/t/helper/test-read-midx.c
@@ -82,7 +82,7 @@ static int read_midx_file(const char *object_dir, const char *checksum,
 		for (i = 0; i < m->num_objects; i++) {
 			nth_midxed_object_oid(&oid, m,
 					      i + m->num_objects_in_base);
-			fill_midx_entry(m, &oid, &e);
+			fill_midx_entry(m, &oid, &e, NULL);
 
 			printf("%s %"PRIu64"\t%s\n",
 			       oid_to_hex(&oid), e.offset, e.p->pack_name);

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 4/7] odb/source-loose: distinguish missing and corrupt objects

[Patrick Steinhardt]

The loose source returns a negative value from its `read_object_info()`
callback both when the object is missing and when the object exists but
cannot be read. Consequently, callers cannot tell apart whether the
object does not exist in this source at all or whether it is corrupt.

Adapt the code to return a positive value for missing objects according
to the new calling convention.

This also allows us to get rid of the separate `corrupt:` label, as we
can now clearly distinguish between corrupt and missing objects in the
function ourselves. This makes us handle failures to read loose objects
more consistently, as not all failure cases were jumping that label.

Note that there's one call to `die()` when the object type is invalid
that should arguably be converted to an error, too. But adapting that
call results in quite a lot of broken tests, so this is left as-is for
now.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-loose.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/odb/source-loose.c b/odb/source-loose.c
index ef0e919277..e786560ad1 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -91,11 +91,16 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 		struct stat st;
 
 		if ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) {
-			ret = quick_has_loose(loose, oid) ? 0 : -1;
+			ret = quick_has_loose(loose, oid) ? 0 : 1;
 			goto out;
 		}
 
 		if (lstat(path, &st) < 0) {
+			if (errno == ENOENT) {
+				ret = 1;
+				goto out;
+			}
+
 			ret = -1;
 			goto out;
 		}
@@ -113,9 +118,12 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 
 	fd = git_open(path);
 	if (fd < 0) {
-		if (errno != ENOENT)
-			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
-		ret = -1;
+		if (errno == ENOENT) {
+			ret = 1;
+			goto out;
+		}
+
+		ret = error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
 		goto out;
 	}
 
@@ -155,7 +163,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 
 		if (parse_loose_header(hdr, oi) < 0) {
 			ret = error(_("unable to parse %s header"), oid_to_hex(oid));
-			goto corrupt;
+			goto out;
 		}
 
 		if (*oi->typep < 0)
@@ -165,7 +173,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 			*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
 			if (!*oi->contentp) {
 				ret = -1;
-				goto corrupt;
+				goto out;
 			}
 		}
 
@@ -173,21 +181,20 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 	case ULHR_BAD:
 		ret = error(_("unable to unpack %s header"),
 			    oid_to_hex(oid));
-		goto corrupt;
+		goto out;
 	case ULHR_TOO_LONG:
 		ret = error(_("header for %s too long, exceeds %d bytes"),
 			    oid_to_hex(oid), MAX_HEADER_LEN);
-		goto corrupt;
+		goto out;
 	}
 
 	ret = 0;
 
-corrupt:
-	if (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+out:
+	if (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
 		die(_("loose object %s (stored in %s) is corrupt"),
 		    oid_to_hex(oid), path);
 
-out:
 	if (stream_to_end)
 		git_inflate_end(stream_to_end);
 	if (map)
@@ -221,7 +228,7 @@ static int odb_source_loose_read_object_info(struct odb_source *source,
 	 * second time.
 	 */
 	if (flags & OBJECT_INFO_SECOND_READ)
-		return -1;
+		return 1;
 
 	odb_loose_path(loose, &buf, oid);
 	return read_object_info_from_path(loose, buf.buf, oid, oi, flags);
@@ -421,7 +428,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))
 			return -1;
 
 		return data->cb(oid, &oi, data->cb_data);
@@ -439,7 +446,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))
 			return -1;
 
 		return data->cb(oid, &oi, data->cb_data);

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 5/7] odb/source-files: signal mark objects via positive return

[Patrick Steinhardt]

The files source conflates all failures of its child sources into a
negative return value, so callers cannot tell apart whether an object is
missing or whether reading it has failed. Both the packed and the loose
source have been converted to adhere to the tri-state return convention
of `read_object_info()` by now, so all that is left to do is to
propagate their respective return values.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-files.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/odb/source-files.c b/odb/source-files.c
index 5a68af7d84..1124a18091 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -65,12 +65,26 @@ static int odb_source_files_read_object_info(struct odb_source *source,
 					     enum object_info_flags flags)
 {
 	struct odb_source_files *files = odb_source_files_downcast(source);
+	int ret_packed, ret_loose;
 
-	if (!odb_source_read_object_info(&files->packed->base, oid, oi, flags) ||
-	    !odb_source_read_object_info(&files->loose->base, oid, oi, flags))
+	ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags);
+	if (!ret_packed)
 		return 0;
 
-	return -1;
+	ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags);
+	if (!ret_loose)
+		return 0;
+
+	/*
+	 * Reading the packed object may have failed even though the object
+	 * exists, for example because it is corrupt. Report this failure to
+	 * the caller in case neither of the sources was able to read the
+	 * object, and prefer the error of the packed source in case both
+	 * reads have failed.
+	 */
+	if (ret_packed < 0)
+		return ret_packed;
+	return ret_loose;
 }
 
 static int odb_source_files_read_object_stream(struct odb_read_stream **out,

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 6/7] odb/source: allow `read_object_info()` to bubble up error messages

[Patrick Steinhardt]

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 <ps@pks.im>
---
 builtin/pack-objects.c        |  6 +++---
 odb.c                         |  7 ++++---
 odb/source-files.c            |  9 ++++++---
 odb/source-inmemory.c         |  3 ++-
 odb/source-loose.c            | 16 +++++++++++-----
 odb/source-packed.c           | 34 ++++++++++++++++++++++++++--------
 odb/source.h                  | 16 +++++++++++++---
 packfile.c                    |  2 +-
 t/unit-tests/u-odb-inmemory.c |  4 ++--
 9 files changed, 68 insertions(+), 29 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 caf1d0f542..6cb0a9534b 100644
--- a/odb.c
+++ b/odb.c
@@ -560,7 +560,7 @@ static int do_oid_object_info_extended(struct object_database *odb,
 	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 int do_oid_object_info_extended(struct object_database *odb,
 		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 int do_oid_object_info_extended(struct object_database *odb,
 		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 1124a18091..4727670e4d 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 int 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);
 	int 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 < 0 ? NULL : errmsg);
 	if (!ret_loose)
 		return 0;
 
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 57183daf4d..a14d6daeda 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 int 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 e786560ad1..3cee012a6d 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,6 +192,10 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 	ret = 0;
 
 out:
+	if (ret < 0 && errmsg)
+		strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"),
+			    oid_to_hex(oid), path);
+
 	if (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
 		die(_("loose object %s (stored in %s) is corrupt"),
 		    oid_to_hex(oid), path);
@@ -216,7 +221,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 static int 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 +237,7 @@ static int odb_source_loose_read_object_info(struct odb_source *source,
 		return 1;
 
 	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 +434,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))
+		if (read_object_info_from_path(data->loose, path, oid, &oi, 0, NULL))
 			return -1;
 
 		return data->cb(oid, &oi, data->cb_data);
@@ -446,7 +452,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))
+						oid, &oi, 0, NULL))
 			return -1;
 
 		return data->cb(oid, &oi, data->cb_data);
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 50e9be3b4c..bcd040aeb6 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 int 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;
@@ -60,25 +64,39 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
 		 * corresponding pack entries are skipped. Report the object
 		 * as corrupt instead of as missing in that case.
 		 */
-		if (bad_pack)
-			return -1;
-		return 1;
+		if (bad_pack) {
+			ret = -1;
+			goto out;
+		}
+
+		ret = 1;
+		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 (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 4ae6cc160e..2b39f06166 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;
 
 /*
@@ -121,11 +122,16 @@ struct odb_source {
 	 *   - A negative value in case the object exists in this source, but
 	 *     reading its object info has failed, for example because its
 	 *     on-disk state is corrupt.
+	 *
+	 * In case reading the object has failed and `errmsg` is non-NULL, the
+	 * callback is expected to populate it with a human-readable message
+	 * that describes the failure.
 	 */
 	int (*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
@@ -352,13 +358,17 @@ static inline void odb_source_prepare(struct odb_source *source,
  * Returns 0 on success, a positive value in case the object is missing in the
  * source and a negative value in case the object exists, but reading it has
  * failed.
+ *
+ * In case reading the object has failed and `errmsg` is non-NULL it will be
+ * populated with a human-readable message that describes the failure.
  */
 static inline int 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 93b3f38dab..102fc8db2f 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(odb_source_read_object_info(&source->base, &oid, NULL, 0) > 0);
+	cl_assert(odb_source_read_object_info(&source->base, &oid, NULL, 0, NULL) > 0);
 
 	odb_source_free(&source->base);
 }

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 7/7] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically

[Patrick Steinhardt]

When a lookup with `OBJECT_INFO_DIE_IF_CORRUPT` fails we want to die in
case the object exists, but cannot be read. This flag is handled in two
different spots right now:

  - `do_oid_object_info_extended()` calls `has_packed_and_bad()` to
    check whether the object is known to be corrupt in any packfile.
    This function reaches into the internals of the packed source and
    thus breaks the abstraction provided by our object sources.

  - The loose source handles the flag itself and dies directly in
    `read_object_info_from_path()`, which means that we die even in
    cases where another source may still have a good copy of the
    object.

Besides being inconsistent, it also ties us to the specific backend used
by the database sources because `has_packed_and_bad()` assumes that they
use the "files" backend. Any other backend will instead cause us to die
when calling `odb_source_files_downcast()`, even if the object was
simply nonexistent.

In the preceding commits we've carved out the infrastructure to make
this mechanism fully generic. On the one hand, all backends now tell us
whether the object is missing or corrupt via their return values. And
on the other hand, they have been tought to provide a readable error
message to the caller.

Adapt `do_oid_object_info_extended()` to use those new mechanisms. This
means that we won't die immediately anymore when a loose object is
corrupt, and we properly handle backends other than the "files" backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c                        | 46 ++++++++++++++++++++++++++++++--------------
 odb/source-loose.c           |  5 -----
 packfile.c                   | 17 ----------------
 packfile.h                   |  1 -
 t/t1060-object-corruption.sh | 18 +++++++++++++++++
 5 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/odb.c b/odb.c
index 6cb0a9534b..206988f39b 100644
--- a/odb.c
+++ b/odb.c
@@ -15,7 +15,6 @@
 #include "object-name.h"
 #include "odb.h"
 #include "odb/source-inmemory.h"
-#include "packfile.h"
 #include "path.h"
 #include "promisor-remote.h"
 #include "quote.h"
@@ -551,8 +550,11 @@ static int do_oid_object_info_extended(struct object_database *odb,
 				       const struct object_id *oid,
 				       struct object_info *oi, unsigned flags)
 {
+	struct strbuf corrupt_err = STRBUF_INIT;
 	const struct object_id *real = oid;
 	int already_retried = 0;
+	bool corrupt = false;
+	int ret;
 
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
 		real = lookup_replace_object(odb->repo, oid);
@@ -568,9 +570,14 @@ static int do_oid_object_info_extended(struct object_database *odb,
 	while (1) {
 		struct odb_source *source;
 
-		for (source = odb->sources; source; source = source->next)
-			if (!odb_source_read_object_info(source, real, oi, flags, NULL))
-				return 0;
+		for (source = odb->sources; source; source = source->next) {
+			ret = odb_source_read_object_info(source, real, oi, flags,
+							  corrupt_err.len ? NULL : &corrupt_err);
+			if (!ret)
+				goto out;
+			if (ret < 0)
+				corrupt = true;
+		}
 
 		/*
 		 * When the object hasn't been found we try a second read and
@@ -578,11 +585,15 @@ static int do_oid_object_info_extended(struct object_database *odb,
 		 * caches or reload on-disk state.
 		 */
 		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,
-								 NULL))
-					return 0;
+			for (source = odb->sources; source; source = source->next) {
+				ret = odb_source_read_object_info(source, real, oi,
+								  flags | OBJECT_INFO_SECOND_READ,
+								  corrupt_err.len ? NULL : &corrupt_err);
+				if (!ret)
+					goto out;
+				if (ret < 0)
+					corrupt = true;
+			}
 		}
 
 		/*
@@ -605,16 +616,23 @@ static int do_oid_object_info_extended(struct object_database *odb,
 		}
 
 		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
-			const struct packed_git *p;
 			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
 				die(_("replacement %s not found for %s"),
 				    oid_to_hex(real), oid_to_hex(oid));
-			if ((p = has_packed_and_bad(odb->repo, real)))
-				die(_("packed object %s (stored in %s) is corrupt"),
-				    oid_to_hex(real), p->pack_name);
+			if (corrupt) {
+				if (corrupt_err.len)
+					die("%s", corrupt_err.buf);
+				die(_("object %s is corrupt"), oid_to_hex(real));
+			}
 		}
-		return -1;
+
+		ret = -1;
+		goto out;
 	}
+
+out:
+	strbuf_release(&corrupt_err);
+	return ret;
 }
 
 static int oid_object_info_convert(struct repository *r,
diff --git a/odb/source-loose.c b/odb/source-loose.c
index 3cee012a6d..8ca5a78858 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -195,11 +195,6 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 	if (ret < 0 && errmsg)
 		strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"),
 			    oid_to_hex(oid), path);
-
-	if (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
-		die(_("loose object %s (stored in %s) is corrupt"),
-		    oid_to_hex(oid), path);
-
 	if (stream_to_end)
 		git_inflate_end(stream_to_end);
 	if (map)
diff --git a/packfile.c b/packfile.c
index 3cde39a01c..cd38be088d 100644
--- a/packfile.c
+++ b/packfile.c
@@ -985,23 +985,6 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)
 	oidset_insert(&p->bad_objects, oid);
 }
 
-const struct packed_git *has_packed_and_bad(struct repository *r,
-					    const struct object_id *oid)
-{
-	struct odb_source *source;
-
-	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct packfile_list_entry *e;
-
-		for (e = files->packed->packs.head; e; e = e->next)
-			if (oidset_contains(&e->pack->bad_objects, oid))
-				return e->pack;
-	}
-
-	return NULL;
-}
-
 off_t get_delta_base(struct packed_git *p,
 		     struct pack_window **w_curs,
 		     off_t *curpos,
diff --git a/packfile.h b/packfile.h
index 3229a6ed47..573fe003d0 100644
--- a/packfile.h
+++ b/packfile.h
@@ -329,7 +329,6 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source,
 				      uint32_t *maybe_index_pos, struct object_info *oi);
 
 void mark_bad_packed_object(struct packed_git *, const struct object_id *);
-const struct packed_git *has_packed_and_bad(struct repository *, const struct object_id *);
 
 int has_object_pack(struct repository *r, const struct object_id *oid);
 int has_object_kept_pack(struct repository *r, const struct object_id *oid,
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index 502a5ea1c5..d2ef468b45 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -145,4 +145,22 @@ test_expect_success 'partial clone of corrupted repository' '
 	test_must_fail git -C corrupt-partial checkout --force
 '
 
+test_expect_success 'corrupted loose commit can be read from alternate' '
+	git init repo-a &&
+	tree=$(git -C repo-a write-tree) &&
+	commit=$(git -C repo-a commit-tree $tree </dev/null) &&
+
+	cp -r repo-a repo-b &&
+	(
+		cd repo-b &&
+		echo ../../../repo-a/.git/objects >.git/objects/info/alternates &&
+		corrupt_byte "$commit" 1
+	) &&
+
+	git -C repo-a cat-file -p "$commit" >expect &&
+	git -C repo-b cat-file -p "$commit" >actual 2>err &&
+	test_cmp expect actual &&
+	test_grep "inflate: data stream error" err
+'
+
 test_done

-- 
2.55.0.822.g20453c30eb.dirty

Re: [PATCH 1/7] odb/source: discern missing and corrupt objects

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The `read_object_info()` callback of `struct odb_source` is documented
> to return a negative error code in case reading the object has failed,
> and zero otherwise. This is overly broad though, as there are two very
> different kinds of failures:
>
>   - The object may not exist in the source at all.
>
>   - The object exists, but reading it has failed, for example because
>     its on-disk state is corrupt.
>
> This distinction matters to callers: when an object is corrupt in one
> source we may still find a good copy of it in another source, so we may
> still be able to proceed with a given operation.
>
> The "packed" source already distinguishes these cases by returning a
> positive value for missing objects and a negative value in case reading
> the object has failed. But all the other sources conflate them into a
> single negative return value.

In other words, "packed" did not honor the documented contract with
the callers and nobody noticed?  It gives us a usable escape hatch ;-)

Do we need to support many other "it is an error but we treat as non
error in some context" values, like the "does not exist"?  If so, it
does make sense to say 0 is absolute success, positive values are
such half-errors, and negative values are absolute failures.  If
not, it would have been much nicer if "you asked me about this
information but there is no such object" were still signalled as an
error (i.e., negative return value) that is distinct from other
kinds of errors like I/O error (which also should be signalled by a
negative return value), instead of a positive value whose meanings
were not defined, though.

> Adapt the documentation to explicitly require the semantics of the
> "packed" backend, where we return a positive value for missing objects
> and a negative value for corrupt ones. Subsequent commits will adapt all
> the other implementations to respect those new semantics.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source.h | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/odb/source.h b/odb/source.h
> index d69f8e2d1c..4ae6cc160e 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -110,8 +110,17 @@ struct odb_source {
>  	 *     second read in case they know that the first read would have
>  	 *     already surfaced the object without reloading any on-disk state.
>  	 *
> -	 * The callback is expected to return a negative error code in case
> -	 * reading the object has failed, 0 otherwise.
> +	 * The callback is expected to return one of the following values:
> +	 *
> +	 *   - Zero in case the object has been found and its object info has
> +	 *     been read successfully.
> +	 *
> +	 *   - A positive value in case the object does not exist in this
> +	 *     source.
> +	 *
> +	 *   - A negative value in case the object exists in this source, but
> +	 *     reading its object info has failed, for example because its
> +	 *     on-disk state is corrupt.
>  	 */
>  	int (*read_object_info)(struct odb_source *source,
>  				const struct object_id *oid,
> @@ -340,7 +349,9 @@ static inline void odb_source_prepare(struct odb_source *source,
>  
>  /*
>   * Read an object from the object database source identified by its object ID.
> - * Returns 0 on success, a negative error code otherwise.
> + * Returns 0 on success, a positive value in case the object is missing in the
> + * source and a negative value in case the object exists, but reading it has
> + * failed.
>   */
>  static inline int odb_source_read_object_info(struct odb_source *source,
>  					      const struct object_id *oid,

Re: [PATCH 2/7] odb/source-inmemory: signal missing objects via positive return

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The in-memory source returns a negative value from its
> `read_object_info()` callback when the object in question does not
> exist. Adapt the callback to return a positive value for missing objects
> according to the new calling convention.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-inmemory.c         | 2 +-
>  t/unit-tests/u-odb-inmemory.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
> index 3e71611b8e..57183daf4d 100644
> --- a/odb/source-inmemory.c
> +++ b/odb/source-inmemory.c
> @@ -66,7 +66,7 @@ static int odb_source_inmemory_read_object_info(struct odb_source *source,
>  
>  	object = find_cached_object(inmemory, oid);
>  	if (!object)
> -		return -1;
> +		return 1;

Let's not define "any positive value means this single thing: it
does not exist" and then return a mysterious and unspecified hard
coded constant like this.  Instead perhaps something along this
line?

    enum odb_roi_status {
	ODB_ROI_SUCCESS = 0,
	ODB_ROI_MISSING = 1,
	ODB_ROI_IO_ERROR = -1,
	...
    };

As I already said, I personally prefer to define MISSING also as
a negative value.

Re: [PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When reading a packed object that doesn't verify we mark it as bad and
> indicate to the caller that we failed reading the object despite the
> fact that it supposedly exists. This matches the semantics we have now
> established in a preceding commit, where we discern failure to read a
> corrupt object from a missing object.
>
> What doesn't work yet though is when a call tries to read an object that
> has already been marked as corrupt in a previous call. In that case,
> `find_pack_entry()` will tell us that the object in question does not
> exist, and consequently we'll not flag the object as corrupt but as
> missing.

Thanks for attacking this one.  I've always felt it awkward that we
treat a corrupt/unreadable object as if we do not have it, and we
even silently recover from it if we have another copy, making fsck
practically the only thing that notices such breakages.

>  int fill_midx_entry(struct multi_pack_index *m,
>  		    const struct object_id *oid,
> -		    struct pack_entry *e)
> +		    struct pack_entry *e,
> +		    struct packed_git **bad_pack)
>  {
>  	uint32_t pos;
>  	uint32_t pack_int_id;
> @@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,
>  		return 0;
>  
>  	if (oidset_size(&p->bad_objects) &&
> -	    oidset_contains(&p->bad_objects, oid))
> +	    oidset_contains(&p->bad_objects, oid)) {
> +		if (bad_pack && !*bad_pack)
> +			*bad_pack = p;
>  		return 0;
> +	}

Hmph, so the idea is that if you have even one bad thing, you are
marked as bad, because who knows what other parts of you are broken?

Re: [PATCH 4/7] odb/source-loose: distinguish missing and corrupt objects

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> @@ -91,11 +91,16 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
>  		struct stat st;
>  
>  		if ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) {
> -			ret = quick_has_loose(loose, oid) ? 0 : -1;
> +			ret = quick_has_loose(loose, oid) ? 0 : 1;
>  			goto out;
>  		}
>  
>  		if (lstat(path, &st) < 0) {
> +			if (errno == ENOENT) {
> +				ret = 1;
> +				goto out;
> +			}
> +
>  			ret = -1;
>  			goto out;

Exactly the same comment about "turn it into an enum with meaningful
names once you add to an yes/no set a third choice" applies here.

> @@ -155,7 +163,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
>  
>  		if (parse_loose_header(hdr, oi) < 0) {
>  			ret = error(_("unable to parse %s header"), oid_to_hex(oid));
> -			goto corrupt;
> +			goto out;
>  		}
>  
>  		if (*oi->typep < 0)
> @@ -165,7 +173,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
>  			*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
>  			if (!*oi->contentp) {
>  				ret = -1;
> -				goto corrupt;
> +				goto out;
>  			}
>  		}
>  
> @@ -173,21 +181,20 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
>  	case ULHR_BAD:
>  		ret = error(_("unable to unpack %s header"),
>  			    oid_to_hex(oid));
> -		goto corrupt;
> +		goto out;
>  	case ULHR_TOO_LONG:
>  		ret = error(_("header for %s too long, exceeds %d bytes"),
>  			    oid_to_hex(oid), MAX_HEADER_LEN);
> -		goto corrupt;
> +		goto out;
>  	}
>  
>  	ret = 0;
>  
> -corrupt:
> -	if (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
> +out:
> +	if (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
>  		die(_("loose object %s (stored in %s) is corrupt"),
>  		    oid_to_hex(oid), path);

A missing object is not necessarily repository corruption, and the
code path to deal with it needs to jump here, so naming the label
"out:" is more appropriate.  OK.

Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Subject: Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return

"missing" is what you meant intead of "mark".

Re: [PATCH 2/7] odb/source-inmemory: signal missing objects via positive return

[Patrick Steinhardt]

On Tue, Aug 18, 2026 at 11:05:33AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The in-memory source returns a negative value from its
> > `read_object_info()` callback when the object in question does not
> > exist. Adapt the callback to return a positive value for missing objects
> > according to the new calling convention.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  odb/source-inmemory.c         | 2 +-
> >  t/unit-tests/u-odb-inmemory.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
> > index 3e71611b8e..57183daf4d 100644
> > --- a/odb/source-inmemory.c
> > +++ b/odb/source-inmemory.c
> > @@ -66,7 +66,7 @@ static int odb_source_inmemory_read_object_info(struct odb_source *source,
> >  
> >  	object = find_cached_object(inmemory, oid);
> >  	if (!object)
> > -		return -1;
> > +		return 1;
> 
> Let's not define "any positive value means this single thing: it
> does not exist" and then return a mysterious and unspecified hard
> coded constant like this.  Instead perhaps something along this
> line?
> 
>     enum odb_roi_status {
> 	ODB_ROI_SUCCESS = 0,
> 	ODB_ROI_MISSING = 1,
> 	ODB_ROI_IO_ERROR = -1,
> 	...
>     };
> 
> As I already said, I personally prefer to define MISSING also as
> a negative value.

Fair enough, will adapt.

Patrick

Re: [PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing

[Patrick Steinhardt]

On Tue, Aug 18, 2026 at 11:17:47AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > @@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,
> >  		return 0;
> >  
> >  	if (oidset_size(&p->bad_objects) &&
> > -	    oidset_contains(&p->bad_objects, oid))
> > +	    oidset_contains(&p->bad_objects, oid)) {
> > +		if (bad_pack && !*bad_pack)
> > +			*bad_pack = p;
> >  		return 0;
> > +	}
> 
> Hmph, so the idea is that if you have even one bad thing, you are
> marked as bad, because who knows what other parts of you are broken?

No, not quite. We don't mark the whole pack itself as bad, we only mark
the objects that's contained in there as bad. The only reason why we
also bubble up the pack is so that we can provide a better error message
in a subsequent commit, where we can then tell the user which pack it
was specifically that contains the bad commit.

That's by itself not visible in this commit yet, but I do mention it as
part of the commit message.

Patrick

Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return

[Patrick Steinhardt]

On Tue, Aug 18, 2026 at 11:58:33AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Subject: Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return
> 
> "missing" is what you meant intead of "mark".

D'oh, obviously. I've massaged this specific subject probably half a
dozen times because I couldn't find a nice summary, and this here is the
result. Will fix.

Patrick

Re: [PATCH 1/7] odb/source: discern missing and corrupt objects

[Patrick Steinhardt]

On Tue, Aug 18, 2026 at 11:00:40AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The `read_object_info()` callback of `struct odb_source` is documented
> > to return a negative error code in case reading the object has failed,
> > and zero otherwise. This is overly broad though, as there are two very
> > different kinds of failures:
> >
> >   - The object may not exist in the source at all.
> >
> >   - The object exists, but reading it has failed, for example because
> >     its on-disk state is corrupt.
> >
> > This distinction matters to callers: when an object is corrupt in one
> > source we may still find a good copy of it in another source, so we may
> > still be able to proceed with a given operation.
> >
> > The "packed" source already distinguishes these cases by returning a
> > positive value for missing objects and a negative value in case reading
> > the object has failed. But all the other sources conflate them into a
> > single negative return value.
> 
> In other words, "packed" did not honor the documented contract with
> the callers and nobody noticed?  It gives us a usable escape hatch ;-)

Yes, kind of. It didn't matter much though, as the "files" backend
knew to translate the positive value into a negative one.

> Do we need to support many other "it is an error but we treat as non
> error in some context" values, like the "does not exist"?  If so, it
> does make sense to say 0 is absolute success, positive values are
> such half-errors, and negative values are absolute failures.  If
> not, it would have been much nicer if "you asked me about this
> information but there is no such object" were still signalled as an
> error (i.e., negative return value) that is distinct from other
> kinds of errors like I/O error (which also should be signalled by a
> negative return value), instead of a positive value whose meanings
> were not defined, though.

I cannot think of any other classes of errors where we'd want to fail
gracefully from the top of my head. The only one that's potentially
worth thinking about is in case an object disappears right while we are
looking at it. But that's basically just another edge case of a missing
object.

In any case, I think I'm aligned with the proposal to turn this into a
proper enum and then use negative values exclusively. Thanks!

Patrick

[PATCH v2 0/5] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically

[Patrick Steinhardt]

Hi,

when looking up an object with `OBJECT_INFO_DIE_IF_CORRUPT` fails we
want to die in case the object exists but is corrupted. This flag is
handled in two different spots right now:

  - `do_oid_object_info_extended()` calls `has_packed_and_bad()` to
    check whether the object is known to be corrupt in any packfile.
    This function reaches into the internals of the packed source and
    thus breaks the abstraction provided by our object sources.

  - The loose source handles the flag itself and dies directly in
    `read_object_info_from_path()`, which means that we die even in
    cases where another source may still have a good copy of the
    object.

Besides being inconsistent, it also ties us to the specific backend used
by the database sources because `has_packed_and_bad()` assumes that they
use the "files" backend. Any other backend will instead cause us to die
when calling `odb_source_files_downcast()`, even if the object was
simply nonexistent.

This series fixes these issues and makes the check backend-agnostic by
extending semantics of `odb_source_read_object_info()`: on the one hand
it now distinguishes whether an object is missing or corrput, and on the
other hand it starts to return an error message to the caller.

Changes in v2:
  - Adapt the series to use an `enum odb_read_status` with negative
    error codes exclusively, as suggested by Junio. This results in a
    rather big restructure of the series.
  - Link to v1: https://patch.msgid.link/20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (5):
      odb/source-packed: flag known-bad objects as corrupt and not missing
      odb/source: introduce error status when reading objects
      odb/source: let callers discern missing and corrupt objects
      odb/source: allow `read_object_info()` to bubble up error messages
      odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically

 builtin/pack-objects.c        |  8 +++---
 midx.c                        | 10 ++++---
 midx.h                        |  3 ++-
 odb.c                         | 63 ++++++++++++++++++++++++++++---------------
 odb.h                         | 17 +++++++++---
 odb/source-files.c            | 31 ++++++++++++++++-----
 odb/source-inmemory.c         | 11 ++++----
 odb/source-loose.c            | 52 ++++++++++++++++++++---------------
 odb/source-packed.c           | 58 ++++++++++++++++++++++++++++-----------
 odb/source.h                  | 34 ++++++++++++++---------
 packfile.c                    | 29 ++++++--------------
 packfile.h                    |  4 +--
 t/helper/test-read-midx.c     |  2 +-
 t/t1060-object-corruption.sh  | 18 +++++++++++++
 t/unit-tests/u-odb-inmemory.c |  5 ++--
 15 files changed, 224 insertions(+), 121 deletions(-)

Range-diff versus v1:

1:  ea7d64242a < -:  ---------- odb/source: discern missing and corrupt objects
2:  f4944112b3 < -:  ---------- odb/source-inmemory: signal missing objects via positive return
3:  9080d7f138 ! 1:  c821c3b004 odb/source-packed: flag known-bad objects as corrupt and not missing
    @@ Metadata
      ## Commit message ##
         odb/source-packed: flag known-bad objects as corrupt and not missing
     
    -    When reading a packed object that doesn't verify we mark it as bad and
    -    indicate to the caller that we failed reading the object despite the
    -    fact that it supposedly exists. This matches the semantics we have now
    -    established in a preceding commit, where we discern failure to read a
    -    corrupt object from a missing object.
    +    When reading packed objects we know to tell apart missing objects and
    +    corrupt objects by returning a positive error code in the former case,
    +    and a negative one in the latter case. We do that by distinguishing
    +    between errors returned by `find_pack_entry()`, which yields the offset
    +    of the object, and `packed_object_info()`, which reads the object
    +    contents.
     
    -    What doesn't work yet though is when a call tries to read an object that
    -    has already been marked as corrupt in a previous call. In that case,
    -    `find_pack_entry()` will tell us that the object in question does not
    -    exist, and consequently we'll not flag the object as corrupt but as
    -    missing.
    +    But even though we already distinguish those cases when reading packed
    +    objects, the logic is broken in case a caller tries to read an object
    +    that has been marked as corrupt. In that case, `find_pack_entry()` will
    +    tell us that the object in question does not exist, and consequently
    +    we'll not flag the object as corrupt but as missing.
     
         Fix this issue by bubbling up whether the object is corrupt and, if so,
    -    which packfile contains the corrupted object. We don't yet need the
    -    latter information about the specific packfile, so we could've just as
    -    well made this a `bool *corrupted` pointer. But we'll need information
    -    about the containing packfile in a subsequent commit.
    +    which packfile contains the corrupted object.
    +
    +    Note that we don't yet need the information about the specific packfile,
    +    so we could've just as well made this a `bool *corrupted` pointer. But
    +    we'll need information about the containing packfile in a subsequent
    +    commit so that we can generate a proper error message telling the user
    +    which packfile contains the broken object.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ odb/source-packed.c: static int odb_source_packed_read_object_info(struct odb_so
     -	if (!find_pack_entry(packed, oid, &e))
     +	if (!find_pack_entry(packed, oid, &e, &bad_pack)) {
     +		/*
    -+		 * The lookup may have failed because the object is known to
    -+		 * be corrupt in one of our packfiles, in which case the
    -+		 * corresponding pack entries are skipped. Report the object
    -+		 * as corrupt instead of as missing in that case.
    ++		 * The lookup may have failed because the object is known to be
    ++		 * corrupt in one of the packfiles. Report the object as
    ++		 * corrupt instead of missing in that case.
     +		 */
     +		if (bad_pack)
     +			return -1;
-:  ---------- > 2:  602249a58e odb/source: introduce error status when reading objects
4:  db2bd77c61 ! 3:  269fb8e6a6 odb/source-loose: distinguish missing and corrupt objects
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    odb/source-loose: distinguish missing and corrupt objects
    +    odb/source: let callers discern missing and corrupt objects
     
    -    The loose source returns a negative value from its `read_object_info()`
    -    callback both when the object is missing and when the object exists but
    -    cannot be read. Consequently, callers cannot tell apart whether the
    -    object does not exist in this source at all or whether it is corrupt.
    +    As explained in the preceding commits, reading objects can either fail
    +    because the object truly does not exist or because it exists, but its
    +    data is corrupt. Some callers do care about this distinction, but there
    +    is no way to tell these two cases apart right now.
     
    -    Adapt the code to return a positive value for missing objects according
    -    to the new calling convention.
    +    Introduce a new `ODB_READ_NOT_FOUND` value that ought to be returned by
    +    the backends in case the object truly does not exist and adapt backends
    +    to use it.
     
    -    This also allows us to get rid of the separate `corrupt:` label, as we
    -    can now clearly distinguish between corrupt and missing objects in the
    -    function ourselves. This makes us handle failures to read loose objects
    -    more consistently, as not all failure cases were jumping that label.
    -
    -    Note that there's one call to `die()` when the object type is invalid
    -    that should arguably be converted to an error, too. But adapting that
    -    call results in quite a lot of broken tests, so this is left as-is for
    -    now.
    +    Note that we don't yet return this error from `odb_read_object_info()`
    +    itself. This will be fixed in a subsequent commit.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    + ## odb.h ##
    +@@ odb.h: enum odb_read_status {
    + 	ODB_READ_OK = 0,
    + 	/* The read resulted in a generic error. */
    + 	ODB_READ_ERROR = -1,
    ++	/* The object could not be found. */
    ++	ODB_READ_NOT_FOUND = -2,
    + };
    + 
    + /*
    +
    + ## odb/source-files.c ##
    +@@ odb/source-files.c: static enum odb_read_status odb_source_files_read_object_info(struct odb_source
    + 							      enum object_info_flags flags)
    + {
    + 	struct odb_source_files *files = odb_source_files_downcast(source);
    ++	enum odb_read_status ret_packed, ret_loose;
    + 
    +-	if (!odb_source_read_object_info(&files->packed->base, oid, oi, flags) ||
    +-	    !odb_source_read_object_info(&files->loose->base, oid, oi, flags))
    ++	ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags);
    ++	if (!ret_packed)
    + 		return 0;
    + 
    +-	return -1;
    ++	ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags);
    ++	if (!ret_loose)
    ++		return 0;
    ++
    ++	/*
    ++	 * Reading the packed object may have failed even though the object
    ++	 * exists, for example because it is corrupt. Report this failure to
    ++	 * the caller in case neither of the sources was able to read the
    ++	 * object, and prefer the error of the packed source in case both
    ++	 * reads have failed.
    ++	 */
    ++	if (ret_packed != ODB_READ_NOT_FOUND)
    ++		return ret_packed;
    ++	return ret_loose;
    + }
    + 
    + static int odb_source_files_read_object_stream(struct odb_read_stream **out,
    +
    + ## odb/source-inmemory.c ##
    +@@ odb/source-inmemory.c: static enum odb_read_status odb_source_inmemory_read_object_info(struct odb_sour
    + 
    + 	object = find_cached_object(inmemory, oid);
    + 	if (!object)
    +-		return -1;
    ++		return ODB_READ_NOT_FOUND;
    + 
    + 	populate_object_info(inmemory, oi, object);
    + 	return 0;
    +
      ## odb/source-loose.c ##
     @@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loose *loose,
      		struct stat st;
      
      		if ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) {
     -			ret = quick_has_loose(loose, oid) ? 0 : -1;
    -+			ret = quick_has_loose(loose, oid) ? 0 : 1;
    ++			ret = quick_has_loose(loose, oid) ? 0 : ODB_READ_NOT_FOUND;
      			goto out;
      		}
      
      		if (lstat(path, &st) < 0) {
     +			if (errno == ENOENT) {
    -+				ret = 1;
    ++				ret = ODB_READ_NOT_FOUND;
     +				goto out;
     +			}
     +
    @@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loos
     -			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
     -		ret = -1;
     +		if (errno == ENOENT) {
    -+			ret = 1;
    ++			ret = ODB_READ_NOT_FOUND;
     +			goto out;
     +		}
     +
    @@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loos
     -corrupt:
     -	if (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
     +out:
    -+	if (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
    ++	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);
      
    @@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loos
      	if (stream_to_end)
      		git_inflate_end(stream_to_end);
      	if (map)
    -@@ odb/source-loose.c: static int odb_source_loose_read_object_info(struct odb_source *source,
    +@@ odb/source-loose.c: static enum odb_read_status odb_source_loose_read_object_info(struct odb_source
      	 * second time.
      	 */
      	if (flags & OBJECT_INFO_SECOND_READ)
     -		return -1;
    -+		return 1;
    ++		return ODB_READ_NOT_FOUND;
      
      	odb_loose_path(loose, &buf, oid);
      	return read_object_info_from_path(loose, buf.buf, oid, oi, flags);
    -@@ odb/source-loose.c: 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))
    +
    + ## odb/source-packed.c ##
    +@@ odb/source-packed.c: static enum odb_read_status odb_source_packed_read_object_info(struct odb_source
    + 		 */
    + 		if (bad_pack)
      			return -1;
    +-		return 1;
    ++		return ODB_READ_NOT_FOUND;
    + 	}
      
    - 		return data->cb(oid, &oi, data->cb_data);
    -@@ odb/source-loose.c: static int for_each_prefixed_object_wrapper_cb(const struct object_id *oid,
    - 		struct object_info oi = *data->request;
    + 	/*
    +
    + ## t/unit-tests/u-odb-inmemory.c ##
    +@@ t/unit-tests/u-odb-inmemory.c: void test_odb_inmemory__read_missing_object(void)
    + 	const char *end;
      
    - 		if (odb_source_read_object_info(&data->loose->base,
    --						oid, &oi, 0) < 0)
    -+						oid, &oi, 0))
    - 			return -1;
    + 	cl_must_pass(parse_oid_hex_algop(RANDOM_OID, &oid, &end, repo.hash_algo));
    +-	cl_must_fail(odb_source_read_object_info(&source->base, &oid, NULL, 0));
    ++	cl_assert_equal_i(odb_source_read_object_info(&source->base, &oid, NULL, 0),
    ++			  ODB_READ_NOT_FOUND);
      
    - 		return data->cb(oid, &oi, data->cb_data);
    + 	odb_source_free(&source->base);
    + }
5:  0c9be02ab4 < -:  ---------- odb/source-files: signal mark objects via positive return
6:  f633262bd4 ! 4:  7de629151d odb/source: allow `read_object_info()` to bubble up error messages
    @@ builtin/pack-objects.c: static int force_object_loose(struct odb_source *source,
      
     
      ## odb.c ##
    -@@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
    +@@ odb.c: static enum odb_read_status do_oid_object_info_extended(struct object_database *
      	if (is_null_oid(real))
      		return -1;
      
    @@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
      		return 0;
      
      	odb_prepare_alternates(odb);
    -@@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
    +@@ odb.c: 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)
    @@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
      				return 0;
      
      		/*
    -@@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
    +@@ odb.c: 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,
    @@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
     
      ## odb/source-files.c ##
     @@ odb/source-files.c: static void odb_source_files_prepare(struct odb_source *source,
    - static int 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)
    + 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);
    - 	int ret_packed, ret_loose;
    + 	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,
    @@ odb/source-files.c: static void odb_source_files_prepare(struct odb_source *sour
      		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 < 0 ? NULL : errmsg);
    ++	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;
      
     
      ## odb/source-inmemory.c ##
     @@ odb/source-inmemory.c: static void populate_object_info(struct odb_source_inmemory *source,
    - static int 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)
    + 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;
    @@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loos
      	ret = 0;
      
      out:
    -+	if (ret < 0 && errmsg)
    -+		strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"),
    +-	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 (ret < 0 && (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);
     @@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loose *loose,
    - static int 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)
    + 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;
    -@@ odb/source-loose.c: static int odb_source_loose_read_object_info(struct odb_source *source,
    - 		return 1;
    +@@ odb/source-loose.c: 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);
    @@ odb/source-loose.c: static int for_each_object_wrapper_cb(const struct object_id
      	if (data->request) {
      		struct object_info oi = *data->request;
      
    --		if (read_object_info_from_path(data->loose, path, oid, &oi, 0))
    -+		if (read_object_info_from_path(data->loose, path, oid, &oi, 0, NULL))
    +-		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);
    @@ odb/source-loose.c: static int for_each_prefixed_object_wrapper_cb(const struct
      		struct object_info oi = *data->request;
      
      		if (odb_source_read_object_info(&data->loose->base,
    --						oid, &oi, 0))
    -+						oid, &oi, 0, NULL))
    +-						oid, &oi, 0) < 0)
    ++						oid, &oi, 0, NULL) < 0)
      			return -1;
      
      		return data->cb(oid, &oi, data->cb_data);
    @@ odb/source-packed.c
      static int find_pack_entry(struct odb_source_packed *store,
      			   const struct object_id *oid,
     @@ odb/source-packed.c: static int find_pack_entry(struct odb_source_packed *store,
    - static int 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)
    + 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;
    -@@ odb/source-packed.c: static int odb_source_packed_read_object_info(struct odb_source *source,
    - 		 * corresponding pack entries are skipped. Report the object
    - 		 * as corrupt instead of as missing in that case.
    +@@ odb/source-packed.c: 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 1;
    +-		return ODB_READ_NOT_FOUND;
     +		if (bad_pack) {
     +			ret = -1;
     +			goto out;
     +		}
     +
    -+		ret = 1;
    ++		ret = ODB_READ_NOT_FOUND;
     +		goto out;
      	}
      
    @@ odb/source-packed.c: static int odb_source_packed_read_object_info(struct odb_so
     +	ret = 0;
     +
     +out:
    -+	if (bad_pack && errmsg)
    ++	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);
     +
    @@ odb/source.h: enum odb_source_type {
      
      /*
     @@ odb/source.h: struct odb_source {
    - 	 *   - A negative value in case the object exists in this source, but
    - 	 *     reading its object info has failed, for example because its
    - 	 *     on-disk state is corrupt.
    -+	 *
    -+	 * In case reading the object has failed and `errmsg` is non-NULL, the
    -+	 * callback is expected to populate it with a human-readable message
    -+	 * that describes the failure.
    + 	 *     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.
      	 */
    - 	int (*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);
    + 	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
     @@ odb/source.h: static inline void odb_source_prepare(struct odb_source *source,
    -  * Returns 0 on success, a positive value in case the object is missing in the
    -  * source and a negative value in case the object exists, but reading it has
    -  * failed.
    + /*
    +  * 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 and `errmsg` is non-NULL it will be
    -+ * populated with a human-readable message that describes the failure.
    ++ * 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 int 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)
    + 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);
    @@ t/unit-tests/u-odb-inmemory.c: 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(odb_source_read_object_info(&source->base, &oid, NULL, 0) > 0);
    -+	cl_assert(odb_source_read_object_info(&source->base, &oid, NULL, 0, NULL) > 0);
    +-	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);
    - }
7:  a82f4341e1 ! 5:  4af62fe3bf odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically
    @@ Commit message
         In the preceding commits we've carved out the infrastructure to make
         this mechanism fully generic. On the one hand, all backends now tell us
         whether the object is missing or corrupt via their return values. And
    -    on the other hand, they have been tought to provide a readable error
    +    on the other hand, they have been taught to provide a readable error
         message to the caller.
     
         Adapt `do_oid_object_info_extended()` to use those new mechanisms. This
    @@ odb.c
      #include "path.h"
      #include "promisor-remote.h"
      #include "quote.h"
    -@@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
    - 				       const struct object_id *oid,
    - 				       struct object_info *oi, unsigned flags)
    +@@ odb.c: static enum odb_read_status do_oid_object_info_extended(struct object_database *
    + 							const struct object_id *oid,
    + 							struct object_info *oi, unsigned flags)
      {
     +	struct strbuf corrupt_err = STRBUF_INIT;
      	const struct object_id *real = oid;
    ++	enum odb_read_status ret;
      	int already_retried = 0;
     +	bool corrupt = false;
    -+	int ret;
      
      	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
      		real = lookup_replace_object(odb->repo, oid);
    -@@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
    +@@ odb.c: static enum odb_read_status do_oid_object_info_extended(struct object_database *
      	while (1) {
      		struct odb_source *source;
      
    @@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
     +							  corrupt_err.len ? NULL : &corrupt_err);
     +			if (!ret)
     +				goto out;
    -+			if (ret < 0)
    ++			if (ret != ODB_READ_NOT_FOUND)
     +				corrupt = true;
     +		}
      
      		/*
      		 * When the object hasn't been found we try a second read and
    -@@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
    +@@ odb.c: static enum odb_read_status do_oid_object_info_extended(struct object_database *
      		 * caches or reload on-disk state.
      		 */
      		if (!(flags & OBJECT_INFO_QUICK)) {
    @@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
     +								  corrupt_err.len ? NULL : &corrupt_err);
     +				if (!ret)
     +					goto out;
    -+				if (ret < 0)
    ++				if (ret != ODB_READ_NOT_FOUND)
     +					corrupt = true;
     +			}
      		}
      
      		/*
    -@@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
    +@@ odb.c: static enum odb_read_status do_oid_object_info_extended(struct object_database *
      		}
      
      		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
    @@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
      		}
     -		return -1;
     +
    -+		ret = -1;
    ++		ret = corrupt ? ODB_READ_ERROR : ODB_READ_NOT_FOUND;
     +		goto out;
      	}
     +
    @@ odb.c: static int do_oid_object_info_extended(struct object_database *odb,
     
      ## odb/source-loose.c ##
     @@ odb/source-loose.c: static int read_object_info_from_path(struct odb_source_loose *loose,
    - 	if (ret < 0 && errmsg)
    - 		strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"),
    + 	ret = 0;
    + 
    + out:
    +-	if (ret && ret != ODB_READ_NOT_FOUND) {
    +-		if ((flags & OBJECT_INFO_DIE_IF_CORRUPT))
    +-			die(_("loose object %s (stored in %s) is corrupt"),
    ++	if (ret && ret != ODB_READ_NOT_FOUND && errmsg)
    ++		strbuf_addf(errmsg, _("loose object %s (stored in %s) is corrupt"),
      			    oid_to_hex(oid), path);
    --
    --	if (ret < 0 && (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);

---
base-commit: 18e66859d87fb4b76599f73460b54f0848c76b16
change-id: 20260818-pks-odb-generic-corrupt-objects-52a47d6214d9

[PATCH v2 1/5] odb/source-packed: flag known-bad objects as corrupt and not missing

[Patrick Steinhardt]

When reading packed objects we know to tell apart missing objects and
corrupt objects by returning a positive error code in the former case,
and a negative one in the latter case. We do that by distinguishing
between errors returned by `find_pack_entry()`, which yields the offset
of the object, and `packed_object_info()`, which reads the object
contents.

But even though we already distinguish those cases when reading packed
objects, the logic is broken in case a caller tries to read an object
that has been marked as corrupt. In that case, `find_pack_entry()` will
tell us that the object in question does not exist, and consequently
we'll not flag the object as corrupt but as missing.

Fix this issue by bubbling up whether the object is corrupt and, if so,
which packfile contains the corrupted object.

Note that we don't yet need the information about the specific packfile,
so we could've just as well made this a `bool *corrupted` pointer. But
we'll need information about the containing packfile in a subsequent
commit so that we can generate a proper error message telling the user
which packfile contains the broken object.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/pack-objects.c    |  2 +-
 midx.c                    | 10 +++++++---
 midx.h                    |  3 ++-
 odb/source-packed.c       | 22 ++++++++++++++++------
 packfile.c                | 10 +++++++---
 packfile.h                |  3 ++-
 t/helper/test-read-midx.c |  2 +-
 7 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1ec5b6f206..10c2471024 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
 		struct multi_pack_index *m = get_multi_pack_index(files->packed);
 		struct pack_entry e;
 
-		if (m && fill_midx_entry(m, oid, &e)) {
+		if (m && fill_midx_entry(m, oid, &e, NULL)) {
 			want = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);
 			if (want != -1)
 				return want;
diff --git a/midx.c b/midx.c
index 76c3f92cc3..37f082dbdd 100644
--- a/midx.c
+++ b/midx.c
@@ -591,7 +591,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)
 
 int fill_midx_entry(struct multi_pack_index *m,
 		    const struct object_id *oid,
-		    struct pack_entry *e)
+		    struct pack_entry *e,
+		    struct packed_git **bad_pack)
 {
 	uint32_t pos;
 	uint32_t pack_int_id;
@@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,
 		return 0;
 
 	if (oidset_size(&p->bad_objects) &&
-	    oidset_contains(&p->bad_objects, oid))
+	    oidset_contains(&p->bad_objects, oid)) {
+		if (bad_pack && !*bad_pack)
+			*bad_pack = p;
 		return 0;
+	}
 
 	e->offset = nth_midxed_offset(m, pos);
 	e->p = p;
@@ -1028,7 +1032,7 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags)
 
 		nth_midxed_object_oid(&oid, m, pairs[i].pos);
 
-		if (!fill_midx_entry(m, &oid, &e)) {
+		if (!fill_midx_entry(m, &oid, &e, NULL)) {
 			midx_report(_("failed to load pack entry for oid[%d] = %s"),
 				    pairs[i].pos, oid_to_hex(&oid));
 			continue;
diff --git a/midx.h b/midx.h
index 939c18e588..1f2f2d5321 100644
--- a/midx.h
+++ b/midx.h
@@ -117,7 +117,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos);
 struct object_id *nth_midxed_object_oid(struct object_id *oid,
 					struct multi_pack_index *m,
 					uint32_t n);
-int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid, struct pack_entry *e);
+int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid,
+		    struct pack_entry *e, struct packed_git **bad_pack);
 int midx_contains_pack(struct multi_pack_index *m,
 		       const char *idx_or_pack_name);
 int midx_layer_contains_pack(struct multi_pack_index *m,
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0890704e76..16fa4f5769 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -13,18 +13,19 @@
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
-			   struct pack_entry *e)
+			   struct pack_entry *e,
+			   struct packed_git **bad_pack)
 {
 	struct packfile_list_entry *l;
 
 	odb_source_prepare(&store->base, 0);
-	if (store->midx && fill_midx_entry(store->midx, oid, e))
+	if (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))
 		return 1;
 
 	for (l = store->packs.head; l; l = l->next) {
 		struct packed_git *p = l->pack;
 
-		if (!p->multi_pack_index && packfile_fill_entry(p, oid, e)) {
+		if (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) {
 			if (!store->skip_mru_updates)
 				packfile_list_prepend(&store->packs, p);
 			return 1;
@@ -40,6 +41,7 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
 					      enum object_info_flags flags)
 {
 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
+	struct packed_git *bad_pack = NULL;
 	struct pack_entry e;
 	int ret;
 
@@ -51,8 +53,16 @@ static int odb_source_packed_read_object_info(struct odb_source *source,
 	if (flags & OBJECT_INFO_SECOND_READ)
 		odb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);
 
-	if (!find_pack_entry(packed, oid, &e))
+	if (!find_pack_entry(packed, oid, &e, &bad_pack)) {
+		/*
+		 * The lookup may have failed because the object is known to be
+		 * corrupt in one of the packfiles. Report the object as
+		 * corrupt instead of missing in that case.
+		 */
+		if (bad_pack)
+			return -1;
 		return 1;
+	}
 
 	/*
 	 * We know that the caller doesn't actually need the
@@ -77,7 +87,7 @@ static int odb_source_packed_read_object_stream(struct odb_read_stream **out,
 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
 	struct pack_entry e;
 
-	if (!find_pack_entry(packed, oid, &e))
+	if (!find_pack_entry(packed, oid, &e, NULL))
 		return -1;
 
 	return packfile_read_object_stream(out, oid, e.p, e.offset);
@@ -583,7 +593,7 @@ static int odb_source_packed_freshen_object(struct odb_source *source,
 		timesp = &times;
 	}
 
-	if (!find_pack_entry(packed, oid, &e))
+	if (!find_pack_entry(packed, oid, &e, NULL))
 		return 0;
 	if (e.p->is_cruft)
 		return 0;
diff --git a/packfile.c b/packfile.c
index 0eee45055f..34e2f9bb8b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1859,13 +1859,17 @@ int is_pack_valid(struct packed_git *p)
 
 int packfile_fill_entry(struct packed_git *p,
 			const struct object_id *oid,
-			struct pack_entry *e)
+			struct pack_entry *e,
+			struct packed_git **bad_pack)
 {
 	off_t offset;
 
 	if (oidset_size(&p->bad_objects) &&
-	    oidset_contains(&p->bad_objects, oid))
+	    oidset_contains(&p->bad_objects, oid)) {
+		if (bad_pack && !*bad_pack)
+			*bad_pack = p;
 		return 0;
+	}
 
 	offset = find_pack_entry_one(oid, p);
 	if (!offset)
@@ -1962,7 +1966,7 @@ int has_object_kept_pack(struct repository *r, const struct object_id *oid,
 
 		for (; *cache; cache++) {
 			struct packed_git *p = *cache;
-			if (packfile_fill_entry(p, oid, &e))
+			if (packfile_fill_entry(p, oid, &e, NULL))
 				return 1;
 		}
 	}
diff --git a/packfile.h b/packfile.h
index e1f77152b5..3229a6ed47 100644
--- a/packfile.h
+++ b/packfile.h
@@ -294,7 +294,8 @@ off_t find_pack_entry_one(const struct object_id *oid, struct packed_git *);
 
 int packfile_fill_entry(struct packed_git *p,
 			const struct object_id *oid,
-			struct pack_entry *e);
+			struct pack_entry *e,
+			struct packed_git **bad_pack);
 
 int is_pack_valid(struct packed_git *);
 void *unpack_entry(struct repository *r, struct packed_git *, off_t,
diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
index fb16ec0176..27a05da957 100644
--- a/t/helper/test-read-midx.c
+++ b/t/helper/test-read-midx.c
@@ -82,7 +82,7 @@ static int read_midx_file(const char *object_dir, const char *checksum,
 		for (i = 0; i < m->num_objects; i++) {
 			nth_midxed_object_oid(&oid, m,
 					      i + m->num_objects_in_base);
-			fill_midx_entry(m, &oid, &e);
+			fill_midx_entry(m, &oid, &e, NULL);
 
 			printf("%s %"PRIu64"\t%s\n",
 			       oid_to_hex(&oid), e.offset, e.p->pack_name);

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH v2 2/5] odb/source: introduce error status when reading objects

[Patrick Steinhardt]

The `read_object_info()` callback of `struct odb_source` is documented
to return a negative error code in case reading the object has failed,
and zero otherwise. This is overly broad though, as there are two very
different kinds of failures:

  - The object may not exist in the source at all.

  - The object exists, but reading it has failed, for example because
    its on-disk state is corrupt.

This distinction matters to callers: when an object is corrupt in one
source we may still find a good copy of it in another source, so we may
still be able to proceed with a given operation.

The "packed" source already distinguishes these cases by returning a
positive value for missing objects and a negative value in case reading
the object has failed. But it is the only such source that distinguishes
those cases, and the returned value is translated into a negative error
code by the "files" backend anyway.

Introduce a new error status that is specific to reading objects and
adapt the infrastructure to return it. For now, we only discern
successful reads from generic failures, which mostly matches the status
quo. In subsequent commits though we're about to add an error that
explicitly tells the caller that an object does not exist.

Note that we keep the "packed" backend as-is with its positive return
code for missing objects. This will be fixed in the next commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c                 | 16 ++++++++--------
 odb.h                 | 15 +++++++++++----
 odb/source-files.c    |  8 ++++----
 odb/source-inmemory.c |  8 ++++----
 odb/source-loose.c    |  8 ++++----
 odb/source-packed.c   |  8 ++++----
 odb/source.h          | 22 +++++++++++-----------
 7 files changed, 46 insertions(+), 39 deletions(-)

diff --git a/odb.c b/odb.c
index caf1d0f542..1b37b26376 100644
--- a/odb.c
+++ b/odb.c
@@ -547,9 +547,9 @@ static int register_all_submodule_sources(struct object_database *odb)
 	return ret;
 }
 
-static int do_oid_object_info_extended(struct object_database *odb,
-				       const struct object_id *oid,
-				       struct object_info *oi, unsigned flags)
+static enum odb_read_status do_oid_object_info_extended(struct object_database *odb,
+							const struct object_id *oid,
+							struct object_info *oi, unsigned flags)
 {
 	const struct object_id *real = oid;
 	int already_retried = 0;
@@ -696,12 +696,12 @@ static int oid_object_info_convert(struct repository *r,
 	return ret;
 }
 
-int odb_read_object_info_extended(struct object_database *odb,
-				  const struct object_id *oid,
-				  struct object_info *oi,
-				  enum object_info_flags flags)
+enum odb_read_status odb_read_object_info_extended(struct object_database *odb,
+						   const struct object_id *oid,
+						   struct object_info *oi,
+						   enum object_info_flags flags)
 {
-	int ret;
+	enum odb_read_status ret;
 
 	if (oid->algo && (hash_algo_by_ptr(odb->repo->hash_algo) != oid->algo))
 		return oid_object_info_convert(odb->repo, oid, oi, flags);
diff --git a/odb.h b/odb.h
index fca67e8253..43cbcc3aba 100644
--- a/odb.h
+++ b/odb.h
@@ -435,14 +435,21 @@ enum object_info_flags {
 	OBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),
 };
 
+enum odb_read_status {
+	/* The read was successful. */
+	ODB_READ_OK = 0,
+	/* The read resulted in a generic error. */
+	ODB_READ_ERROR = -1,
+};
+
 /*
  * Read object info from the object database and populate the `object_info`
  * structure. Returns 0 on success, a negative error code otherwise.
  */
-int odb_read_object_info_extended(struct object_database *odb,
-				  const struct object_id *oid,
-				  struct object_info *oi,
-				  enum object_info_flags flags);
+enum odb_read_status odb_read_object_info_extended(struct object_database *odb,
+						   const struct object_id *oid,
+						   struct object_info *oi,
+						   enum object_info_flags flags);
 
 /*
  * Read a subset of object info for the given object ID. Returns an `enum
diff --git a/odb/source-files.c b/odb/source-files.c
index 5a68af7d84..a28aa5042d 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -59,10 +59,10 @@ static void odb_source_files_prepare(struct odb_source *source,
 	odb_source_prepare(&files->packed->base, flags);
 }
 
-static int odb_source_files_read_object_info(struct odb_source *source,
-					     const struct object_id *oid,
-					     struct object_info *oi,
-					     enum object_info_flags flags)
+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)
 {
 	struct odb_source_files *files = odb_source_files_downcast(source);
 
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 3e71611b8e..53d2e3a852 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -56,10 +56,10 @@ static void populate_object_info(struct odb_source_inmemory *source,
 		oi->source_infop->source = &source->base;
 }
 
-static int 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)
+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)
 {
 	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 ef0e919277..ad8662842d 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -206,10 +206,10 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 	return ret;
 }
 
-static int odb_source_loose_read_object_info(struct odb_source *source,
-					     const struct object_id *oid,
-					     struct object_info *oi,
-					     enum object_info_flags flags)
+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)
 {
 	struct odb_source_loose *loose = odb_source_loose_downcast(source);
 	static struct strbuf buf = STRBUF_INIT;
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 16fa4f5769..dce68a57f7 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -35,10 +35,10 @@ static int find_pack_entry(struct odb_source_packed *store,
 	return 0;
 }
 
-static int odb_source_packed_read_object_info(struct odb_source *source,
-					      const struct object_id *oid,
-					      struct object_info *oi,
-					      enum object_info_flags flags)
+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)
 {
 	struct odb_source_packed *packed = odb_source_packed_downcast(source);
 	struct packed_git *bad_pack = NULL;
diff --git a/odb/source.h b/odb/source.h
index d69f8e2d1c..7b8ff3d19d 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -110,13 +110,13 @@ struct odb_source {
 	 *     second read in case they know that the first read would have
 	 *     already surfaced the object without reloading any on-disk state.
 	 *
-	 * The callback is expected to return a negative error code in case
-	 * reading the object has failed, 0 otherwise.
+	 * The callback is expected to return an `enum odb_read_status`. Please
+	 * refer to the individual values that can be returned.
 	 */
-	int (*read_object_info)(struct odb_source *source,
-				const struct object_id *oid,
-				struct object_info *oi,
-				enum object_info_flags flags);
+	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);
 
 	/*
 	 * This callback is expected to create a new read stream that can be
@@ -340,12 +340,12 @@ static inline void odb_source_prepare(struct odb_source *source,
 
 /*
  * Read an object from the object database source identified by its object ID.
- * Returns 0 on success, a negative error code otherwise.
+ * Please refer to `enum odb_read_status` for the individual error codes.
  */
-static inline int odb_source_read_object_info(struct odb_source *source,
-					      const struct object_id *oid,
-					      struct object_info *oi,
-					      enum object_info_flags flags)
+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)
 {
 	return source->read_object_info(source, oid, oi, flags);
 }

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH v2 3/5] odb/source: let callers discern missing and corrupt objects

[Patrick Steinhardt]

As explained in the preceding commits, reading objects can either fail
because the object truly does not exist or because it exists, but its
data is corrupt. Some callers do care about this distinction, but there
is no way to tell these two cases apart right now.

Introduce a new `ODB_READ_NOT_FOUND` value that ought to be returned by
the backends in case the object truly does not exist and adapt backends
to use it.

Note that we don't yet return this error from `odb_read_object_info()`
itself. This will be fixed in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.h                         |  2 ++
 odb/source-files.c            | 20 +++++++++++++++++---
 odb/source-inmemory.c         |  2 +-
 odb/source-loose.c            | 31 +++++++++++++++++++------------
 odb/source-packed.c           |  2 +-
 t/unit-tests/u-odb-inmemory.c |  3 ++-
 6 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/odb.h b/odb.h
index 43cbcc3aba..1264d4ce7d 100644
--- a/odb.h
+++ b/odb.h
@@ -440,6 +440,8 @@ enum odb_read_status {
 	ODB_READ_OK = 0,
 	/* The read resulted in a generic error. */
 	ODB_READ_ERROR = -1,
+	/* The object could not be found. */
+	ODB_READ_NOT_FOUND = -2,
 };
 
 /*
diff --git a/odb/source-files.c b/odb/source-files.c
index a28aa5042d..e88fd1d399 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -65,12 +65,26 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
 							      enum object_info_flags flags)
 {
 	struct odb_source_files *files = odb_source_files_downcast(source);
+	enum odb_read_status ret_packed, ret_loose;
 
-	if (!odb_source_read_object_info(&files->packed->base, oid, oi, flags) ||
-	    !odb_source_read_object_info(&files->loose->base, oid, oi, flags))
+	ret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags);
+	if (!ret_packed)
 		return 0;
 
-	return -1;
+	ret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags);
+	if (!ret_loose)
+		return 0;
+
+	/*
+	 * Reading the packed object may have failed even though the object
+	 * exists, for example because it is corrupt. Report this failure to
+	 * the caller in case neither of the sources was able to read the
+	 * object, and prefer the error of the packed source in case both
+	 * reads have failed.
+	 */
+	if (ret_packed != ODB_READ_NOT_FOUND)
+		return ret_packed;
+	return ret_loose;
 }
 
 static int odb_source_files_read_object_stream(struct odb_read_stream **out,
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 53d2e3a852..3f3bd12de3 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -66,7 +66,7 @@ static enum odb_read_status odb_source_inmemory_read_object_info(struct odb_sour
 
 	object = find_cached_object(inmemory, oid);
 	if (!object)
-		return -1;
+		return ODB_READ_NOT_FOUND;
 
 	populate_object_info(inmemory, oi, object);
 	return 0;
diff --git a/odb/source-loose.c b/odb/source-loose.c
index ad8662842d..3c942a1069 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -91,11 +91,16 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 		struct stat st;
 
 		if ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) {
-			ret = quick_has_loose(loose, oid) ? 0 : -1;
+			ret = quick_has_loose(loose, oid) ? 0 : ODB_READ_NOT_FOUND;
 			goto out;
 		}
 
 		if (lstat(path, &st) < 0) {
+			if (errno == ENOENT) {
+				ret = ODB_READ_NOT_FOUND;
+				goto out;
+			}
+
 			ret = -1;
 			goto out;
 		}
@@ -113,9 +118,12 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 
 	fd = git_open(path);
 	if (fd < 0) {
-		if (errno != ENOENT)
-			error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
-		ret = -1;
+		if (errno == ENOENT) {
+			ret = ODB_READ_NOT_FOUND;
+			goto out;
+		}
+
+		ret = error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
 		goto out;
 	}
 
@@ -155,7 +163,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 
 		if (parse_loose_header(hdr, oi) < 0) {
 			ret = error(_("unable to parse %s header"), oid_to_hex(oid));
-			goto corrupt;
+			goto out;
 		}
 
 		if (*oi->typep < 0)
@@ -165,7 +173,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 			*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
 			if (!*oi->contentp) {
 				ret = -1;
-				goto corrupt;
+				goto out;
 			}
 		}
 
@@ -173,21 +181,20 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 	case ULHR_BAD:
 		ret = error(_("unable to unpack %s header"),
 			    oid_to_hex(oid));
-		goto corrupt;
+		goto out;
 	case ULHR_TOO_LONG:
 		ret = error(_("header for %s too long, exceeds %d bytes"),
 			    oid_to_hex(oid), MAX_HEADER_LEN);
-		goto corrupt;
+		goto out;
 	}
 
 	ret = 0;
 
-corrupt:
-	if (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+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);
 
-out:
 	if (stream_to_end)
 		git_inflate_end(stream_to_end);
 	if (map)
@@ -221,7 +228,7 @@ static enum odb_read_status odb_source_loose_read_object_info(struct odb_source
 	 * second time.
 	 */
 	if (flags & OBJECT_INFO_SECOND_READ)
-		return -1;
+		return ODB_READ_NOT_FOUND;
 
 	odb_loose_path(loose, &buf, oid);
 	return read_object_info_from_path(loose, buf.buf, oid, oi, flags);
diff --git a/odb/source-packed.c b/odb/source-packed.c
index dce68a57f7..9b19405380 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -61,7 +61,7 @@ static enum odb_read_status odb_source_packed_read_object_info(struct odb_source
 		 */
 		if (bad_pack)
 			return -1;
-		return 1;
+		return ODB_READ_NOT_FOUND;
 	}
 
 	/*
diff --git a/t/unit-tests/u-odb-inmemory.c b/t/unit-tests/u-odb-inmemory.c
index ddf2db5c81..3e5068080c 100644
--- a/t/unit-tests/u-odb-inmemory.c
+++ b/t/unit-tests/u-odb-inmemory.c
@@ -72,7 +72,8 @@ 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_must_fail(odb_source_read_object_info(&source->base, &oid, NULL, 0));
+	cl_assert_equal_i(odb_source_read_object_info(&source->base, &oid, NULL, 0),
+			  ODB_READ_NOT_FOUND);
 
 	odb_source_free(&source->base);
 }

-- 
2.55.0.822.g20453c30eb.dirty

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

[Patrick Steinhardt]

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 <ps@pks.im>
---
 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

[PATCH v2 5/5] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically

[Patrick Steinhardt]

When a lookup with `OBJECT_INFO_DIE_IF_CORRUPT` fails we want to die in
case the object exists, but cannot be read. This flag is handled in two
different spots right now:

  - `do_oid_object_info_extended()` calls `has_packed_and_bad()` to
    check whether the object is known to be corrupt in any packfile.
    This function reaches into the internals of the packed source and
    thus breaks the abstraction provided by our object sources.

  - The loose source handles the flag itself and dies directly in
    `read_object_info_from_path()`, which means that we die even in
    cases where another source may still have a good copy of the
    object.

Besides being inconsistent, it also ties us to the specific backend used
by the database sources because `has_packed_and_bad()` assumes that they
use the "files" backend. Any other backend will instead cause us to die
when calling `odb_source_files_downcast()`, even if the object was
simply nonexistent.

In the preceding commits we've carved out the infrastructure to make
this mechanism fully generic. On the one hand, all backends now tell us
whether the object is missing or corrupt via their return values. And
on the other hand, they have been taught to provide a readable error
message to the caller.

Adapt `do_oid_object_info_extended()` to use those new mechanisms. This
means that we won't die immediately anymore when a loose object is
corrupt, and we properly handle backends other than the "files" backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c                        | 46 ++++++++++++++++++++++++++++++--------------
 odb/source-loose.c           | 10 ++--------
 packfile.c                   | 17 ----------------
 packfile.h                   |  1 -
 t/t1060-object-corruption.sh | 18 +++++++++++++++++
 5 files changed, 52 insertions(+), 40 deletions(-)

diff --git a/odb.c b/odb.c
index 83a53f7f6b..6bbea64033 100644
--- a/odb.c
+++ b/odb.c
@@ -15,7 +15,6 @@
 #include "object-name.h"
 #include "odb.h"
 #include "odb/source-inmemory.h"
-#include "packfile.h"
 #include "path.h"
 #include "promisor-remote.h"
 #include "quote.h"
@@ -551,8 +550,11 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 							const struct object_id *oid,
 							struct object_info *oi, unsigned flags)
 {
+	struct strbuf corrupt_err = STRBUF_INIT;
 	const struct object_id *real = oid;
+	enum odb_read_status ret;
 	int already_retried = 0;
+	bool corrupt = false;
 
 	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
 		real = lookup_replace_object(odb->repo, oid);
@@ -568,9 +570,14 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 	while (1) {
 		struct odb_source *source;
 
-		for (source = odb->sources; source; source = source->next)
-			if (!odb_source_read_object_info(source, real, oi, flags, NULL))
-				return 0;
+		for (source = odb->sources; source; source = source->next) {
+			ret = odb_source_read_object_info(source, real, oi, flags,
+							  corrupt_err.len ? NULL : &corrupt_err);
+			if (!ret)
+				goto out;
+			if (ret != ODB_READ_NOT_FOUND)
+				corrupt = true;
+		}
 
 		/*
 		 * When the object hasn't been found we try a second read and
@@ -578,11 +585,15 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 		 * caches or reload on-disk state.
 		 */
 		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,
-								 NULL))
-					return 0;
+			for (source = odb->sources; source; source = source->next) {
+				ret = odb_source_read_object_info(source, real, oi,
+								  flags | OBJECT_INFO_SECOND_READ,
+								  corrupt_err.len ? NULL : &corrupt_err);
+				if (!ret)
+					goto out;
+				if (ret != ODB_READ_NOT_FOUND)
+					corrupt = true;
+			}
 		}
 
 		/*
@@ -605,16 +616,23 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
 		}
 
 		if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
-			const struct packed_git *p;
 			if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
 				die(_("replacement %s not found for %s"),
 				    oid_to_hex(real), oid_to_hex(oid));
-			if ((p = has_packed_and_bad(odb->repo, real)))
-				die(_("packed object %s (stored in %s) is corrupt"),
-				    oid_to_hex(real), p->pack_name);
+			if (corrupt) {
+				if (corrupt_err.len)
+					die("%s", corrupt_err.buf);
+				die(_("object %s is corrupt"), oid_to_hex(real));
+			}
 		}
-		return -1;
+
+		ret = corrupt ? ODB_READ_ERROR : ODB_READ_NOT_FOUND;
+		goto out;
 	}
+
+out:
+	strbuf_release(&corrupt_err);
+	return ret;
 }
 
 static int oid_object_info_convert(struct repository *r,
diff --git a/odb/source-loose.c b/odb/source-loose.c
index b57ee2701a..540b2dd40d 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -192,15 +192,9 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
 	ret = 0;
 
 out:
-	if (ret && ret != ODB_READ_NOT_FOUND) {
-		if ((flags & OBJECT_INFO_DIE_IF_CORRUPT))
-			die(_("loose object %s (stored in %s) is corrupt"),
+	if (ret && ret != ODB_READ_NOT_FOUND && errmsg)
+		strbuf_addf(errmsg, _("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);
 	if (map)
diff --git a/packfile.c b/packfile.c
index 3cde39a01c..cd38be088d 100644
--- a/packfile.c
+++ b/packfile.c
@@ -985,23 +985,6 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)
 	oidset_insert(&p->bad_objects, oid);
 }
 
-const struct packed_git *has_packed_and_bad(struct repository *r,
-					    const struct object_id *oid)
-{
-	struct odb_source *source;
-
-	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct packfile_list_entry *e;
-
-		for (e = files->packed->packs.head; e; e = e->next)
-			if (oidset_contains(&e->pack->bad_objects, oid))
-				return e->pack;
-	}
-
-	return NULL;
-}
-
 off_t get_delta_base(struct packed_git *p,
 		     struct pack_window **w_curs,
 		     off_t *curpos,
diff --git a/packfile.h b/packfile.h
index 3229a6ed47..573fe003d0 100644
--- a/packfile.h
+++ b/packfile.h
@@ -329,7 +329,6 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source,
 				      uint32_t *maybe_index_pos, struct object_info *oi);
 
 void mark_bad_packed_object(struct packed_git *, const struct object_id *);
-const struct packed_git *has_packed_and_bad(struct repository *, const struct object_id *);
 
 int has_object_pack(struct repository *r, const struct object_id *oid);
 int has_object_kept_pack(struct repository *r, const struct object_id *oid,
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index 502a5ea1c5..d2ef468b45 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -145,4 +145,22 @@ test_expect_success 'partial clone of corrupted repository' '
 	test_must_fail git -C corrupt-partial checkout --force
 '
 
+test_expect_success 'corrupted loose commit can be read from alternate' '
+	git init repo-a &&
+	tree=$(git -C repo-a write-tree) &&
+	commit=$(git -C repo-a commit-tree $tree </dev/null) &&
+
+	cp -r repo-a repo-b &&
+	(
+		cd repo-b &&
+		echo ../../../repo-a/.git/objects >.git/objects/info/alternates &&
+		corrupt_byte "$commit" 1
+	) &&
+
+	git -C repo-a cat-file -p "$commit" >expect &&
+	git -C repo-b cat-file -p "$commit" >actual 2>err &&
+	test_cmp expect actual &&
+	test_grep "inflate: data stream error" err
+'
+
 test_done

-- 
2.55.0.822.g20453c30eb.dirty

Re: [PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Aug 18, 2026 at 11:17:47AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > @@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,
>> >  		return 0;
>> >  
>> >  	if (oidset_size(&p->bad_objects) &&
>> > -	    oidset_contains(&p->bad_objects, oid))
>> > +	    oidset_contains(&p->bad_objects, oid)) {
>> > +		if (bad_pack && !*bad_pack)
>> > +			*bad_pack = p;
>> >  		return 0;
>> > +	}
>> 
>> Hmph, so the idea is that if you have even one bad thing, you are
>> marked as bad, because who knows what other parts of you are broken?
>
> No, not quite. We don't mark the whole pack itself as bad, we only mark
> the objects that's contained in there as bad. The only reason why we
> also bubble up the pack is so that we can provide a better error message
> in a subsequent commit, where we can then tell the user which pack it
> was specifically that contains the bad commit.
>
> That's by itself not visible in this commit yet, but I do mention it as
> part of the commit message.
>
> Patrick

OK.

This is a tangent but the argument heavily relies on the invariant
that a single pack can contain one object at most once.  Once a
corrupt pack that has copies of the same object duplicated in it
comes into the picture, the error message has to say which copy is
bad.