{"thread":{"id":"66190","subject":"[PATCH 0/7] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically","startedAt":"2026-08-18T14:19:41Z","lastAt":"2026-08-19T17:42:47Z","messageCount":18,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"550748","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","threadId":"66190","inReplyTo":null,"subject":"[PATCH 0/7] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:27Z","receivedAt":"2026-08-18T14:19:41Z","isPatch":true,"body":"Hi,\n\nwhen looking up an object with `OBJECT_INFO_DIE_IF_CORRUPT` fails we\nwant to die in case the object exists but is corrupted. This flag is\nhandled in two different spots right now:\n\n  - `do_oid_object_info_extended()` calls `has_packed_and_bad()` to\n    check whether the object is known to be corrupt in any packfile.\n    This function reaches into the internals of the packed source and\n    thus breaks the abstraction provided by our object sources.\n\n  - The loose source handles the flag itself and dies directly in\n    `read_object_info_from_path()`, which means that we die even in\n    cases where another source may still have a good copy of the\n    object.\n\nBesides being inconsistent, it also ties us to the specific backend used\nby the database sources because `has_packed_and_bad()` assumes that they\nuse the \"files\" backend. Any other backend will instead cause us to die\nwhen calling `odb_source_files_downcast()`, even if the object was\nsimply nonexistent.\n\nThis series fixes these issues and makes the check backend-agnostic by\nextending semantics of `odb_source_read_object_info()`: on the one hand\nit now distinguishes whether an object is missing or corrput, and on the\nother hand it starts to return an error message to the caller.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (7):\n      odb/source: discern missing and corrupt objects\n      odb/source-inmemory: signal missing objects via positive return\n      odb/source-packed: flag known-bad objects as corrupt and not missing\n      odb/source-loose: distinguish missing and corrupt objects\n      odb/source-files: signal mark objects via positive return\n      odb/source: allow `read_object_info()` to bubble up error messages\n      odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically\n\n builtin/pack-objects.c        |  8 +++----\n midx.c                        | 10 +++++---\n midx.h                        |  3 ++-\n odb.c                         | 47 ++++++++++++++++++++++++++------------\n odb/source-files.c            | 25 ++++++++++++++++----\n odb/source-inmemory.c         |  5 ++--\n odb/source-loose.c            | 46 +++++++++++++++++++++----------------\n odb/source-packed.c           | 53 +++++++++++++++++++++++++++++++++----------\n odb/source.h                  | 33 ++++++++++++++++++++++-----\n packfile.c                    | 29 +++++++----------------\n packfile.h                    |  4 ++--\n t/helper/test-read-midx.c     |  2 +-\n t/t1060-object-corruption.sh  | 18 +++++++++++++++\n t/unit-tests/u-odb-inmemory.c |  4 ++--\n 14 files changed, 196 insertions(+), 91 deletions(-)\n\n\n---\nbase-commit: 18e66859d87fb4b76599f73460b54f0848c76b16\nchange-id: 20260818-pks-odb-generic-corrupt-objects-52a47d6214d9\n\n"},{"id":"550749","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-1-ec234567510f@pks.im","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","subject":"[PATCH 1/7] odb/source: discern missing and corrupt objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:28Z","receivedAt":"2026-08-18T14:19:42Z","isPatch":true,"body":"The `read_object_info()` callback of `struct odb_source` is documented\nto return a negative error code in case reading the object has failed,\nand zero otherwise. This is overly broad though, as there are two very\ndifferent kinds of failures:\n\n  - The object may not exist in the source at all.\n\n  - The object exists, but reading it has failed, for example because\n    its on-disk state is corrupt.\n\nThis distinction matters to callers: when an object is corrupt in one\nsource we may still find a good copy of it in another source, so we may\nstill be able to proceed with a given operation.\n\nThe \"packed\" source already distinguishes these cases by returning a\npositive value for missing objects and a negative value in case reading\nthe object has failed. But all the other sources conflate them into a\nsingle negative return value.\n\nAdapt the documentation to explicitly require the semantics of the\n\"packed\" backend, where we return a positive value for missing objects\nand a negative value for corrupt ones. Subsequent commits will adapt all\nthe other implementations to respect those new semantics.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n odb/source.h | 17 ++++++++++++++---\n 1 file changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/odb/source.h b/odb/source.h\nindex d69f8e2d1c..4ae6cc160e 100644\n--- a/odb/source.h\n+++ b/odb/source.h\n@@ -110,8 +110,17 @@ struct odb_source {\n \t *     second read in case they know that the first read would have\n \t *     already surfaced the object without reloading any on-disk state.\n \t *\n-\t * The callback is expected to return a negative error code in case\n-\t * reading the object has failed, 0 otherwise.\n+\t * The callback is expected to return one of the following values:\n+\t *\n+\t *   - Zero in case the object has been found and its object info has\n+\t *     been read successfully.\n+\t *\n+\t *   - A positive value in case the object does not exist in this\n+\t *     source.\n+\t *\n+\t *   - A negative value in case the object exists in this source, but\n+\t *     reading its object info has failed, for example because its\n+\t *     on-disk state is corrupt.\n \t */\n \tint (*read_object_info)(struct odb_source *source,\n \t\t\t\tconst struct object_id *oid,\n@@ -340,7 +349,9 @@ static inline void odb_source_prepare(struct odb_source *source,\n \n /*\n  * Read an object from the object database source identified by its object ID.\n- * Returns 0 on success, a negative error code otherwise.\n+ * Returns 0 on success, a positive value in case the object is missing in the\n+ * source and a negative value in case the object exists, but reading it has\n+ * failed.\n  */\n static inline int odb_source_read_object_info(struct odb_source *source,\n \t\t\t\t\t      const struct object_id *oid,\n\n-- \n2.55.0.822.g20453c30eb.dirty\n\n"},{"id":"550750","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-2-ec234567510f@pks.im","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","subject":"[PATCH 2/7] odb/source-inmemory: signal missing objects via positive return","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:29Z","receivedAt":"2026-08-18T14:19:44Z","isPatch":true,"body":"The in-memory source returns a negative value from its\n`read_object_info()` callback when the object in question does not\nexist. Adapt the callback to return a positive value for missing objects\naccording to the new calling convention.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n odb/source-inmemory.c         | 2 +-\n t/unit-tests/u-odb-inmemory.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/odb/source-inmemory.c b/odb/source-inmemory.c\nindex 3e71611b8e..57183daf4d 100644\n--- a/odb/source-inmemory.c\n+++ b/odb/source-inmemory.c\n@@ -66,7 +66,7 @@ static int odb_source_inmemory_read_object_info(struct odb_source *source,\n \n \tobject = find_cached_object(inmemory, oid);\n \tif (!object)\n-\t\treturn -1;\n+\t\treturn 1;\n \n \tpopulate_object_info(inmemory, oi, object);\n \treturn 0;\ndiff --git a/t/unit-tests/u-odb-inmemory.c b/t/unit-tests/u-odb-inmemory.c\nindex ddf2db5c81..93b3f38dab 100644\n--- a/t/unit-tests/u-odb-inmemory.c\n+++ b/t/unit-tests/u-odb-inmemory.c\n@@ -72,7 +72,7 @@ void test_odb_inmemory__read_missing_object(void)\n \tconst char *end;\n \n \tcl_must_pass(parse_oid_hex_algop(RANDOM_OID, &oid, &end, repo.hash_algo));\n-\tcl_must_fail(odb_source_read_object_info(&source->base, &oid, NULL, 0));\n+\tcl_assert(odb_source_read_object_info(&source->base, &oid, NULL, 0) > 0);\n \n \todb_source_free(&source->base);\n }\n\n-- \n2.55.0.822.g20453c30eb.dirty\n\n"},{"id":"550751","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-3-ec234567510f@pks.im","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","subject":"[PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:30Z","receivedAt":"2026-08-18T14:19:47Z","isPatch":true,"body":"When reading a packed object that doesn't verify we mark it as bad and\nindicate to the caller that we failed reading the object despite the\nfact that it supposedly exists. This matches the semantics we have now\nestablished in a preceding commit, where we discern failure to read a\ncorrupt object from a missing object.\n\nWhat doesn't work yet though is when a call tries to read an object that\nhas already been marked as corrupt in a previous call. In that case,\n`find_pack_entry()` will tell us that the object in question does not\nexist, and consequently we'll not flag the object as corrupt but as\nmissing.\n\nFix this issue by bubbling up whether the object is corrupt and, if so,\nwhich packfile contains the corrupted object. We don't yet need the\nlatter information about the specific packfile, so we could've just as\nwell made this a `bool *corrupted` pointer. But we'll need information\nabout the containing packfile in a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/pack-objects.c    |  2 +-\n midx.c                    | 10 +++++++---\n midx.h                    |  3 ++-\n odb/source-packed.c       | 23 +++++++++++++++++------\n packfile.c                | 10 +++++++---\n packfile.h                |  3 ++-\n t/helper/test-read-midx.c |  2 +-\n 7 files changed, 37 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1ec5b6f206..10c2471024 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1786,7 +1786,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\tstruct multi_pack_index *m = get_multi_pack_index(files->packed);\n \t\tstruct pack_entry e;\n \n-\t\tif (m && fill_midx_entry(m, oid, &e)) {\n+\t\tif (m && fill_midx_entry(m, oid, &e, NULL)) {\n \t\t\twant = want_object_in_pack_one(e.p, oid, exclude, found_pack, found_offset, found_mtime);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\ndiff --git a/midx.c b/midx.c\nindex 76c3f92cc3..37f082dbdd 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -591,7 +591,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n \n int fill_midx_entry(struct multi_pack_index *m,\n \t\t    const struct object_id *oid,\n-\t\t    struct pack_entry *e)\n+\t\t    struct pack_entry *e,\n+\t\t    struct packed_git **bad_pack)\n {\n \tuint32_t pos;\n \tuint32_t pack_int_id;\n@@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,\n \t\treturn 0;\n \n \tif (oidset_size(&p->bad_objects) &&\n-\t    oidset_contains(&p->bad_objects, oid))\n+\t    oidset_contains(&p->bad_objects, oid)) {\n+\t\tif (bad_pack && !*bad_pack)\n+\t\t\t*bad_pack = p;\n \t\treturn 0;\n+\t}\n \n \te->offset = nth_midxed_offset(m, pos);\n \te->p = p;\n@@ -1028,7 +1032,7 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags)\n \n \t\tnth_midxed_object_oid(&oid, m, pairs[i].pos);\n \n-\t\tif (!fill_midx_entry(m, &oid, &e)) {\n+\t\tif (!fill_midx_entry(m, &oid, &e, NULL)) {\n \t\t\tmidx_report(_(\"failed to load pack entry for oid[%d] = %s\"),\n \t\t\t\t    pairs[i].pos, oid_to_hex(&oid));\n \t\t\tcontinue;\ndiff --git a/midx.h b/midx.h\nindex 939c18e588..1f2f2d5321 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -117,7 +117,8 @@ uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos);\n struct object_id *nth_midxed_object_oid(struct object_id *oid,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t n);\n-int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid, struct pack_entry *e);\n+int fill_midx_entry(struct multi_pack_index *m, const struct object_id *oid,\n+\t\t    struct pack_entry *e, struct packed_git **bad_pack);\n int midx_contains_pack(struct multi_pack_index *m,\n \t\t       const char *idx_or_pack_name);\n int midx_layer_contains_pack(struct multi_pack_index *m,\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 0890704e76..50e9be3b4c 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -13,18 +13,19 @@\n \n static int find_pack_entry(struct odb_source_packed *store,\n \t\t\t   const struct object_id *oid,\n-\t\t\t   struct pack_entry *e)\n+\t\t\t   struct pack_entry *e,\n+\t\t\t   struct packed_git **bad_pack)\n {\n \tstruct packfile_list_entry *l;\n \n \todb_source_prepare(&store->base, 0);\n-\tif (store->midx && fill_midx_entry(store->midx, oid, e))\n+\tif (store->midx && fill_midx_entry(store->midx, oid, e, bad_pack))\n \t\treturn 1;\n \n \tfor (l = store->packs.head; l; l = l->next) {\n \t\tstruct packed_git *p = l->pack;\n \n-\t\tif (!p->multi_pack_index && packfile_fill_entry(p, oid, e)) {\n+\t\tif (!p->multi_pack_index && packfile_fill_entry(p, oid, e, bad_pack)) {\n \t\t\tif (!store->skip_mru_updates)\n \t\t\t\tpackfile_list_prepend(&store->packs, p);\n \t\t\treturn 1;\n@@ -40,6 +41,7 @@ static int odb_source_packed_read_object_info(struct odb_source *source,\n \t\t\t\t\t      enum object_info_flags flags)\n {\n \tstruct odb_source_packed *packed = odb_source_packed_downcast(source);\n+\tstruct packed_git *bad_pack = NULL;\n \tstruct pack_entry e;\n \tint ret;\n \n@@ -51,8 +53,17 @@ static int odb_source_packed_read_object_info(struct odb_source *source,\n \tif (flags & OBJECT_INFO_SECOND_READ)\n \t\todb_source_prepare(source, ODB_PREPARE_FLUSH_CACHES);\n \n-\tif (!find_pack_entry(packed, oid, &e))\n+\tif (!find_pack_entry(packed, oid, &e, &bad_pack)) {\n+\t\t/*\n+\t\t * The lookup may have failed because the object is known to\n+\t\t * be corrupt in one of our packfiles, in which case the\n+\t\t * corresponding pack entries are skipped. Report the object\n+\t\t * as corrupt instead of as missing in that case.\n+\t\t */\n+\t\tif (bad_pack)\n+\t\t\treturn -1;\n \t\treturn 1;\n+\t}\n \n \t/*\n \t * We know that the caller doesn't actually need the\n@@ -77,7 +88,7 @@ static int odb_source_packed_read_object_stream(struct odb_read_stream **out,\n \tstruct odb_source_packed *packed = odb_source_packed_downcast(source);\n \tstruct pack_entry e;\n \n-\tif (!find_pack_entry(packed, oid, &e))\n+\tif (!find_pack_entry(packed, oid, &e, NULL))\n \t\treturn -1;\n \n \treturn packfile_read_object_stream(out, oid, e.p, e.offset);\n@@ -583,7 +594,7 @@ static int odb_source_packed_freshen_object(struct odb_source *source,\n \t\ttimesp = &times;\n \t}\n \n-\tif (!find_pack_entry(packed, oid, &e))\n+\tif (!find_pack_entry(packed, oid, &e, NULL))\n \t\treturn 0;\n \tif (e.p->is_cruft)\n \t\treturn 0;\ndiff --git a/packfile.c b/packfile.c\nindex 0eee45055f..34e2f9bb8b 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1859,13 +1859,17 @@ int is_pack_valid(struct packed_git *p)\n \n int packfile_fill_entry(struct packed_git *p,\n \t\t\tconst struct object_id *oid,\n-\t\t\tstruct pack_entry *e)\n+\t\t\tstruct pack_entry *e,\n+\t\t\tstruct packed_git **bad_pack)\n {\n \toff_t offset;\n \n \tif (oidset_size(&p->bad_objects) &&\n-\t    oidset_contains(&p->bad_objects, oid))\n+\t    oidset_contains(&p->bad_objects, oid)) {\n+\t\tif (bad_pack && !*bad_pack)\n+\t\t\t*bad_pack = p;\n \t\treturn 0;\n+\t}\n \n \toffset = find_pack_entry_one(oid, p);\n \tif (!offset)\n@@ -1962,7 +1966,7 @@ int has_object_kept_pack(struct repository *r, const struct object_id *oid,\n \n \t\tfor (; *cache; cache++) {\n \t\t\tstruct packed_git *p = *cache;\n-\t\t\tif (packfile_fill_entry(p, oid, &e))\n+\t\t\tif (packfile_fill_entry(p, oid, &e, NULL))\n \t\t\t\treturn 1;\n \t\t}\n \t}\ndiff --git a/packfile.h b/packfile.h\nindex e1f77152b5..3229a6ed47 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -294,7 +294,8 @@ off_t find_pack_entry_one(const struct object_id *oid, struct packed_git *);\n \n int packfile_fill_entry(struct packed_git *p,\n \t\t\tconst struct object_id *oid,\n-\t\t\tstruct pack_entry *e);\n+\t\t\tstruct pack_entry *e,\n+\t\t\tstruct packed_git **bad_pack);\n \n int is_pack_valid(struct packed_git *);\n void *unpack_entry(struct repository *r, struct packed_git *, off_t,\ndiff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\nindex fb16ec0176..27a05da957 100644\n--- a/t/helper/test-read-midx.c\n+++ b/t/helper/test-read-midx.c\n@@ -82,7 +82,7 @@ static int read_midx_file(const char *object_dir, const char *checksum,\n \t\tfor (i = 0; i < m->num_objects; i++) {\n \t\t\tnth_midxed_object_oid(&oid, m,\n \t\t\t\t\t      i + m->num_objects_in_base);\n-\t\t\tfill_midx_entry(m, &oid, &e);\n+\t\t\tfill_midx_entry(m, &oid, &e, NULL);\n \n \t\t\tprintf(\"%s %\"PRIu64\"\\t%s\\n\",\n \t\t\t       oid_to_hex(&oid), e.offset, e.p->pack_name);\n\n-- \n2.55.0.822.g20453c30eb.dirty\n\n"},{"id":"550752","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-4-ec234567510f@pks.im","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","subject":"[PATCH 4/7] odb/source-loose: distinguish missing and corrupt objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:31Z","receivedAt":"2026-08-18T14:19:49Z","isPatch":true,"body":"The loose source returns a negative value from its `read_object_info()`\ncallback both when the object is missing and when the object exists but\ncannot be read. Consequently, callers cannot tell apart whether the\nobject does not exist in this source at all or whether it is corrupt.\n\nAdapt the code to return a positive value for missing objects according\nto the new calling convention.\n\nThis also allows us to get rid of the separate `corrupt:` label, as we\ncan now clearly distinguish between corrupt and missing objects in the\nfunction ourselves. This makes us handle failures to read loose objects\nmore consistently, as not all failure cases were jumping that label.\n\nNote that there's one call to `die()` when the object type is invalid\nthat should arguably be converted to an error, too. But adapting that\ncall results in quite a lot of broken tests, so this is left as-is for\nnow.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n odb/source-loose.c | 35 +++++++++++++++++++++--------------\n 1 file changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/odb/source-loose.c b/odb/source-loose.c\nindex ef0e919277..e786560ad1 100644\n--- a/odb/source-loose.c\n+++ b/odb/source-loose.c\n@@ -91,11 +91,16 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \t\tstruct stat st;\n \n \t\tif ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) {\n-\t\t\tret = quick_has_loose(loose, oid) ? 0 : -1;\n+\t\t\tret = quick_has_loose(loose, oid) ? 0 : 1;\n \t\t\tgoto out;\n \t\t}\n \n \t\tif (lstat(path, &st) < 0) {\n+\t\t\tif (errno == ENOENT) {\n+\t\t\t\tret = 1;\n+\t\t\t\tgoto out;\n+\t\t\t}\n+\n \t\t\tret = -1;\n \t\t\tgoto out;\n \t\t}\n@@ -113,9 +118,12 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \n \tfd = git_open(path);\n \tif (fd < 0) {\n-\t\tif (errno != ENOENT)\n-\t\t\terror_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n-\t\tret = -1;\n+\t\tif (errno == ENOENT) {\n+\t\t\tret = 1;\n+\t\t\tgoto out;\n+\t\t}\n+\n+\t\tret = error_errno(_(\"unable to open loose object %s\"), oid_to_hex(oid));\n \t\tgoto out;\n \t}\n \n@@ -155,7 +163,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \n \t\tif (parse_loose_header(hdr, oi) < 0) {\n \t\t\tret = error(_(\"unable to parse %s header\"), oid_to_hex(oid));\n-\t\t\tgoto corrupt;\n+\t\t\tgoto out;\n \t\t}\n \n \t\tif (*oi->typep < 0)\n@@ -165,7 +173,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \t\t\t*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);\n \t\t\tif (!*oi->contentp) {\n \t\t\t\tret = -1;\n-\t\t\t\tgoto corrupt;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t}\n \n@@ -173,21 +181,20 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \tcase ULHR_BAD:\n \t\tret = error(_(\"unable to unpack %s header\"),\n \t\t\t    oid_to_hex(oid));\n-\t\tgoto corrupt;\n+\t\tgoto out;\n \tcase ULHR_TOO_LONG:\n \t\tret = error(_(\"header for %s too long, exceeds %d bytes\"),\n \t\t\t    oid_to_hex(oid), MAX_HEADER_LEN);\n-\t\tgoto corrupt;\n+\t\tgoto out;\n \t}\n \n \tret = 0;\n \n-corrupt:\n-\tif (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n+out:\n+\tif (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n \t\t    oid_to_hex(oid), path);\n \n-out:\n \tif (stream_to_end)\n \t\tgit_inflate_end(stream_to_end);\n \tif (map)\n@@ -221,7 +228,7 @@ static int odb_source_loose_read_object_info(struct odb_source *source,\n \t * second time.\n \t */\n \tif (flags & OBJECT_INFO_SECOND_READ)\n-\t\treturn -1;\n+\t\treturn 1;\n \n \todb_loose_path(loose, &buf, oid);\n \treturn read_object_info_from_path(loose, buf.buf, oid, oi, flags);\n@@ -421,7 +428,7 @@ static int for_each_object_wrapper_cb(const struct object_id *oid,\n \tif (data->request) {\n \t\tstruct object_info oi = *data->request;\n \n-\t\tif (read_object_info_from_path(data->loose, path, oid, &oi, 0) < 0)\n+\t\tif (read_object_info_from_path(data->loose, path, oid, &oi, 0))\n \t\t\treturn -1;\n \n \t\treturn data->cb(oid, &oi, data->cb_data);\n@@ -439,7 +446,7 @@ static int for_each_prefixed_object_wrapper_cb(const struct object_id *oid,\n \t\tstruct object_info oi = *data->request;\n \n \t\tif (odb_source_read_object_info(&data->loose->base,\n-\t\t\t\t\t\toid, &oi, 0) < 0)\n+\t\t\t\t\t\toid, &oi, 0))\n \t\t\treturn -1;\n \n \t\treturn data->cb(oid, &oi, data->cb_data);\n\n-- \n2.55.0.822.g20453c30eb.dirty\n\n"},{"id":"550753","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-5-ec234567510f@pks.im","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","subject":"[PATCH 5/7] odb/source-files: signal mark objects via positive return","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:32Z","receivedAt":"2026-08-18T14:19:52Z","isPatch":true,"body":"The files source conflates all failures of its child sources into a\nnegative return value, so callers cannot tell apart whether an object is\nmissing or whether reading it has failed. Both the packed and the loose\nsource have been converted to adhere to the tri-state return convention\nof `read_object_info()` by now, so all that is left to do is to\npropagate their respective return values.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n odb/source-files.c | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/odb/source-files.c b/odb/source-files.c\nindex 5a68af7d84..1124a18091 100644\n--- a/odb/source-files.c\n+++ b/odb/source-files.c\n@@ -65,12 +65,26 @@ static int odb_source_files_read_object_info(struct odb_source *source,\n \t\t\t\t\t     enum object_info_flags flags)\n {\n \tstruct odb_source_files *files = odb_source_files_downcast(source);\n+\tint ret_packed, ret_loose;\n \n-\tif (!odb_source_read_object_info(&files->packed->base, oid, oi, flags) ||\n-\t    !odb_source_read_object_info(&files->loose->base, oid, oi, flags))\n+\tret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags);\n+\tif (!ret_packed)\n \t\treturn 0;\n \n-\treturn -1;\n+\tret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags);\n+\tif (!ret_loose)\n+\t\treturn 0;\n+\n+\t/*\n+\t * Reading the packed object may have failed even though the object\n+\t * exists, for example because it is corrupt. Report this failure to\n+\t * the caller in case neither of the sources was able to read the\n+\t * object, and prefer the error of the packed source in case both\n+\t * reads have failed.\n+\t */\n+\tif (ret_packed < 0)\n+\t\treturn ret_packed;\n+\treturn ret_loose;\n }\n \n static int odb_source_files_read_object_stream(struct odb_read_stream **out,\n\n-- \n2.55.0.822.g20453c30eb.dirty\n\n"},{"id":"550754","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-6-ec234567510f@pks.im","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","subject":"[PATCH 6/7] odb/source: allow `read_object_info()` to bubble up error messages","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:33Z","receivedAt":"2026-08-18T14:19:54Z","isPatch":true,"body":"When reading an object fails even though it exists, the sources know\nbest what exactly went wrong and where the corrupt object is located.\nThis information is lost though when bubbling up the error to the object\ndatabase layer, which forces that layer to reconstruct it after the\nfact. This is exactly what `do_oid_object_info_extended()` does via\n`has_packed_and_bad()`, but that function only really knows to handle\nthe \"files\" backend by reaching into its internals.\n\nIntroduce a new `errmsg` parameter for the `read_object_info()` callback\nthat sources are expected to populate with a human-readable message in\ncase reading the object has failed. Adapt the packed and loose sources\nto populate the buffer with the messages that we ultimately want to\nsurface to the user.\n\nFor now, all callers are adapted to pass a `NULL` pointer. We will add a\nuser of this new infrastructure in a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/pack-objects.c        |  6 +++---\n odb.c                         |  7 ++++---\n odb/source-files.c            |  9 ++++++---\n odb/source-inmemory.c         |  3 ++-\n odb/source-loose.c            | 16 +++++++++++-----\n odb/source-packed.c           | 34 ++++++++++++++++++++++++++--------\n odb/source.h                  | 16 +++++++++++++---\n packfile.c                    |  2 +-\n t/unit-tests/u-odb-inmemory.c |  4 ++--\n 9 files changed, 68 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 10c2471024..399acd0f22 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1759,7 +1759,7 @@ static int want_object_in_pack_mtime(const struct object_id *oid,\n \t\tstruct odb_source *source = the_repository->objects->sources->next;\n \t\tfor (; source; source = source->next) {\n \t\t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n-\t\t\tif (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0))\n+\t\t\tif (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL))\n \t\t\t\treturn 0;\n \t\t}\n \t}\n@@ -4171,7 +4171,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type\n \n \t\t\tfor (; !found && source; source = source->next) {\n \t\t\t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n-\t\t\t\tif (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0))\n+\t\t\t\tif (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL))\n \t\t\t\t\tfound = 1;\n \t\t\t}\n \n@@ -4637,7 +4637,7 @@ static int force_object_loose(struct odb_source *source,\n \n \tfor (struct odb_source *s = source->odb->sources; s; s = s->next) {\n \t\tstruct odb_source_files *files = odb_source_files_downcast(s);\n-\t\tif (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0))\n+\t\tif (!odb_source_read_object_info(&files->loose->base, oid, NULL, 0, NULL))\n \t\t\treturn 0;\n \t}\n \ndiff --git a/odb.c b/odb.c\nindex caf1d0f542..6cb0a9534b 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -560,7 +560,7 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \tif (is_null_oid(real))\n \t\treturn -1;\n \n-\tif (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags))\n+\tif (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags, NULL))\n \t\treturn 0;\n \n \todb_prepare_alternates(odb);\n@@ -569,7 +569,7 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \t\tstruct odb_source *source;\n \n \t\tfor (source = odb->sources; source; source = source->next)\n-\t\t\tif (!odb_source_read_object_info(source, real, oi, flags))\n+\t\t\tif (!odb_source_read_object_info(source, real, oi, flags, NULL))\n \t\t\t\treturn 0;\n \n \t\t/*\n@@ -580,7 +580,8 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \t\tif (!(flags & OBJECT_INFO_QUICK)) {\n \t\t\tfor (source = odb->sources; source; source = source->next)\n \t\t\t\tif (!odb_source_read_object_info(source, real, oi,\n-\t\t\t\t\t\t\t\t flags | OBJECT_INFO_SECOND_READ))\n+\t\t\t\t\t\t\t\t flags | OBJECT_INFO_SECOND_READ,\n+\t\t\t\t\t\t\t\t NULL))\n \t\t\t\t\treturn 0;\n \t\t}\n \ndiff --git a/odb/source-files.c b/odb/source-files.c\nindex 1124a18091..4727670e4d 100644\n--- a/odb/source-files.c\n+++ b/odb/source-files.c\n@@ -62,16 +62,19 @@ static void odb_source_files_prepare(struct odb_source *source,\n static int odb_source_files_read_object_info(struct odb_source *source,\n \t\t\t\t\t     const struct object_id *oid,\n \t\t\t\t\t     struct object_info *oi,\n-\t\t\t\t\t     enum object_info_flags flags)\n+\t\t\t\t\t     enum object_info_flags flags,\n+\t\t\t\t\t     struct strbuf *errmsg)\n {\n \tstruct odb_source_files *files = odb_source_files_downcast(source);\n \tint ret_packed, ret_loose;\n \n-\tret_packed = odb_source_read_object_info(&files->packed->base, oid, oi, flags);\n+\tret_packed = odb_source_read_object_info(&files->packed->base, oid, oi,\n+\t\t\t\t\t\t flags, errmsg);\n \tif (!ret_packed)\n \t\treturn 0;\n \n-\tret_loose = odb_source_read_object_info(&files->loose->base, oid, oi, flags);\n+\tret_loose = odb_source_read_object_info(&files->loose->base, oid, oi,\n+\t\t\t\t\t\t flags, ret_packed < 0 ? NULL : errmsg);\n \tif (!ret_loose)\n \t\treturn 0;\n \ndiff --git a/odb/source-inmemory.c b/odb/source-inmemory.c\nindex 57183daf4d..a14d6daeda 100644\n--- a/odb/source-inmemory.c\n+++ b/odb/source-inmemory.c\n@@ -59,7 +59,8 @@ static void populate_object_info(struct odb_source_inmemory *source,\n static int odb_source_inmemory_read_object_info(struct odb_source *source,\n \t\t\t\t\t\tconst struct object_id *oid,\n \t\t\t\t\t\tstruct object_info *oi,\n-\t\t\t\t\t\tenum object_info_flags flags UNUSED)\n+\t\t\t\t\t\tenum object_info_flags flags UNUSED,\n+\t\t\t\t\t\tstruct strbuf *errmsg UNUSED)\n {\n \tstruct odb_source_inmemory *inmemory = odb_source_inmemory_downcast(source);\n \tconst struct inmemory_object *object;\ndiff --git a/odb/source-loose.c b/odb/source-loose.c\nindex e786560ad1..3cee012a6d 100644\n--- a/odb/source-loose.c\n+++ b/odb/source-loose.c\n@@ -67,7 +67,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \t\t\t\t      const char *path,\n \t\t\t\t      const struct object_id *oid,\n \t\t\t\t      struct object_info *oi,\n-\t\t\t\t      enum object_info_flags flags)\n+\t\t\t\t      enum object_info_flags flags,\n+\t\t\t\t      struct strbuf *errmsg)\n {\n \tint ret;\n \tint fd;\n@@ -191,6 +192,10 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \tret = 0;\n \n out:\n+\tif (ret < 0 && errmsg)\n+\t\tstrbuf_addf(errmsg, _(\"loose object %s (stored in %s) is corrupt\"),\n+\t\t\t    oid_to_hex(oid), path);\n+\n \tif (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n \t\t    oid_to_hex(oid), path);\n@@ -216,7 +221,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n static int odb_source_loose_read_object_info(struct odb_source *source,\n \t\t\t\t\t     const struct object_id *oid,\n \t\t\t\t\t     struct object_info *oi,\n-\t\t\t\t\t     enum object_info_flags flags)\n+\t\t\t\t\t     enum object_info_flags flags,\n+\t\t\t\t\t     struct strbuf *errmsg)\n {\n \tstruct odb_source_loose *loose = odb_source_loose_downcast(source);\n \tstatic struct strbuf buf = STRBUF_INIT;\n@@ -231,7 +237,7 @@ static int odb_source_loose_read_object_info(struct odb_source *source,\n \t\treturn 1;\n \n \todb_loose_path(loose, &buf, oid);\n-\treturn read_object_info_from_path(loose, buf.buf, oid, oi, flags);\n+\treturn read_object_info_from_path(loose, buf.buf, oid, oi, flags, errmsg);\n }\n \n /*\n@@ -428,7 +434,7 @@ static int for_each_object_wrapper_cb(const struct object_id *oid,\n \tif (data->request) {\n \t\tstruct object_info oi = *data->request;\n \n-\t\tif (read_object_info_from_path(data->loose, path, oid, &oi, 0))\n+\t\tif (read_object_info_from_path(data->loose, path, oid, &oi, 0, NULL))\n \t\t\treturn -1;\n \n \t\treturn data->cb(oid, &oi, data->cb_data);\n@@ -446,7 +452,7 @@ static int for_each_prefixed_object_wrapper_cb(const struct object_id *oid,\n \t\tstruct object_info oi = *data->request;\n \n \t\tif (odb_source_read_object_info(&data->loose->base,\n-\t\t\t\t\t\toid, &oi, 0))\n+\t\t\t\t\t\toid, &oi, 0, NULL))\n \t\t\treturn -1;\n \n \t\treturn data->cb(oid, &oi, data->cb_data);\ndiff --git a/odb/source-packed.c b/odb/source-packed.c\nindex 50e9be3b4c..bcd040aeb6 100644\n--- a/odb/source-packed.c\n+++ b/odb/source-packed.c\n@@ -2,7 +2,9 @@\n #include \"abspath.h\"\n #include \"chdir-notify.h\"\n #include \"dir.h\"\n+#include \"gettext.h\"\n #include \"git-zlib.h\"\n+#include \"hex.h\"\n #include \"list-objects-filter-options.h\"\n #include \"mergesort.h\"\n #include \"midx.h\"\n@@ -10,6 +12,7 @@\n #include \"odb/streaming.h\"\n #include \"packfile.h\"\n #include \"pack-bitmap.h\"\n+#include \"strbuf.h\"\n \n static int find_pack_entry(struct odb_source_packed *store,\n \t\t\t   const struct object_id *oid,\n@@ -38,7 +41,8 @@ static int find_pack_entry(struct odb_source_packed *store,\n static int odb_source_packed_read_object_info(struct odb_source *source,\n \t\t\t\t\t      const struct object_id *oid,\n \t\t\t\t\t      struct object_info *oi,\n-\t\t\t\t\t      enum object_info_flags flags)\n+\t\t\t\t\t      enum object_info_flags flags,\n+\t\t\t\t\t      struct strbuf *errmsg)\n {\n \tstruct odb_source_packed *packed = odb_source_packed_downcast(source);\n \tstruct packed_git *bad_pack = NULL;\n@@ -60,25 +64,39 @@ static int odb_source_packed_read_object_info(struct odb_source *source,\n \t\t * corresponding pack entries are skipped. Report the object\n \t\t * as corrupt instead of as missing in that case.\n \t\t */\n-\t\tif (bad_pack)\n-\t\t\treturn -1;\n-\t\treturn 1;\n+\t\tif (bad_pack) {\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\n+\t\tret = 1;\n+\t\tgoto out;\n \t}\n \n \t/*\n \t * We know that the caller doesn't actually need the\n \t * information below, so return early.\n \t */\n-\tif (!oi)\n-\t\treturn 0;\n+\tif (!oi) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n \n \tret = packed_object_info(packed, e.p, e.offset, oi);\n \tif (ret < 0) {\n+\t\tbad_pack = e.p;\n \t\tmark_bad_packed_object(e.p, oid);\n-\t\treturn -1;\n+\t\tgoto out;\n \t}\n \n-\treturn 0;\n+\tret = 0;\n+\n+out:\n+\tif (bad_pack && errmsg)\n+\t\tstrbuf_addf(errmsg, _(\"packed object %s (stored in %s) is corrupt\"),\n+\t\t\t    oid_to_hex(oid), bad_pack->pack_name);\n+\n+\treturn ret;\n }\n \n static int odb_source_packed_read_object_stream(struct odb_read_stream **out,\ndiff --git a/odb/source.h b/odb/source.h\nindex 4ae6cc160e..2b39f06166 100644\n--- a/odb/source.h\n+++ b/odb/source.h\n@@ -27,6 +27,7 @@ enum odb_source_type {\n \n struct object_id;\n struct odb_read_stream;\n+struct strbuf;\n struct strvec;\n \n /*\n@@ -121,11 +122,16 @@ struct odb_source {\n \t *   - A negative value in case the object exists in this source, but\n \t *     reading its object info has failed, for example because its\n \t *     on-disk state is corrupt.\n+\t *\n+\t * In case reading the object has failed and `errmsg` is non-NULL, the\n+\t * callback is expected to populate it with a human-readable message\n+\t * that describes the failure.\n \t */\n \tint (*read_object_info)(struct odb_source *source,\n \t\t\t\tconst struct object_id *oid,\n \t\t\t\tstruct object_info *oi,\n-\t\t\t\tenum object_info_flags flags);\n+\t\t\t\tenum object_info_flags flags,\n+\t\t\t\tstruct strbuf *errmsg);\n \n \t/*\n \t * This callback is expected to create a new read stream that can be\n@@ -352,13 +358,17 @@ static inline void odb_source_prepare(struct odb_source *source,\n  * Returns 0 on success, a positive value in case the object is missing in the\n  * source and a negative value in case the object exists, but reading it has\n  * failed.\n+ *\n+ * In case reading the object has failed and `errmsg` is non-NULL it will be\n+ * populated with a human-readable message that describes the failure.\n  */\n static inline int odb_source_read_object_info(struct odb_source *source,\n \t\t\t\t\t      const struct object_id *oid,\n \t\t\t\t\t      struct object_info *oi,\n-\t\t\t\t\t      enum object_info_flags flags)\n+\t\t\t\t\t      enum object_info_flags flags,\n+\t\t\t\t\t      struct strbuf *errmsg)\n {\n-\treturn source->read_object_info(source, oid, oi, flags);\n+\treturn source->read_object_info(source, oid, oi, flags, errmsg);\n }\n \n /*\ndiff --git a/packfile.c b/packfile.c\nindex 34e2f9bb8b..3cde39a01c 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1945,7 +1945,7 @@ int has_object_pack(struct repository *r, const struct object_id *oid)\n \todb_prepare_alternates(r->objects);\n \tfor (source = r->objects->sources; source; source = source->next) {\n \t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n-\t\tif (!odb_source_read_object_info(&files->packed->base, oid, NULL, 0))\n+\t\tif (!odb_source_read_object_info(&files->packed->base, oid, NULL, 0, NULL))\n \t\t\treturn 1;\n \t}\n \ndiff --git a/t/unit-tests/u-odb-inmemory.c b/t/unit-tests/u-odb-inmemory.c\nindex 93b3f38dab..102fc8db2f 100644\n--- a/t/unit-tests/u-odb-inmemory.c\n+++ b/t/unit-tests/u-odb-inmemory.c\n@@ -29,7 +29,7 @@ static void cl_assert_object_info(struct odb_source_inmemory *source,\n \t\t.contentp = &actual_content,\n \t};\n \n-\tcl_must_pass(odb_source_read_object_info(&source->base, oid, &oi, 0));\n+\tcl_must_pass(odb_source_read_object_info(&source->base, oid, &oi, 0, NULL));\n \tcl_assert_equal_u(actual_size, strlen(expected_content));\n \tcl_assert_equal_u(actual_type, expected_type);\n \tcl_assert_equal_s((char *) actual_content, expected_content);\n@@ -72,7 +72,7 @@ void test_odb_inmemory__read_missing_object(void)\n \tconst char *end;\n \n \tcl_must_pass(parse_oid_hex_algop(RANDOM_OID, &oid, &end, repo.hash_algo));\n-\tcl_assert(odb_source_read_object_info(&source->base, &oid, NULL, 0) > 0);\n+\tcl_assert(odb_source_read_object_info(&source->base, &oid, NULL, 0, NULL) > 0);\n \n \todb_source_free(&source->base);\n }\n\n-- \n2.55.0.822.g20453c30eb.dirty\n\n"},{"id":"550755","messageId":"20260818-pks-odb-generic-corrupt-objects-v1-7-ec234567510f@pks.im","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-0-ec234567510f@pks.im","subject":"[PATCH 7/7] odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-18T14:19:34Z","receivedAt":"2026-08-18T14:19:57Z","isPatch":true,"body":"When a lookup with `OBJECT_INFO_DIE_IF_CORRUPT` fails we want to die in\ncase the object exists, but cannot be read. This flag is handled in two\ndifferent spots right now:\n\n  - `do_oid_object_info_extended()` calls `has_packed_and_bad()` to\n    check whether the object is known to be corrupt in any packfile.\n    This function reaches into the internals of the packed source and\n    thus breaks the abstraction provided by our object sources.\n\n  - The loose source handles the flag itself and dies directly in\n    `read_object_info_from_path()`, which means that we die even in\n    cases where another source may still have a good copy of the\n    object.\n\nBesides being inconsistent, it also ties us to the specific backend used\nby the database sources because `has_packed_and_bad()` assumes that they\nuse the \"files\" backend. Any other backend will instead cause us to die\nwhen calling `odb_source_files_downcast()`, even if the object was\nsimply nonexistent.\n\nIn the preceding commits we've carved out the infrastructure to make\nthis mechanism fully generic. On the one hand, all backends now tell us\nwhether the object is missing or corrupt via their return values. And\non the other hand, they have been tought to provide a readable error\nmessage to the caller.\n\nAdapt `do_oid_object_info_extended()` to use those new mechanisms. This\nmeans that we won't die immediately anymore when a loose object is\ncorrupt, and we properly handle backends other than the \"files\" backend.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n odb.c                        | 46 ++++++++++++++++++++++++++++++--------------\n odb/source-loose.c           |  5 -----\n packfile.c                   | 17 ----------------\n packfile.h                   |  1 -\n t/t1060-object-corruption.sh | 18 +++++++++++++++++\n 5 files changed, 50 insertions(+), 37 deletions(-)\n\ndiff --git a/odb.c b/odb.c\nindex 6cb0a9534b..206988f39b 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -15,7 +15,6 @@\n #include \"object-name.h\"\n #include \"odb.h\"\n #include \"odb/source-inmemory.h\"\n-#include \"packfile.h\"\n #include \"path.h\"\n #include \"promisor-remote.h\"\n #include \"quote.h\"\n@@ -551,8 +550,11 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \t\t\t\t       const struct object_id *oid,\n \t\t\t\t       struct object_info *oi, unsigned flags)\n {\n+\tstruct strbuf corrupt_err = STRBUF_INIT;\n \tconst struct object_id *real = oid;\n \tint already_retried = 0;\n+\tbool corrupt = false;\n+\tint ret;\n \n \tif (flags & OBJECT_INFO_LOOKUP_REPLACE)\n \t\treal = lookup_replace_object(odb->repo, oid);\n@@ -568,9 +570,14 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \twhile (1) {\n \t\tstruct odb_source *source;\n \n-\t\tfor (source = odb->sources; source; source = source->next)\n-\t\t\tif (!odb_source_read_object_info(source, real, oi, flags, NULL))\n-\t\t\t\treturn 0;\n+\t\tfor (source = odb->sources; source; source = source->next) {\n+\t\t\tret = odb_source_read_object_info(source, real, oi, flags,\n+\t\t\t\t\t\t\t  corrupt_err.len ? NULL : &corrupt_err);\n+\t\t\tif (!ret)\n+\t\t\t\tgoto out;\n+\t\t\tif (ret < 0)\n+\t\t\t\tcorrupt = true;\n+\t\t}\n \n \t\t/*\n \t\t * When the object hasn't been found we try a second read and\n@@ -578,11 +585,15 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \t\t * caches or reload on-disk state.\n \t\t */\n \t\tif (!(flags & OBJECT_INFO_QUICK)) {\n-\t\t\tfor (source = odb->sources; source; source = source->next)\n-\t\t\t\tif (!odb_source_read_object_info(source, real, oi,\n-\t\t\t\t\t\t\t\t flags | OBJECT_INFO_SECOND_READ,\n-\t\t\t\t\t\t\t\t NULL))\n-\t\t\t\t\treturn 0;\n+\t\t\tfor (source = odb->sources; source; source = source->next) {\n+\t\t\t\tret = odb_source_read_object_info(source, real, oi,\n+\t\t\t\t\t\t\t\t  flags | OBJECT_INFO_SECOND_READ,\n+\t\t\t\t\t\t\t\t  corrupt_err.len ? NULL : &corrupt_err);\n+\t\t\t\tif (!ret)\n+\t\t\t\t\tgoto out;\n+\t\t\t\tif (ret < 0)\n+\t\t\t\t\tcorrupt = true;\n+\t\t\t}\n \t\t}\n \n \t\t/*\n@@ -605,16 +616,23 @@ static int do_oid_object_info_extended(struct object_database *odb,\n \t\t}\n \n \t\tif (flags & OBJECT_INFO_DIE_IF_CORRUPT) {\n-\t\t\tconst struct packed_git *p;\n \t\t\tif ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))\n \t\t\t\tdie(_(\"replacement %s not found for %s\"),\n \t\t\t\t    oid_to_hex(real), oid_to_hex(oid));\n-\t\t\tif ((p = has_packed_and_bad(odb->repo, real)))\n-\t\t\t\tdie(_(\"packed object %s (stored in %s) is corrupt\"),\n-\t\t\t\t    oid_to_hex(real), p->pack_name);\n+\t\t\tif (corrupt) {\n+\t\t\t\tif (corrupt_err.len)\n+\t\t\t\t\tdie(\"%s\", corrupt_err.buf);\n+\t\t\t\tdie(_(\"object %s is corrupt\"), oid_to_hex(real));\n+\t\t\t}\n \t\t}\n-\t\treturn -1;\n+\n+\t\tret = -1;\n+\t\tgoto out;\n \t}\n+\n+out:\n+\tstrbuf_release(&corrupt_err);\n+\treturn ret;\n }\n \n static int oid_object_info_convert(struct repository *r,\ndiff --git a/odb/source-loose.c b/odb/source-loose.c\nindex 3cee012a6d..8ca5a78858 100644\n--- a/odb/source-loose.c\n+++ b/odb/source-loose.c\n@@ -195,11 +195,6 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n \tif (ret < 0 && errmsg)\n \t\tstrbuf_addf(errmsg, _(\"loose object %s (stored in %s) is corrupt\"),\n \t\t\t    oid_to_hex(oid), path);\n-\n-\tif (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n-\t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n-\t\t    oid_to_hex(oid), path);\n-\n \tif (stream_to_end)\n \t\tgit_inflate_end(stream_to_end);\n \tif (map)\ndiff --git a/packfile.c b/packfile.c\nindex 3cde39a01c..cd38be088d 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -985,23 +985,6 @@ void mark_bad_packed_object(struct packed_git *p, const struct object_id *oid)\n \toidset_insert(&p->bad_objects, oid);\n }\n \n-const struct packed_git *has_packed_and_bad(struct repository *r,\n-\t\t\t\t\t    const struct object_id *oid)\n-{\n-\tstruct odb_source *source;\n-\n-\tfor (source = r->objects->sources; source; source = source->next) {\n-\t\tstruct odb_source_files *files = odb_source_files_downcast(source);\n-\t\tstruct packfile_list_entry *e;\n-\n-\t\tfor (e = files->packed->packs.head; e; e = e->next)\n-\t\t\tif (oidset_contains(&e->pack->bad_objects, oid))\n-\t\t\t\treturn e->pack;\n-\t}\n-\n-\treturn NULL;\n-}\n-\n off_t get_delta_base(struct packed_git *p,\n \t\t     struct pack_window **w_curs,\n \t\t     off_t *curpos,\ndiff --git a/packfile.h b/packfile.h\nindex 3229a6ed47..573fe003d0 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -329,7 +329,6 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source,\n \t\t\t\t      uint32_t *maybe_index_pos, struct object_info *oi);\n \n void mark_bad_packed_object(struct packed_git *, const struct object_id *);\n-const struct packed_git *has_packed_and_bad(struct repository *, const struct object_id *);\n \n int has_object_pack(struct repository *r, const struct object_id *oid);\n int has_object_kept_pack(struct repository *r, const struct object_id *oid,\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 502a5ea1c5..d2ef468b45 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -145,4 +145,22 @@ test_expect_success 'partial clone of corrupted repository' '\n \ttest_must_fail git -C corrupt-partial checkout --force\n '\n \n+test_expect_success 'corrupted loose commit can be read from alternate' '\n+\tgit init repo-a &&\n+\ttree=$(git -C repo-a write-tree) &&\n+\tcommit=$(git -C repo-a commit-tree $tree </dev/null) &&\n+\n+\tcp -r repo-a repo-b &&\n+\t(\n+\t\tcd repo-b &&\n+\t\techo ../../../repo-a/.git/objects >.git/objects/info/alternates &&\n+\t\tcorrupt_byte \"$commit\" 1\n+\t) &&\n+\n+\tgit -C repo-a cat-file -p \"$commit\" >expect &&\n+\tgit -C repo-b cat-file -p \"$commit\" >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_grep \"inflate: data stream error\" err\n+'\n+\n test_done\n\n-- \n2.55.0.822.g20453c30eb.dirty\n\n"},{"id":"550768","messageId":"xmqqh5krz4tz.fsf@gitster.g","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-1-ec234567510f@pks.im","subject":"Re: [PATCH 1/7] odb/source: discern missing and corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-18T18:00:40Z","receivedAt":"2026-08-18T18:00:43Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The `read_object_info()` callback of `struct odb_source` is documented\n> to return a negative error code in case reading the object has failed,\n> and zero otherwise. This is overly broad though, as there are two very\n> different kinds of failures:\n>\n>   - The object may not exist in the source at all.\n>\n>   - The object exists, but reading it has failed, for example because\n>     its on-disk state is corrupt.\n>\n> This distinction matters to callers: when an object is corrupt in one\n> source we may still find a good copy of it in another source, so we may\n> still be able to proceed with a given operation.\n>\n> The \"packed\" source already distinguishes these cases by returning a\n> positive value for missing objects and a negative value in case reading\n> the object has failed. But all the other sources conflate them into a\n> single negative return value.\n\nIn other words, \"packed\" did not honor the documented contract with\nthe callers and nobody noticed?  It gives us a usable escape hatch ;-)\n\nDo we need to support many other \"it is an error but we treat as non\nerror in some context\" values, like the \"does not exist\"?  If so, it\ndoes make sense to say 0 is absolute success, positive values are\nsuch half-errors, and negative values are absolute failures.  If\nnot, it would have been much nicer if \"you asked me about this\ninformation but there is no such object\" were still signalled as an\nerror (i.e., negative return value) that is distinct from other\nkinds of errors like I/O error (which also should be signalled by a\nnegative return value), instead of a positive value whose meanings\nwere not defined, though.\n\n> Adapt the documentation to explicitly require the semantics of the\n> \"packed\" backend, where we return a positive value for missing objects\n> and a negative value for corrupt ones. Subsequent commits will adapt all\n> the other implementations to respect those new semantics.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  odb/source.h | 17 ++++++++++++++---\n>  1 file changed, 14 insertions(+), 3 deletions(-)\n>\n> diff --git a/odb/source.h b/odb/source.h\n> index d69f8e2d1c..4ae6cc160e 100644\n> --- a/odb/source.h\n> +++ b/odb/source.h\n> @@ -110,8 +110,17 @@ struct odb_source {\n>  \t *     second read in case they know that the first read would have\n>  \t *     already surfaced the object without reloading any on-disk state.\n>  \t *\n> -\t * The callback is expected to return a negative error code in case\n> -\t * reading the object has failed, 0 otherwise.\n> +\t * The callback is expected to return one of the following values:\n> +\t *\n> +\t *   - Zero in case the object has been found and its object info has\n> +\t *     been read successfully.\n> +\t *\n> +\t *   - A positive value in case the object does not exist in this\n> +\t *     source.\n> +\t *\n> +\t *   - A negative value in case the object exists in this source, but\n> +\t *     reading its object info has failed, for example because its\n> +\t *     on-disk state is corrupt.\n>  \t */\n>  \tint (*read_object_info)(struct odb_source *source,\n>  \t\t\t\tconst struct object_id *oid,\n> @@ -340,7 +349,9 @@ static inline void odb_source_prepare(struct odb_source *source,\n>  \n>  /*\n>   * Read an object from the object database source identified by its object ID.\n> - * Returns 0 on success, a negative error code otherwise.\n> + * Returns 0 on success, a positive value in case the object is missing in the\n> + * source and a negative value in case the object exists, but reading it has\n> + * failed.\n>   */\n>  static inline int odb_source_read_object_info(struct odb_source *source,\n>  \t\t\t\t\t      const struct object_id *oid,\n"},{"id":"550769","messageId":"xmqqcxvfz4lu.fsf@gitster.g","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-2-ec234567510f@pks.im","subject":"Re: [PATCH 2/7] odb/source-inmemory: signal missing objects via positive return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-18T18:05:33Z","receivedAt":"2026-08-18T18:05:36Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The in-memory source returns a negative value from its\n> `read_object_info()` callback when the object in question does not\n> exist. Adapt the callback to return a positive value for missing objects\n> according to the new calling convention.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  odb/source-inmemory.c         | 2 +-\n>  t/unit-tests/u-odb-inmemory.c | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c\n> index 3e71611b8e..57183daf4d 100644\n> --- a/odb/source-inmemory.c\n> +++ b/odb/source-inmemory.c\n> @@ -66,7 +66,7 @@ static int odb_source_inmemory_read_object_info(struct odb_source *source,\n>  \n>  \tobject = find_cached_object(inmemory, oid);\n>  \tif (!object)\n> -\t\treturn -1;\n> +\t\treturn 1;\n\nLet's not define \"any positive value means this single thing: it\ndoes not exist\" and then return a mysterious and unspecified hard\ncoded constant like this.  Instead perhaps something along this\nline?\n\n    enum odb_roi_status {\n\tODB_ROI_SUCCESS = 0,\n\tODB_ROI_MISSING = 1,\n\tODB_ROI_IO_ERROR = -1,\n\t...\n    };\n\nAs I already said, I personally prefer to define MISSING also as\na negative value.\n"},{"id":"550770","messageId":"xmqq5x17z41g.fsf@gitster.g","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-3-ec234567510f@pks.im","subject":"Re: [PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-18T18:17:47Z","receivedAt":"2026-08-18T18:17:50Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When reading a packed object that doesn't verify we mark it as bad and\n> indicate to the caller that we failed reading the object despite the\n> fact that it supposedly exists. This matches the semantics we have now\n> established in a preceding commit, where we discern failure to read a\n> corrupt object from a missing object.\n>\n> What doesn't work yet though is when a call tries to read an object that\n> has already been marked as corrupt in a previous call. In that case,\n> `find_pack_entry()` will tell us that the object in question does not\n> exist, and consequently we'll not flag the object as corrupt but as\n> missing.\n\nThanks for attacking this one.  I've always felt it awkward that we\ntreat a corrupt/unreadable object as if we do not have it, and we\neven silently recover from it if we have another copy, making fsck\npractically the only thing that notices such breakages.\n\n>  int fill_midx_entry(struct multi_pack_index *m,\n>  \t\t    const struct object_id *oid,\n> -\t\t    struct pack_entry *e)\n> +\t\t    struct pack_entry *e,\n> +\t\t    struct packed_git **bad_pack)\n>  {\n>  \tuint32_t pos;\n>  \tuint32_t pack_int_id;\n> @@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,\n>  \t\treturn 0;\n>  \n>  \tif (oidset_size(&p->bad_objects) &&\n> -\t    oidset_contains(&p->bad_objects, oid))\n> +\t    oidset_contains(&p->bad_objects, oid)) {\n> +\t\tif (bad_pack && !*bad_pack)\n> +\t\t\t*bad_pack = p;\n>  \t\treturn 0;\n> +\t}\n\nHmph, so the idea is that if you have even one bad thing, you are\nmarked as bad, because who knows what other parts of you are broken?\n\n"},{"id":"550771","messageId":"xmqqzeyjxp7k.fsf@gitster.g","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-4-ec234567510f@pks.im","subject":"Re: [PATCH 4/7] odb/source-loose: distinguish missing and corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-18T18:23:27Z","receivedAt":"2026-08-18T18:23:39Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> @@ -91,11 +91,16 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n>  \t\tstruct stat st;\n>  \n>  \t\tif ((!oi || (!oi->disk_sizep && !oi->mtimep)) && (flags & OBJECT_INFO_QUICK)) {\n> -\t\t\tret = quick_has_loose(loose, oid) ? 0 : -1;\n> +\t\t\tret = quick_has_loose(loose, oid) ? 0 : 1;\n>  \t\t\tgoto out;\n>  \t\t}\n>  \n>  \t\tif (lstat(path, &st) < 0) {\n> +\t\t\tif (errno == ENOENT) {\n> +\t\t\t\tret = 1;\n> +\t\t\t\tgoto out;\n> +\t\t\t}\n> +\n>  \t\t\tret = -1;\n>  \t\t\tgoto out;\n\nExactly the same comment about \"turn it into an enum with meaningful\nnames once you add to an yes/no set a third choice\" applies here.\n\n> @@ -155,7 +163,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n>  \n>  \t\tif (parse_loose_header(hdr, oi) < 0) {\n>  \t\t\tret = error(_(\"unable to parse %s header\"), oid_to_hex(oid));\n> -\t\t\tgoto corrupt;\n> +\t\t\tgoto out;\n>  \t\t}\n>  \n>  \t\tif (*oi->typep < 0)\n> @@ -165,7 +173,7 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n>  \t\t\t*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);\n>  \t\t\tif (!*oi->contentp) {\n>  \t\t\t\tret = -1;\n> -\t\t\t\tgoto corrupt;\n> +\t\t\t\tgoto out;\n>  \t\t\t}\n>  \t\t}\n>  \n> @@ -173,21 +181,20 @@ static int read_object_info_from_path(struct odb_source_loose *loose,\n>  \tcase ULHR_BAD:\n>  \t\tret = error(_(\"unable to unpack %s header\"),\n>  \t\t\t    oid_to_hex(oid));\n> -\t\tgoto corrupt;\n> +\t\tgoto out;\n>  \tcase ULHR_TOO_LONG:\n>  \t\tret = error(_(\"header for %s too long, exceeds %d bytes\"),\n>  \t\t\t    oid_to_hex(oid), MAX_HEADER_LEN);\n> -\t\tgoto corrupt;\n> +\t\tgoto out;\n>  \t}\n>  \n>  \tret = 0;\n>  \n> -corrupt:\n> -\tif (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n> +out:\n> +\tif (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT))\n>  \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n>  \t\t    oid_to_hex(oid), path);\n\nA missing object is not necessarily repository corruption, and the\ncode path to deal with it needs to jump here, so naming the label\n\"out:\" is more appropriate.  OK.\n"},{"id":"550774","messageId":"xmqq8q63xnl2.fsf@gitster.g","threadId":"66190","inReplyTo":"20260818-pks-odb-generic-corrupt-objects-v1-5-ec234567510f@pks.im","subject":"Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-18T18:58:33Z","receivedAt":"2026-08-18T18:58:35Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Subject: Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return\n\n\"missing\" is what you meant intead of \"mark\".\n"},{"id":"550792","messageId":"aoV-6ClUIPYh_-OJ@pks.im","threadId":"66190","inReplyTo":"xmqqcxvfz4lu.fsf@gitster.g","subject":"Re: [PATCH 2/7] odb/source-inmemory: signal missing objects via positive return","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-19T10:01:12Z","receivedAt":"2026-08-19T10:01:25Z","isPatch":true,"body":"On Tue, Aug 18, 2026 at 11:05:33AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The in-memory source returns a negative value from its\n> > `read_object_info()` callback when the object in question does not\n> > exist. Adapt the callback to return a positive value for missing objects\n> > according to the new calling convention.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  odb/source-inmemory.c         | 2 +-\n> >  t/unit-tests/u-odb-inmemory.c | 2 +-\n> >  2 files changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c\n> > index 3e71611b8e..57183daf4d 100644\n> > --- a/odb/source-inmemory.c\n> > +++ b/odb/source-inmemory.c\n> > @@ -66,7 +66,7 @@ static int odb_source_inmemory_read_object_info(struct odb_source *source,\n> >  \n> >  \tobject = find_cached_object(inmemory, oid);\n> >  \tif (!object)\n> > -\t\treturn -1;\n> > +\t\treturn 1;\n> \n> Let's not define \"any positive value means this single thing: it\n> does not exist\" and then return a mysterious and unspecified hard\n> coded constant like this.  Instead perhaps something along this\n> line?\n> \n>     enum odb_roi_status {\n> \tODB_ROI_SUCCESS = 0,\n> \tODB_ROI_MISSING = 1,\n> \tODB_ROI_IO_ERROR = -1,\n> \t...\n>     };\n> \n> As I already said, I personally prefer to define MISSING also as\n> a negative value.\n\nFair enough, will adapt.\n\nPatrick\n"},{"id":"550793","messageId":"aoV--DSQq8-Krg3M@pks.im","threadId":"66190","inReplyTo":"xmqq5x17z41g.fsf@gitster.g","subject":"Re: [PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-19T10:01:28Z","receivedAt":"2026-08-19T10:01:35Z","isPatch":true,"body":"On Tue, Aug 18, 2026 at 11:17:47AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> > @@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,\n> >  \t\treturn 0;\n> >  \n> >  \tif (oidset_size(&p->bad_objects) &&\n> > -\t    oidset_contains(&p->bad_objects, oid))\n> > +\t    oidset_contains(&p->bad_objects, oid)) {\n> > +\t\tif (bad_pack && !*bad_pack)\n> > +\t\t\t*bad_pack = p;\n> >  \t\treturn 0;\n> > +\t}\n> \n> Hmph, so the idea is that if you have even one bad thing, you are\n> marked as bad, because who knows what other parts of you are broken?\n\nNo, not quite. We don't mark the whole pack itself as bad, we only mark\nthe objects that's contained in there as bad. The only reason why we\nalso bubble up the pack is so that we can provide a better error message\nin a subsequent commit, where we can then tell the user which pack it\nwas specifically that contains the bad commit.\n\nThat's by itself not visible in this commit yet, but I do mention it as\npart of the commit message.\n\nPatrick\n"},{"id":"550794","messageId":"aoV_AEqPDiEwNLZO@pks.im","threadId":"66190","inReplyTo":"xmqq8q63xnl2.fsf@gitster.g","subject":"Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-19T10:01:36Z","receivedAt":"2026-08-19T10:01:43Z","isPatch":true,"body":"On Tue, Aug 18, 2026 at 11:58:33AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Subject: Re: [PATCH 5/7] odb/source-files: signal mark objects via positive return\n> \n> \"missing\" is what you meant intead of \"mark\".\n\nD'oh, obviously. I've massaged this specific subject probably half a\ndozen times because I couldn't find a nice summary, and this here is the\nresult. Will fix.\n\nPatrick\n"},{"id":"550795","messageId":"aoV_C8MQsTZSDqX8@pks.im","threadId":"66190","inReplyTo":"xmqqh5krz4tz.fsf@gitster.g","subject":"Re: [PATCH 1/7] odb/source: discern missing and corrupt objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-19T10:01:47Z","receivedAt":"2026-08-19T10:01:52Z","isPatch":true,"body":"On Tue, Aug 18, 2026 at 11:00:40AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The `read_object_info()` callback of `struct odb_source` is documented\n> > to return a negative error code in case reading the object has failed,\n> > and zero otherwise. This is overly broad though, as there are two very\n> > different kinds of failures:\n> >\n> >   - The object may not exist in the source at all.\n> >\n> >   - The object exists, but reading it has failed, for example because\n> >     its on-disk state is corrupt.\n> >\n> > This distinction matters to callers: when an object is corrupt in one\n> > source we may still find a good copy of it in another source, so we may\n> > still be able to proceed with a given operation.\n> >\n> > The \"packed\" source already distinguishes these cases by returning a\n> > positive value for missing objects and a negative value in case reading\n> > the object has failed. But all the other sources conflate them into a\n> > single negative return value.\n> \n> In other words, \"packed\" did not honor the documented contract with\n> the callers and nobody noticed?  It gives us a usable escape hatch ;-)\n\nYes, kind of. It didn't matter much though, as the \"files\" backend\nknew to translate the positive value into a negative one.\n\n> Do we need to support many other \"it is an error but we treat as non\n> error in some context\" values, like the \"does not exist\"?  If so, it\n> does make sense to say 0 is absolute success, positive values are\n> such half-errors, and negative values are absolute failures.  If\n> not, it would have been much nicer if \"you asked me about this\n> information but there is no such object\" were still signalled as an\n> error (i.e., negative return value) that is distinct from other\n> kinds of errors like I/O error (which also should be signalled by a\n> negative return value), instead of a positive value whose meanings\n> were not defined, though.\n\nI cannot think of any other classes of errors where we'd want to fail\ngracefully from the top of my head. The only one that's potentially\nworth thinking about is in case an object disappears right while we are\nlooking at it. But that's basically just another edge case of a missing\nobject.\n\nIn any case, I think I'm aligned with the proposal to turn this into a\nproper enum and then use negative values exclusively. Thanks!\n\nPatrick\n"},{"id":"550833","messageId":"xmqqpkzeuhuz.fsf@gitster.g","threadId":"66190","inReplyTo":"aoV--DSQq8-Krg3M@pks.im","subject":"Re: [PATCH 3/7] odb/source-packed: flag known-bad objects as corrupt and not missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-19T17:42:44Z","receivedAt":"2026-08-19T17:42:47Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Tue, Aug 18, 2026 at 11:17:47AM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt <ps@pks.im> writes:\n>> > @@ -618,8 +619,11 @@ int fill_midx_entry(struct multi_pack_index *m,\n>> >  \t\treturn 0;\n>> >  \n>> >  \tif (oidset_size(&p->bad_objects) &&\n>> > -\t    oidset_contains(&p->bad_objects, oid))\n>> > +\t    oidset_contains(&p->bad_objects, oid)) {\n>> > +\t\tif (bad_pack && !*bad_pack)\n>> > +\t\t\t*bad_pack = p;\n>> >  \t\treturn 0;\n>> > +\t}\n>> \n>> Hmph, so the idea is that if you have even one bad thing, you are\n>> marked as bad, because who knows what other parts of you are broken?\n>\n> No, not quite. We don't mark the whole pack itself as bad, we only mark\n> the objects that's contained in there as bad. The only reason why we\n> also bubble up the pack is so that we can provide a better error message\n> in a subsequent commit, where we can then tell the user which pack it\n> was specifically that contains the bad commit.\n>\n> That's by itself not visible in this commit yet, but I do mention it as\n> part of the commit message.\n>\n> Patrick\n\nOK.\n\nThis is a tangent but the argument heavily relies on the invariant\nthat a single pack can contain one object at most once.  Once a\ncorrupt pack that has copies of the same object duplicated in it\ncomes into the picture, the error message has to say which copy is\nbad.\n\n"}]}