Volume XXII, number 280Wednesday, October 7, 2026Latest message 1 hour ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchmidx: state what failed correctly

2 messages between Apr 16, 2026 and Apr 16, 2026, from Junio C Hamano, Taylor Blau.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Junio C HamanoApr 16, 2026, 20:33 UTC on lore

A helper function load_multi_pack_index_one() was introduced in 4d80560c (multi-pack-index: load into memory, 2018-07-12) and is used to read either the multi-pack-index file or chained set of multi-pack-index files. For the former, the caller calls it without even knowing if such a file should exist (it is a totally optional component in a repository and it is normal not to have it), but for the latter, the names of these chained multi-pack-index files are read from a central catalog file.

In order to avoid complaining about missing the multi-pack-index file, a failure to open(2) the given file by this helper function results in a silent no-op return, but it means that there won't be any error if we fail to open a multi-pack-index file that is part of a chain.

Give an extra "missing-ok" parameter to the helper function, and report a failure to open the named file, unless we are told that ENOENT is OK. If open() failed with an error other than ENOENT, we will report failure to open the file unconditionally.

While at it, we used to report failure to fstat(2) as "failed to read"; the fstat() is done to learn the size of the file, and not to read, so correct the message to say so. We could say "failed to fstat", but that may not be a great end-user-facing message.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 midx.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)
Show changes to diff +17 −9
diff --git c/midx.c w/midx.c
index 81d6ab11e6..a6facc13a8 100644
--- c/midx.c
+++ w/midx.c
@@ -107,7 +107,8 @@ struct multi_pack_index *get_multi_pack_index(struct odb_source *source)
 }
 
 static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *source,
-							  const char *midx_name)
+							  const char *midx_name,
+							  bool missing_ok)
 {
 	struct repository *r = source->odb->repo;
 	struct multi_pack_index *m = NULL;
@@ -122,10 +123,13 @@ static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *sou
 
 	fd = git_open(midx_name);
 
-	if (fd < 0)
+	if (fd < 0) {
+		if (!missing_ok || errno != ENOENT)
+			error_errno(_("failed to open %s"), midx_name);
 		goto cleanup_fail;
+	}
 	if (fstat(fd, &st)) {
-		error_errno(_("failed to read %s"), midx_name);
+		error_errno(_("failed to learn the size of %s"), midx_name);
 		goto cleanup_fail;
 	}
 
@@ -145,14 +149,18 @@ static struct multi_pack_index *load_multi_pack_index_one(struct odb_source *sou
 	m->source = source;
 
 	m->signature = get_be32(m->data);
-	if (m->signature != MIDX_SIGNATURE)
-		die(_("multi-pack-index signature 0x%08x does not match signature 0x%08x"),
+	if (m->signature != MIDX_SIGNATURE) {
+		error(_("multi-pack-index signature 0x%08x does not match signature 0x%08x"),
 		      m->signature, MIDX_SIGNATURE);
+		goto cleanup_fail;
+	}
 
 	m->version = m->data[MIDX_BYTE_FILE_VERSION];
-	if (m->version != MIDX_VERSION_V1 && m->version != MIDX_VERSION_V2)
-		die(_("multi-pack-index version %d not recognized"),
+	if (m->version != MIDX_VERSION_V1 && m->version != MIDX_VERSION_V2) {
+		error(_("multi-pack-index version %d not recognized"),
 		      m->version);
+		goto cleanup_fail;
+	}
 
 	hash_version = m->data[MIDX_BYTE_HASH_VERSION];
 	if (hash_version != oid_version(r->hash_algo)) {
@@ -339,7 +347,7 @@ static struct multi_pack_index *load_midx_chain_fd_st(struct odb_source *source,
 		strbuf_reset(&buf);
 		get_split_midx_filename_ext(source, &buf,
 					    layer.hash, MIDX_EXT_MIDX);
-		m = load_multi_pack_index_one(source, buf.buf);
+		m = load_multi_pack_index_one(source, buf.buf, 0);
 
 		if (m) {
 			if (add_midx_to_chain(m, midx_chain)) {
@@ -387,7 +395,7 @@ struct multi_pack_index *load_multi_pack_index(struct odb_source *source)
 
 	get_midx_filename(source, &midx_name);
 
-	m = load_multi_pack_index_one(source, midx_name.buf);
+	m = load_multi_pack_index_one(source, midx_name.buf, true);
 	if (!m)
 		m = load_multi_pack_index_chain(source);
 
Taylor BlauApr 16, 2026, 21:17 UTC in reply to Junio C Hamano on lore

Re: [PATCH] midx: state what failed correctly

On Thu, Apr 16, 2026 at 01:33:23PM -0700, Junio C Hamano wrote:
> ---
>  midx.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

The approach here seems very reasonable to me, and the implementation matches it faithfully. I think that this makes sense to pick up, though I suspect that there are other quality-of-life fixes that we could write on top, e.g., to suppress duplicate "failed to load"-like messages, which I recall having to deal with in the past.

The patch looks good to me, with one small nitpick:
Show 6 quoted lines
> @@ -339,7 +347,7 @@ static struct multi_pack_index *load_midx_chain_fd_st(struct odb_source *source,
>  		strbuf_reset(&buf);
>  		get_split_midx_filename_ext(source, &buf,
>  					    layer.hash, MIDX_EXT_MIDX);
> -		m = load_multi_pack_index_one(source, buf.buf);
> +		m = load_multi_pack_index_one(source, buf.buf, 0);
Here you specify "missing_ok" as "0", but...
Show 6 quoted lines
> @@ -387,7 +395,7 @@ struct multi_pack_index *load_multi_pack_index(struct odb_source *source)
>
>  	get_midx_filename(source, &midx_name);
>
> -	m = load_multi_pack_index_one(source, midx_name.buf);
> +	m = load_multi_pack_index_one(source, midx_name.buf, true);

Here you specify it as "true". Given the above I would have expected "1" here, but I think that this hunk is preferable, and the earlier one should use "false" instead.

Thanks, Taylor

Back to recent threads