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

From: Taylor Blau <ttaylorr@openai.com>
Date: Sep 8, 2026, 04:09 UTC
Message-ID: <ap-KYtsDXXwbBzDM@com-79390>
In-Reply-To: <20260908014720.19705-1-pia@pierre.co>

On Mon, Sep 07, 2026 at 06:46:40PM -0700, Pia Park wrote:

Show 5 quoted lines

> 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".

Show 5 quoted lines

> 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.

Show 13 quoted lines

>
>     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.

Show 6 quoted lines

> 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