{"thread":{"id":"65500","subject":"[PATCH] midx: state what failed correctly","startedAt":"2026-04-16T20:33:25Z","lastAt":"2026-04-16T21:17:40Z","messageCount":2,"participants":["Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"541773","messageId":"xmqqik9qzlv0.fsf@gitster.g","threadId":"65500","inReplyTo":null,"subject":"[PATCH] midx: state what failed correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-16T20:33:23Z","receivedAt":"2026-04-16T20:33:25Z","isPatch":true,"body":"A helper function load_multi_pack_index_one() was introduced in\n4d80560c (multi-pack-index: load into memory, 2018-07-12) and is\nused to read either the multi-pack-index file or chained set of\nmulti-pack-index files.  For the former, the caller calls it without\neven knowing if such a file should exist (it is a totally optional\ncomponent in a repository and it is normal not to have it), but for\nthe latter, the names of these chained multi-pack-index files are\nread from a central catalog file.\n\nIn order to avoid complaining about missing the multi-pack-index\nfile, a failure to open(2) the given file by this helper function\nresults in a silent no-op return, but it means that there won't be\nany error if we fail to open a multi-pack-index file that is part of\na chain.\n\nGive an extra \"missing-ok\" parameter to the helper function, and\nreport a failure to open the named file, unless we are told that\nENOENT is OK.  If open() failed with an error other than ENOENT,\nwe will report failure to open the file unconditionally.\n\nWhile at it, we used to report failure to fstat(2) as \"failed to\nread\"; the fstat() is done to learn the size of the file, and not to\nread, so correct the message to say so.  We could say \"failed to\nfstat\", but that may not be a great end-user-facing message.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n midx.c | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git c/midx.c w/midx.c\nindex 81d6ab11e6..a6facc13a8 100644\n--- c/midx.c\n+++ w/midx.c\n@@ -107,7 +107,8 @@ struct multi_pack_index *get_multi_pack_index(struct odb_source *source)\n }\n \n static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *source,\n-\t\t\t\t\t\t\t  const char *midx_name)\n+\t\t\t\t\t\t\t  const char *midx_name,\n+\t\t\t\t\t\t\t  bool missing_ok)\n {\n \tstruct repository *r = source->odb->repo;\n \tstruct multi_pack_index *m = NULL;\n@@ -122,10 +123,13 @@ static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *sou\n \n \tfd = git_open(midx_name);\n \n-\tif (fd < 0)\n+\tif (fd < 0) {\n+\t\tif (!missing_ok || errno != ENOENT)\n+\t\t\terror_errno(_(\"failed to open %s\"), midx_name);\n \t\tgoto cleanup_fail;\n+\t}\n \tif (fstat(fd, &st)) {\n-\t\terror_errno(_(\"failed to read %s\"), midx_name);\n+\t\terror_errno(_(\"failed to learn the size of %s\"), midx_name);\n \t\tgoto cleanup_fail;\n \t}\n \n@@ -145,14 +149,18 @@ static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *sou\n \tm->source = source;\n \n \tm->signature = get_be32(m->data);\n-\tif (m->signature != MIDX_SIGNATURE)\n-\t\tdie(_(\"multi-pack-index signature 0x%08x does not match signature 0x%08x\"),\n+\tif (m->signature != MIDX_SIGNATURE) {\n+\t\terror(_(\"multi-pack-index signature 0x%08x does not match signature 0x%08x\"),\n \t\t      m->signature, MIDX_SIGNATURE);\n+\t\tgoto cleanup_fail;\n+\t}\n \n \tm->version = m->data[MIDX_BYTE_FILE_VERSION];\n-\tif (m->version != MIDX_VERSION_V1 && m->version != MIDX_VERSION_V2)\n-\t\tdie(_(\"multi-pack-index version %d not recognized\"),\n+\tif (m->version != MIDX_VERSION_V1 && m->version != MIDX_VERSION_V2) {\n+\t\terror(_(\"multi-pack-index version %d not recognized\"),\n \t\t      m->version);\n+\t\tgoto cleanup_fail;\n+\t}\n \n \thash_version = m->data[MIDX_BYTE_HASH_VERSION];\n \tif (hash_version != oid_version(r->hash_algo)) {\n@@ -339,7 +347,7 @@ static struct multi_pack_index *load_midx_chain_fd_st(struct odb_source *source,\n \t\tstrbuf_reset(&buf);\n \t\tget_split_midx_filename_ext(source, &buf,\n \t\t\t\t\t    layer.hash, MIDX_EXT_MIDX);\n-\t\tm = load_multi_pack_index_one(source, buf.buf);\n+\t\tm = load_multi_pack_index_one(source, buf.buf, 0);\n \n \t\tif (m) {\n \t\t\tif (add_midx_to_chain(m, midx_chain)) {\n@@ -387,7 +395,7 @@ struct multi_pack_index *load_multi_pack_index(struct odb_source *source)\n \n \tget_midx_filename(source, &midx_name);\n \n-\tm = load_multi_pack_index_one(source, midx_name.buf);\n+\tm = load_multi_pack_index_one(source, midx_name.buf, true);\n \tif (!m)\n \t\tm = load_multi_pack_index_chain(source);\n \n"},{"id":"541779","messageId":"aeFR8qOTBGA922eY@nand.local","threadId":"65500","inReplyTo":"xmqqik9qzlv0.fsf@gitster.g","subject":"Re: [PATCH] midx: state what failed correctly","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2026-04-16T21:17:38Z","receivedAt":"2026-04-16T21:17:40Z","isPatch":true,"body":"On Thu, Apr 16, 2026 at 01:33:23PM -0700, Junio C Hamano wrote:\n> ---\n>  midx.c | 26 +++++++++++++++++---------\n>  1 file changed, 17 insertions(+), 9 deletions(-)\n\nThe approach here seems very reasonable to me, and the implementation\nmatches it faithfully. I think that this makes sense to pick up, though\nI suspect that there are other quality-of-life fixes that we could write\non top, e.g., to suppress duplicate \"failed to load\"-like messages,\nwhich I recall having to deal with in the past.\n\nThe patch looks good to me, with one small nitpick:\n\n> @@ -339,7 +347,7 @@ static struct multi_pack_index *load_midx_chain_fd_st(struct odb_source *source,\n>  \t\tstrbuf_reset(&buf);\n>  \t\tget_split_midx_filename_ext(source, &buf,\n>  \t\t\t\t\t    layer.hash, MIDX_EXT_MIDX);\n> -\t\tm = load_multi_pack_index_one(source, buf.buf);\n> +\t\tm = load_multi_pack_index_one(source, buf.buf, 0);\n\nHere you specify \"missing_ok\" as \"0\", but...\n\n> @@ -387,7 +395,7 @@ struct multi_pack_index *load_multi_pack_index(struct odb_source *source)\n>\n>  \tget_midx_filename(source, &midx_name);\n>\n> -\tm = load_multi_pack_index_one(source, midx_name.buf);\n> +\tm = load_multi_pack_index_one(source, midx_name.buf, true);\n\nHere you specify it as \"true\". Given the above I would have expected \"1\"\nhere, but I think that this hunk is preferable, and the earlier one\nshould use \"false\" instead.\n\nThanks,\nTaylor\n"}]}