Re: [PATCH] midx-write: skip empty incremental layers

[Taylor Blau <ttaylorr@openai.com>]

On Mon, Sep 07, 2026 at 06:46:40PM -0700, Pia Park wrote:
> An incremental MIDX write can find new packs without finding any new
> objects, either because the packs are empty or because their objects
> are already indexed by an earlier layer.
>
> The existing early exit checks the number of packs, so these cases

First, thanks for working on this :-).

Second, what you wrote makes sense. It may be worth saying "[...] checks
*only* the number of packs", or "[...] but not the number of objects".

> publish a zero-object layer without a reverse index. A subsequent
> incremental write with --bitmap fails when loading that reverse index.
>
> Reproducible on master as of
> b8242b093d9e941a34460d715e3ce616a34ac3fe (2026-09-07), using:

Nit: we typically would abbreviate this using the "reference" pretty
formatter, as in:

    b8242b093d (The 23rd batch, 2026-09-07)

, but I think that it would be more interesting to include the commit
that introduced this breakage, which I would guess (though haven't
bisected) that we've had this bug at least as long as we've been able to
write incremental MIDXs. Though see below for perhaps an earlier origin.

>
>     git init --bare --object-format=sha1 empty.git &&
>     (
>         cd empty.git &&
>         git config midx.version 2 &&
>         git pack-objects objects/pack/pack </dev/null &&
>         git multi-pack-index write --incremental --bitmap &&
>         git multi-pack-index write --incremental --bitmap
>     )
>
> The first write succeeds but publishes the empty layer
> 3c8853aad425100c5ee2ee22209bb0bb3df9ca37. The second exits with status
> 255, reporting "could not load reverse index for MIDX".

Right. This patch message suggests (and I agree with) the fact that the
first layer wrote anything at all is a bug.

It only happened to work because the first invocation did not require
loading the empty reverse index, and so did not read the corruption that
it just wrote. The second invocation notices the bug because we eagerly
read reverse indexes for pack(s) in previous layer(s) when writing
reachability bitmaps.

That makes me wonder whether this bug is unique to incremental MIDXs at
all. I tried testing this out locally with:

    git.compile init --bare empty.git &&
    (
      cd empty.git &&

      git.compile pack-objects objects/pack/pack </dev/null &&
      git.compile multi-pack-index write
    )

, and it happily wrote a MIDX.

> Return success through the existing cleanup path when
> compute_sorted_entries() finds no entries for a non-compacting
> incremental write. This prevents publishing an empty layer that causes
> subsequent incremental writes with --bitmap to fail with exit status
> 255. Exit before acquiring a lock or creating a temporary MIDX file,
> leaving the existing chain untouched.

So I wonder if we should apply the fix even earlier in
write_midx_internal(), perhaps like:

--- 8< ---
git rev-parse 2>/dev/null || cd ~/src/git; git: line 0: cd: /Users/ttaylorr/src/git: No such file or directory
diff --git a/midx-write.c b/midx-write.c
index 580724d21a..5b2aa9acc8 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1617,9 +1617,8 @@ static int write_midx_internal(struct write_midx_opts *opts)
 	}

 	if (!ctx.entries_nr) {
-		if (opts->flags & MIDX_WRITE_BITMAP)
-			warning(_("refusing to write multi-pack .bitmap without any objects"));
-		opts->flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
+		error(_("no objects to index."));
+		goto cleanup;
 	}

 	if (ctx.incremental) {
--- >8 ---

(as an aside, we can probably rework those error messages to be a bit
more descriptive, perhaps, "cannot create a multi-pack-index without any
packs". But that is besides the point of your patch.)

> diff --git a/midx-write.c b/midx-write.c
> index 8537102254..cdb2ef0474 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1518,6 +1518,11 @@ static int write_midx_internal(struct write_midx_opts *opts)
>
>  	compute_sorted_entries(&ctx, start_pack);
>
> +	if (ctx.incremental && !ctx.compact && !ctx.entries_nr) {
> +		result = 0;
> +		goto cleanup;
> +	}
> +

Hmm. So we will avoid writing an empty MIDX when we have no object
entries, but only when doing a non-compact, incremental write? I imagine
that we would want similar treatment for both incremental and
non-incremental MIDXs, regardless of whether we are compacting.

> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
> index f0b82b5f65..4bdfa61d38 100755
> --- a/t/t5334-incremental-multi-pack-index.sh
> +++ b/t/t5334-incremental-multi-pack-index.sh

I suspect that these tests will change a bit, so I'll avoid reviewing
them too carefully for the time being. I am glad, however, that you are
testing cases besides explicitly empty packs, e.g., dropping objects
which are represented in earlier layers.

Thanks,
Taylor