{"thread":{"id":"66290","subject":"[PATCH] midx-write: skip empty incremental layers","startedAt":"2026-09-08T01:48:25Z","lastAt":"2026-09-15T14:25:19Z","messageCount":5,"participants":["Pia Park","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"552176","messageId":"20260908014720.19705-1-pia@pierre.co","threadId":"66290","inReplyTo":null,"subject":"[PATCH] midx-write: skip empty incremental layers","fromName":"Pia Park","fromEmail":"pia@pierre.co","sentAt":"2026-09-08T01:46:40Z","receivedAt":"2026-09-08T01:48:25Z","isPatch":true,"body":"An incremental MIDX write can find new packs without finding any new\nobjects, either because the packs are empty or because their objects\nare already indexed by an earlier layer.\n\nThe existing early exit checks the number of packs, so these cases\npublish a zero-object layer without a reverse index. A subsequent\nincremental write with --bitmap fails when loading that reverse index.\n\nReproducible on master as of\nb8242b093d9e941a34460d715e3ce616a34ac3fe (2026-09-07), using:\n\n    git init --bare --object-format=sha1 empty.git &&\n    (\n        cd empty.git &&\n        git config midx.version 2 &&\n        git pack-objects objects/pack/pack </dev/null &&\n        git multi-pack-index write --incremental --bitmap &&\n        git multi-pack-index write --incremental --bitmap\n    )\n\nThe first write succeeds but publishes the empty layer\n3c8853aad425100c5ee2ee22209bb0bb3df9ca37. The second exits with status\n255, reporting \"could not load reverse index for MIDX\".\n\nReturn success through the existing cleanup path when\ncompute_sorted_entries() finds no entries for a non-compacting\nincremental write. This prevents publishing an empty layer that causes\nsubsequent incremental writes with --bitmap to fail with exit status\n255. Exit before acquiring a lock or creating a temporary MIDX file,\nleaving the existing chain untouched.\n\nAdd regression tests for empty initial layers, empty packs, and packs\ncontaining only duplicate objects. Check silent success, preservation\nof the existing chain and file list, and subsequent writes that add\nnew objects.\n\nSigned-off-by: Pia Park <pia@pierre.co>\n---\nBuilt with DEVELOPER=1 and ran these scripts before and after:\n\n  t5319-multi-pack-index.sh\n  t5326-multi-pack-bitmaps.sh\n  t5327-multi-pack-bitmaps-rev.sh\n  t5332-multi-pack-reuse.sh\n  t5334-incremental-multi-pack-index.sh\n  t5335-compact-multi-pack-index.sh\n  t7700-repack.sh\n  t7705-repack-incremental-midx.sh\n\nAll 881 baseline tests and all 884 tests with the patch passed. The\nthree new regressions fail on the unpatched build. Test lint and\nwhitespace checks passed.\n\nBoth empty and duplicate-only pack reproductions also fail on a fresh,\nunmodified build of upstream master at the base commit shown below.\n\n midx-write.c                            |  5 +++\n t/t5334-incremental-multi-pack-index.sh | 58 +++++++++++++++++++++++++\n 2 files changed, 63 insertions(+)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 8537102254..cdb2ef0474 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1518,6 +1518,11 @@ static int write_midx_internal(struct write_midx_opts *opts)\n \n \tcompute_sorted_entries(&ctx, start_pack);\n \n+\tif (ctx.incremental && !ctx.compact && !ctx.entries_nr) {\n+\t\tresult = 0;\n+\t\tgoto cleanup;\n+\t}\n+\n \tctx.large_offsets_needed = 0;\n \tfor (size_t i = 0; i < ctx.entries_nr; i++) {\n \t\tif (ctx.entries[i].offset > 0x7fffffff)\ndiff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\nindex f0b82b5f65..4bdfa61d38 100755\n--- a/t/t5334-incremental-multi-pack-index.sh\n+++ b/t/t5334-incremental-multi-pack-index.sh\n@@ -195,4 +195,62 @@ test_expect_success 'non-incremental write with existing incremental chain' '\n \t)\n '\n \n+test_expect_success 'skip initial MIDX layer with no objects' '\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\tgit config maintenance.auto false &&\n+\t\tgit pack-objects $packdir/pack </dev/null &&\n+\n+\t\tfor bitmap in --bitmap --no-bitmap\n+\t\tdo\n+\t\t\tgit multi-pack-index write --incremental \"$bitmap\" >out 2>err &&\n+\t\t\ttest_must_be_empty out &&\n+\t\t\ttest_must_be_empty err &&\n+\t\t\ttest_dir_is_empty \"$midxdir\" || return 1\n+\t\tdone &&\n+\n+\t\twrite_midx_layer &&\n+\t\ttest_line_count = 1 \"$midx_chain\" &&\n+\t\tgit multi-pack-index verify\n+\t)\n+'\n+\n+for pack in empty duplicate\n+do\n+\ttest_expect_success \"skip MIDX layer with $pack pack\" '\n+\t\tgit init \"$pack-pack\" &&\n+\t\t(\n+\t\t\tcd \"$pack-pack\" &&\n+\t\t\tgit config maintenance.auto false &&\n+\t\t\twrite_midx_layer &&\n+\n+\t\t\tif test \"$pack\" = duplicate\n+\t\t\tthen\n+\t\t\t\tgit rev-parse HEAD^{tree} >in\n+\t\t\telse\n+\t\t\t\t>in\n+\t\t\tfi &&\n+\t\t\tgit pack-objects $packdir/pack <in &&\n+\t\t\tcp \"$midx_chain\" chain.expect &&\n+\t\t\tls \"$packdir\" \"$midxdir\" >files.expect &&\n+\n+\t\t\tfor bitmap in --bitmap --no-bitmap\n+\t\t\tdo\n+\t\t\t\tgit multi-pack-index write --incremental \"$bitmap\" >out 2>err &&\n+\t\t\t\ttest_must_be_empty out &&\n+\t\t\t\ttest_must_be_empty err &&\n+\t\t\t\ttest_cmp chain.expect \"$midx_chain\" &&\n+\t\t\t\tls \"$packdir\" \"$midxdir\" >files.actual &&\n+\t\t\t\ttest_cmp files.expect files.actual || return 1\n+\t\t\tdone &&\n+\n+\t\t\twrite_midx_layer &&\n+\t\t\ttest_line_count = 2 \"$midx_chain\" &&\n+\t\t\tgit multi-pack-index verify &&\n+\t\t\tgit rev-list --test-bitmap 2.2\n+\t\t)\n+\t'\n+done\n+\n test_done\n\nbase-commit: b8242b093d9e941a34460d715e3ce616a34ac3fe\n-- \n2.55.0\n\n"},{"id":"552180","messageId":"ap-KYtsDXXwbBzDM@com-79390","threadId":"66290","inReplyTo":"20260908014720.19705-1-pia@pierre.co","subject":"Re: [PATCH] midx-write: skip empty incremental layers","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-09-08T04:09:06Z","receivedAt":"2026-09-08T04:09:17Z","isPatch":true,"body":"On Mon, Sep 07, 2026 at 06:46:40PM -0700, Pia Park wrote:\n> An incremental MIDX write can find new packs without finding any new\n> objects, either because the packs are empty or because their objects\n> are already indexed by an earlier layer.\n>\n> The existing early exit checks the number of packs, so these cases\n\nFirst, thanks for working on this :-).\n\nSecond, what you wrote makes sense. It may be worth saying \"[...] checks\n*only* the number of packs\", or \"[...] but not the number of objects\".\n\n> publish a zero-object layer without a reverse index. A subsequent\n> incremental write with --bitmap fails when loading that reverse index.\n>\n> Reproducible on master as of\n> b8242b093d9e941a34460d715e3ce616a34ac3fe (2026-09-07), using:\n\nNit: we typically would abbreviate this using the \"reference\" pretty\nformatter, as in:\n\n    b8242b093d (The 23rd batch, 2026-09-07)\n\n, but I think that it would be more interesting to include the commit\nthat introduced this breakage, which I would guess (though haven't\nbisected) that we've had this bug at least as long as we've been able to\nwrite incremental MIDXs. Though see below for perhaps an earlier origin.\n\n>\n>     git init --bare --object-format=sha1 empty.git &&\n>     (\n>         cd empty.git &&\n>         git config midx.version 2 &&\n>         git pack-objects objects/pack/pack </dev/null &&\n>         git multi-pack-index write --incremental --bitmap &&\n>         git multi-pack-index write --incremental --bitmap\n>     )\n>\n> The first write succeeds but publishes the empty layer\n> 3c8853aad425100c5ee2ee22209bb0bb3df9ca37. The second exits with status\n> 255, reporting \"could not load reverse index for MIDX\".\n\nRight. This patch message suggests (and I agree with) the fact that the\nfirst layer wrote anything at all is a bug.\n\nIt only happened to work because the first invocation did not require\nloading the empty reverse index, and so did not read the corruption that\nit just wrote. The second invocation notices the bug because we eagerly\nread reverse indexes for pack(s) in previous layer(s) when writing\nreachability bitmaps.\n\nThat makes me wonder whether this bug is unique to incremental MIDXs at\nall. I tried testing this out locally with:\n\n    git.compile init --bare empty.git &&\n    (\n      cd empty.git &&\n\n      git.compile pack-objects objects/pack/pack </dev/null &&\n      git.compile multi-pack-index write\n    )\n\n, and it happily wrote a MIDX.\n\n> Return success through the existing cleanup path when\n> compute_sorted_entries() finds no entries for a non-compacting\n> incremental write. This prevents publishing an empty layer that causes\n> subsequent incremental writes with --bitmap to fail with exit status\n> 255. Exit before acquiring a lock or creating a temporary MIDX file,\n> leaving the existing chain untouched.\n\nSo I wonder if we should apply the fix even earlier in\nwrite_midx_internal(), perhaps like:\n\n--- 8< ---\ngit rev-parse 2>/dev/null || cd ~/src/git; git: line 0: cd: /Users/ttaylorr/src/git: No such file or directory\ndiff --git a/midx-write.c b/midx-write.c\nindex 580724d21a..5b2aa9acc8 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1617,9 +1617,8 @@ static int write_midx_internal(struct write_midx_opts *opts)\n \t}\n\n \tif (!ctx.entries_nr) {\n-\t\tif (opts->flags & MIDX_WRITE_BITMAP)\n-\t\t\twarning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n-\t\topts->flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n+\t\terror(_(\"no objects to index.\"));\n+\t\tgoto cleanup;\n \t}\n\n \tif (ctx.incremental) {\n--- >8 ---\n\n(as an aside, we can probably rework those error messages to be a bit\nmore descriptive, perhaps, \"cannot create a multi-pack-index without any\npacks\". But that is besides the point of your patch.)\n\n> diff --git a/midx-write.c b/midx-write.c\n> index 8537102254..cdb2ef0474 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1518,6 +1518,11 @@ static int write_midx_internal(struct write_midx_opts *opts)\n>\n>  \tcompute_sorted_entries(&ctx, start_pack);\n>\n> +\tif (ctx.incremental && !ctx.compact && !ctx.entries_nr) {\n> +\t\tresult = 0;\n> +\t\tgoto cleanup;\n> +\t}\n> +\n\nHmm. So we will avoid writing an empty MIDX when we have no object\nentries, but only when doing a non-compact, incremental write? I imagine\nthat we would want similar treatment for both incremental and\nnon-incremental MIDXs, regardless of whether we are compacting.\n\n> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\n> index f0b82b5f65..4bdfa61d38 100755\n> --- a/t/t5334-incremental-multi-pack-index.sh\n> +++ b/t/t5334-incremental-multi-pack-index.sh\n\nI suspect that these tests will change a bit, so I'll avoid reviewing\nthem too carefully for the time being. I am glad, however, that you are\ntesting cases besides explicitly empty packs, e.g., dropping objects\nwhich are represented in earlier layers.\n\nThanks,\nTaylor\n"},{"id":"552185","messageId":"eef33827000cf106544174ed000129c2989af1cd.1788851232.git.pia@pierre.co","threadId":"66290","inReplyTo":"ap-KYtsDXXwbBzDM@com-79390","subject":"[PATCH v2] midx-write: skip writes with no object entries","fromName":"Pia Park","fromEmail":"pia@pierre.co","sentAt":"2026-09-08T07:10:16Z","receivedAt":"2026-09-08T07:10:50Z","isPatch":true,"body":"A MIDX write can find packs without finding any objects, either because\nthe packs are empty or because an incremental write finds only objects\nalready indexed by an earlier layer. The existing early exit checks only\nthe number of packs, so these cases publish a zero-object MIDX without a\nreverse index.\n\nEmpty MIDXs were already published by 662148c435 (midx: write object\noffsets, 2018-07-12); the failure mode appeared in 27afc272c4 (midx:\nimplement writing incremental MIDX bitmaps, 2025-03-20), when\nincremental bitmap writes began loading reverse indexes from prior\nlayers.\n\nReproducible on master as of b8242b093d (The 23rd batch, 2026-09-07),\nstarting with a non-incremental write:\n\n    git init --bare --object-format=sha1 empty.git &&\n    (\n        cd empty.git &&\n        git pack-objects objects/pack/pack </dev/null &&\n        git multi-pack-index write &&\n        git multi-pack-index write --incremental --bitmap\n    )\n\nThe first write succeeds but publishes an empty MIDX, which also fails\nverification with \"the midx contains no oid\". The second write exits\nwith status 255, reporting \"could not load reverse index for MIDX\".\nStarting with an incremental write has the same problem.\n\nExit before publication whenever the computed entry list is empty, for\nboth non-incremental and incremental writes, including compaction. Take\nthe existing cleanup path after pack/drop validation and before\nacquiring a lock or creating a temporary MIDX file. This leaves existing\nMIDX files untouched and preserves the error when there are no pack\nfiles to index.\n\nReturn success silently. Empty-object writes already return 0, including\nwhen --bitmap warns, so preserve that exit status for existing callers\nwhile omitting the warning and empty MIDX.\n\nTest empty packs in a bare repository, empty incremental layers, and\npacks containing only objects indexed by an earlier layer. Check silent\nsuccess, preservation of existing files, and subsequent writes that add\nnew objects. Update the existing bitmap test to expect neither an empty\nMIDX nor a bitmap.\n\nSigned-off-by: Pia Park <pia@pierre.co>\n---\nThanks, Taylor. I kept exit status 0 because empty-object writes\nalready succeed, including when --bitmap emits its warning. Would you\nprefer an error here instead? The existing no-pack-files error is\nunchanged.\n\nChanges since v1:\n\n* Apply the check to all write modes, including compaction, at the\n  location in your sketch, after pack/drop validation.\n* Preserve exit status 0 and skip empty publication silently.\n* Add the non-incremental bare-repository reproduction and check later\n  incremental and non-incremental bitmap writes with real objects.\n* Update t5326's existing test that expected an empty MIDX and a warning.\n  Keep explicit empty-pack and duplicate-only incremental tests.\n* Identify the verified 2018 empty-MIDX behavior and the 2025 introduction\n  of the reverse-index failure, using reference-format citations.\n\nThe implementation passed 885 tests across the eight MIDX/bitmap/repack\nscripts. After the test-only revisions, all 481 tests in the three\nchanged scripts passed, along with test lint and whitespace checks.\n\n midx-write.c                            |  5 +-\n t/t5319-multi-pack-index.sh             | 35 ++++++++++++\n t/t5326-multi-pack-bitmaps.sh           | 13 +++--\n t/t5334-incremental-multi-pack-index.sh | 75 +++++++++++++++++++++++++\n 4 files changed, 119 insertions(+), 9 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 8537102254..3038bbfad2 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1617,9 +1617,8 @@ static int write_midx_internal(struct write_midx_opts *opts)\n \t}\n \n \tif (!ctx.entries_nr) {\n-\t\tif (opts->flags & MIDX_WRITE_BITMAP)\n-\t\t\twarning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n-\t\topts->flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n+\t\tresult = 0;\n+\t\tgoto cleanup;\n \t}\n \n \tif (ctx.incremental) {\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 68143cb5b7..c239a87d10 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -54,6 +54,41 @@ test_expect_success \"don't write midx with no packs\" '\n \ttest_path_is_missing pack/multi-pack-index\n '\n \n+test_expect_success 'skip non-incremental MIDX with no objects' '\n+\tgit init --bare empty.git &&\n+\t(\n+\t\tcd empty.git &&\n+\t\tgit pack-objects objects/pack/pack </dev/null &&\n+\t\tls objects/pack >files.expect &&\n+\n+\t\tfor bitmap in \"\" --bitmap\n+\t\tdo\n+\t\t\tgit multi-pack-index write $bitmap >out 2>&1 &&\n+\t\t\ttest_must_be_empty out &&\n+\t\t\ttest_path_is_missing objects/pack/multi-pack-index &&\n+\t\t\tls objects/pack >files.actual &&\n+\t\t\ttest_cmp files.expect files.actual || return 1\n+\t\tdone &&\n+\n+\t\tgit multi-pack-index write --incremental --bitmap &&\n+\t\ttest_dir_is_empty objects/pack/multi-pack-index.d &&\n+\n+\t\techo blob | git hash-object -w --stdin >in &&\n+\t\tgit pack-objects objects/pack/pack <in &&\n+\t\tgit multi-pack-index write --incremental --bitmap &&\n+\t\ttest_line_count = 1 objects/pack/multi-pack-index.d/multi-pack-index-chain &&\n+\t\tgit multi-pack-index verify &&\n+\n+\t\techo another | git hash-object -w --stdin >in &&\n+\t\tgit pack-objects objects/pack/pack <in &&\n+\t\tgit multi-pack-index write --bitmap &&\n+\t\ttest_path_is_file objects/pack/multi-pack-index &&\n+\t\tmidx=\"$(midx_checksum objects)\" &&\n+\t\ttest_path_is_file objects/pack/multi-pack-index-$midx.bitmap &&\n+\t\tgit multi-pack-index verify\n+\t)\n+'\n+\n test_expect_success SHA1 'warn if a midx contains no oid' '\n \tcp \"$TEST_DIRECTORY\"/t5319/no-objects.midx $objdir/pack/multi-pack-index &&\n \ttest_must_fail git multi-pack-index verify &&\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 86beab1dae..490008d1d7 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -305,7 +305,7 @@ test_midx_bitmap_cases () {\n \t\t)\n \t'\n \n-\ttest_expect_success 'no .bitmap is written without any objects' '\n+\ttest_expect_success 'no MIDX or .bitmap is written without any objects' '\n \t\trm -fr repo &&\n \t\tgit init repo &&\n \t\ttest_when_finished \"rm -fr repo\" &&\n@@ -318,13 +318,14 @@ test_midx_bitmap_cases () {\n \t\t\tpack-$empty.idx\n \t\t\tEOF\n \n+\t\t\tls $objdir/pack >files.expect &&\n \t\t\tgit multi-pack-index write --bitmap --stdin-packs \\\n-\t\t\t\t<packs 2>err &&\n+\t\t\t\t<packs >out 2>&1 &&\n \n-\t\t\ttest_grep \"bitmap without any objects\" err &&\n-\n-\t\t\ttest_path_is_file $midx &&\n-\t\t\ttest_path_is_missing $midx-$(midx_checksum $objdir).bitmap\n+\t\t\ttest_must_be_empty out &&\n+\t\t\ttest_path_is_missing $midx &&\n+\t\t\tls $objdir/pack >files.actual &&\n+\t\t\ttest_cmp files.expect files.actual\n \t\t)\n \t'\n \ndiff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\nindex f0b82b5f65..fbcc19feeb 100755\n--- a/t/t5334-incremental-multi-pack-index.sh\n+++ b/t/t5334-incremental-multi-pack-index.sh\n@@ -195,4 +195,79 @@ test_expect_success 'non-incremental write with existing incremental chain' '\n \t)\n '\n \n+test_expect_success 'skip initial MIDX layer with no objects' '\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\tgit config maintenance.auto false &&\n+\t\tgit pack-objects $packdir/pack </dev/null &&\n+\n+\t\tfor bitmap in --bitmap --no-bitmap\n+\t\tdo\n+\t\t\tgit multi-pack-index write --incremental \"$bitmap\" >out 2>&1 &&\n+\t\t\ttest_must_be_empty out &&\n+\t\t\ttest_dir_is_empty \"$midxdir\" || return 1\n+\t\tdone &&\n+\n+\t\twrite_midx_layer &&\n+\t\ttest_line_count = 1 \"$midx_chain\" &&\n+\t\tgit multi-pack-index verify\n+\t)\n+'\n+\n+test_expect_success 'skip MIDX layer with empty pack' '\n+\tgit init empty-pack &&\n+\t(\n+\t\tcd empty-pack &&\n+\t\tgit config maintenance.auto false &&\n+\t\twrite_midx_layer &&\n+\n+\t\tgit pack-objects $packdir/pack </dev/null &&\n+\t\tcp \"$midx_chain\" chain.expect &&\n+\t\tls \"$packdir\" \"$midxdir\" >files.expect &&\n+\n+\t\tfor bitmap in --bitmap --no-bitmap\n+\t\tdo\n+\t\t\tgit multi-pack-index write --incremental \"$bitmap\" >out 2>&1 &&\n+\t\t\ttest_must_be_empty out &&\n+\t\t\ttest_cmp chain.expect \"$midx_chain\" &&\n+\t\t\tls \"$packdir\" \"$midxdir\" >files.actual &&\n+\t\t\ttest_cmp files.expect files.actual || return 1\n+\t\tdone &&\n+\n+\t\twrite_midx_layer &&\n+\t\ttest_line_count = 2 \"$midx_chain\" &&\n+\t\tgit multi-pack-index verify &&\n+\t\tgit rev-list --test-bitmap 2.2\n+\t)\n+'\n+\n+test_expect_success 'skip MIDX layer with duplicate pack' '\n+\tgit init duplicate-pack &&\n+\t(\n+\t\tcd duplicate-pack &&\n+\t\tgit config maintenance.auto false &&\n+\t\twrite_midx_layer &&\n+\n+\t\tgit rev-parse HEAD^{tree} >in &&\n+\t\tgit pack-objects $packdir/pack <in &&\n+\t\tcp \"$midx_chain\" chain.expect &&\n+\t\tls \"$packdir\" \"$midxdir\" >files.expect &&\n+\n+\t\tfor bitmap in --bitmap --no-bitmap\n+\t\tdo\n+\t\t\tgit multi-pack-index write --incremental \"$bitmap\" >out 2>&1 &&\n+\t\t\ttest_must_be_empty out &&\n+\t\t\ttest_cmp chain.expect \"$midx_chain\" &&\n+\t\t\tls \"$packdir\" \"$midxdir\" >files.actual &&\n+\t\t\ttest_cmp files.expect files.actual || return 1\n+\t\tdone &&\n+\n+\t\twrite_midx_layer &&\n+\t\ttest_line_count = 2 \"$midx_chain\" &&\n+\t\tgit multi-pack-index verify &&\n+\t\tgit rev-list --test-bitmap 2.2\n+\t)\n+'\n+\n test_done\n"},{"id":"552216","messageId":"aqAkfGZtLJ97nG1m@com-79390","threadId":"66290","inReplyTo":"eef33827000cf106544174ed000129c2989af1cd.1788851232.git.pia@pierre.co","subject":"Re: [PATCH v2] midx-write: skip writes with no object entries","fromName":"Taylor Blau","fromEmail":"ttaylorr@openai.com","sentAt":"2026-09-08T15:06:36Z","receivedAt":"2026-09-08T15:06:48Z","isPatch":true,"body":"On Tue, Sep 08, 2026 at 12:10:16AM -0700, Pia Park wrote:\n> Return success silently. Empty-object writes already return 0, including\n> when --bitmap warns, so preserve that exit status for existing callers\n> while omitting the warning and empty MIDX.\n\nI think that this is OK, and it matches the behavior of other builtins,\ne.g., running \"git repack -d\" twice in a row such that the second\ninvocation has no objects to pack. However, I think that if we want to\nmake this case return successfully when no objects are present, we\nshould apply the same treatment to the case where no packs are present.\n\nBut I want to make sure that others are on the same page. I would be\ncurious to hear Stolee's (CC'd) opinion on whether returning silent\nsuccess in both cases makes sense.\n\n> diff --git a/midx-write.c b/midx-write.c\n> index 8537102254..3038bbfad2 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1617,9 +1617,8 @@ static int write_midx_internal(struct write_midx_opts *opts)\n>  \t}\n>\n>  \tif (!ctx.entries_nr) {\n> -\t\tif (opts->flags & MIDX_WRITE_BITMAP)\n> -\t\t\twarning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n> -\t\topts->flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n> +\t\tresult = 0;\n> +\t\tgoto cleanup;\n\nLooks good, though let's make sure others agree that this is the right\napproach. If they do, I'd recommend changing the no packs case to also\nreturn zero either in a small preparatory patch.\n\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 68143cb5b7..c239a87d10 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -54,6 +54,41 @@ test_expect_success \"don't write midx with no packs\" '\n>  \ttest_path_is_missing pack/multi-pack-index\n>  '\n>\n> +test_expect_success 'skip non-incremental MIDX with no objects' '\n> +\tgit init --bare empty.git &&\n> +\t(\n> +\t\tcd empty.git &&\n> +\t\tgit pack-objects objects/pack/pack </dev/null &&\n> +\t\tls objects/pack >files.expect &&\n\nI think it's fine to drop files.expect and files.actual here. Testing\nthat the MIDX write does nothing should be sufficient here.\n\n> +\n> +\t\tfor bitmap in \"\" --bitmap\n> +\t\tdo\n> +\t\t\tgit multi-pack-index write $bitmap >out 2>&1 &&\n> +\t\t\ttest_must_be_empty out &&\n\nI think it's fine to write \"git multi-pack-index write $bitmap\" without\nthe redirection, so that this is:\n\n    for opt in \"\" --bitmap\n    do\n        git multi-pack-index write $opt &&\n        test_path_is_missing $objdir/pack/multi-pack-index || return 1\n    done &&\n\n\n> +\t\tgit multi-pack-index write --incremental --bitmap &&\n> +\t\ttest_dir_is_empty objects/pack/multi-pack-index.d &&\n\nI was going to ask whether we wanted to test this case with and without\nthe \"--bitmap\" option as well, and likewise recommend that tthis test go\nin t5334 instead. But such a pair of tests already exists in t5334, so I\nthink we can safely drop this hunk.\n\n> +\t\techo blob | git hash-object -w --stdin >in &&\n> +\t\tgit pack-objects objects/pack/pack <in &&\n> +\t\tgit multi-pack-index write --incremental --bitmap &&\n> +\t\ttest_line_count = 1 objects/pack/multi-pack-index.d/multi-pack-index-chain &&\n> +\t\tgit multi-pack-index verify &&\n> +\n> +\t\techo another | git hash-object -w --stdin >in &&\n> +\t\tgit pack-objects objects/pack/pack <in &&\n> +\t\tgit multi-pack-index write --bitmap &&\n> +\t\ttest_path_is_file objects/pack/multi-pack-index &&\n> +\t\tmidx=\"$(midx_checksum objects)\" &&\n> +\t\ttest_path_is_file objects/pack/multi-pack-index-$midx.bitmap &&\n> +\t\tgit multi-pack-index verify\n\nThese two blocks are testing normal MIDX operations that are well\ncovered elsewhere in the test suite. I think we can drop these safely.\n\n> +\t)\n> +'\n> +\n>  test_expect_success SHA1 'warn if a midx contains no oid' '\n>  \tcp \"$TEST_DIRECTORY\"/t5319/no-objects.midx $objdir/pack/multi-pack-index &&\n>  \ttest_must_fail git multi-pack-index verify &&\n> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n> index 86beab1dae..490008d1d7 100755\n> --- a/t/t5326-multi-pack-bitmaps.sh\n> +++ b/t/t5326-multi-pack-bitmaps.sh\n> @@ -305,7 +305,7 @@ test_midx_bitmap_cases () {\n>  \t\t)\n>  \t'\n>\n> -\ttest_expect_success 'no .bitmap is written without any objects' '\n> +\ttest_expect_success 'no MIDX or .bitmap is written without any objects' '\n>  \t\trm -fr repo &&\n>  \t\tgit init repo &&\n>  \t\ttest_when_finished \"rm -fr repo\" &&\n> @@ -318,13 +318,14 @@ test_midx_bitmap_cases () {\n>  \t\t\tpack-$empty.idx\n>  \t\t\tEOF\n>\n> +\t\t\tls $objdir/pack >files.expect &&\n\nSimilar comments here. It should be fine to drop the assertion on\nfiles.expect, along with the content out stdout.\n\n>  \t\t\tgit multi-pack-index write --bitmap --stdin-packs \\\n> -\t\t\t\t<packs 2>err &&\n> +\t\t\t\t<packs >out 2>&1 &&\n>\n> -\t\t\ttest_grep \"bitmap without any objects\" err &&\n> -\n> -\t\t\ttest_path_is_file $midx &&\n> -\t\t\ttest_path_is_missing $midx-$(midx_checksum $objdir).bitmap\n> +\t\t\ttest_must_be_empty out &&\n> +\t\t\ttest_path_is_missing $midx &&\n> +\t\t\tls $objdir/pack >files.actual &&\n> +\t\t\ttest_cmp files.expect files.actual\n>  \t\t)\n>  \t'\n>\n> diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\n> index f0b82b5f65..fbcc19feeb 100755\n> --- a/t/t5334-incremental-multi-pack-index.sh\n> +++ b/t/t5334-incremental-multi-pack-index.sh\n> @@ -195,4 +195,79 @@ test_expect_success 'non-incremental write with existing incremental chain' '\n>  \t)\n>  '\n>\n> +test_expect_success 'skip initial MIDX layer with no objects' '\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> +\t\tgit config maintenance.auto false &&\n> +\t\tgit pack-objects $packdir/pack </dev/null &&\n> +\n> +\t\tfor bitmap in --bitmap --no-bitmap\n> +\t\tdo\n> +\t\t\tgit multi-pack-index write --incremental \"$bitmap\" >out 2>&1 &&\n> +\t\t\ttest_must_be_empty out &&\n\nSame comment about asserting the contents of stdout here as well. I am a\nlittle confused by this test, though, since there are no packs present\nin \"empty\". Shouldn't we be hitting the \"no pack files to index\" error\nhere?\n\n> +\t\t\ttest_dir_is_empty \"$midxdir\" || return 1\n> +\t\tdone &&\n> +\n> +\t\twrite_midx_layer &&\n> +\t\ttest_line_count = 1 \"$midx_chain\" &&\n> +\t\tgit multi-pack-index verify\n\nWe can drop this last block as well.\n\n> +\t)\n> +'\n> +\n> +test_expect_success 'skip MIDX layer with empty pack' '\n\nPerhaps s/skip/& intermediate/ to distinguish from the previous test?\n\n> +\tgit init empty-pack &&\n> +\t(\n> +\t\tcd empty-pack &&\n> +\t\tgit config maintenance.auto false &&\n> +\t\twrite_midx_layer &&\n> +\n> +\t\tgit pack-objects $packdir/pack </dev/null &&\n> +\t\tcp \"$midx_chain\" chain.expect &&\n> +\t\tls \"$packdir\" \"$midxdir\" >files.expect &&\n\nI think testing that the MIDX chain file is unmodified makes sense, but\nno need to test the content of $packdir and $midxdir itself. If there is\na reason to test those as well, please ensure to sort them first before\ncomparison.\n\n> +\n> +\t\tfor bitmap in --bitmap --no-bitmap\n> +\t\tdo\n> +\t\t\tgit multi-pack-index write --incremental \"$bitmap\" >out 2>&1 &&\n> +\t\t\ttest_must_be_empty out &&\n\nSame comment as above.\n\n> +\t\t\ttest_cmp chain.expect \"$midx_chain\" &&\n> +\t\t\tls \"$packdir\" \"$midxdir\" >files.actual &&\n> +\t\t\ttest_cmp files.expect files.actual || return 1\n> +\t\tdone &&\n> +\n> +\t\twrite_midx_layer &&\n> +\t\ttest_line_count = 2 \"$midx_chain\" &&\n> +\t\tgit multi-pack-index verify &&\n> +\t\tgit rev-list --test-bitmap 2.2\n\nLikewise.\n\n> +\t)\n> +'\n> +\n> +test_expect_success 'skip MIDX layer with duplicate pack' '\n\nSame comments as above, though otherwise this test looks good.\n\nThanks,\nTaylor\n"},{"id":"552755","messageId":"CAOWp8q5UqPrJQosjypdcq=KX1TKcVenAOoZUYfTBDKbRUwazTw@mail.gmail.com","threadId":"66290","inReplyTo":"CAOWp8q6uCcVL-0cLxvf-ncydSiapheYTDnFhom3CnKiMWyfJ=w@mail.gmail.com","subject":"Re: [PATCH v2] midx-write: skip writes with no object entries","fromName":"Pia Park","fromEmail":"pia@pierre.co","sentAt":"2026-09-15T14:25:06Z","receivedAt":"2026-09-15T14:25:19Z","isPatch":true,"body":"Hi! Just following up on Stolee’s suggestion to return silent success\nin the no-pack case. I’m happy to prepare v3 if there’s agreement on\nthat approach.\n\nI’ll also be at Git Merge in Lisbon this week and would be happy to\nchat in person if there’s a chance :)\n\nOn Tue, Sep 15, 2026 at 12:40 PM Pia Park <pia@pierre.co> wrote:\n>\n> Hi! I’d like to ping on Stolee’s thoughts on returning silent success on the no-pack case. Happy to prepare v3 if there’s agreement on here.\n>\n> Plus, I’ll be in Git Merge Lisbon this week and would be happy to chat in person if theres chance :)\n>\n> On Tue, Sep 8, 2026 at 4:06 PM Taylor Blau <ttaylorr@openai.com> wrote:\n>>\n>> On Tue, Sep 08, 2026 at 12:10:16AM -0700, Pia Park wrote:\n>> > Return success silently. Empty-object writes already return 0, including\n>> > when --bitmap warns, so preserve that exit status for existing callers\n>> > while omitting the warning and empty MIDX.\n>>\n>> I think that this is OK, and it matches the behavior of other builtins,\n>> e.g., running \"git repack -d\" twice in a row such that the second\n>> invocation has no objects to pack. However, I think that if we want to\n>> make this case return successfully when no objects are present, we\n>> should apply the same treatment to the case where no packs are present.\n>>\n>> But I want to make sure that others are on the same page. I would be\n>> curious to hear Stolee's (CC'd) opinion on whether returning silent\n>> success in both cases makes sense.\n>>\n>> > diff --git a/midx-write.c b/midx-write.c\n>> > index 8537102254..3038bbfad2 100644\n>> > --- a/midx-write.c\n>> > +++ b/midx-write.c\n>> > @@ -1617,9 +1617,8 @@ static int write_midx_internal(struct write_midx_opts *opts)\n>> >       }\n>> >\n>> >       if (!ctx.entries_nr) {\n>> > -             if (opts->flags & MIDX_WRITE_BITMAP)\n>> > -                     warning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n>> > -             opts->flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n>> > +             result = 0;\n>> > +             goto cleanup;\n>>\n>> Looks good, though let's make sure others agree that this is the right\n>> approach. If they do, I'd recommend changing the no packs case to also\n>> return zero either in a small preparatory patch.\n>>\n>> > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n>> > index 68143cb5b7..c239a87d10 100755\n>> > --- a/t/t5319-multi-pack-index.sh\n>> > +++ b/t/t5319-multi-pack-index.sh\n>> > @@ -54,6 +54,41 @@ test_expect_success \"don't write midx with no packs\" '\n>> >       test_path_is_missing pack/multi-pack-index\n>> >  '\n>> >\n>> > +test_expect_success 'skip non-incremental MIDX with no objects' '\n>> > +     git init --bare empty.git &&\n>> > +     (\n>> > +             cd empty.git &&\n>> > +             git pack-objects objects/pack/pack </dev/null &&\n>> > +             ls objects/pack >files.expect &&\n>>\n>> I think it's fine to drop files.expect and files.actual here. Testing\n>> that the MIDX write does nothing should be sufficient here.\n>>\n>> > +\n>> > +             for bitmap in \"\" --bitmap\n>> > +             do\n>> > +                     git multi-pack-index write $bitmap >out 2>&1 &&\n>> > +                     test_must_be_empty out &&\n>>\n>> I think it's fine to write \"git multi-pack-index write $bitmap\" without\n>> the redirection, so that this is:\n>>\n>>     for opt in \"\" --bitmap\n>>     do\n>>         git multi-pack-index write $opt &&\n>>         test_path_is_missing $objdir/pack/multi-pack-index || return 1\n>>     done &&\n>>\n>>\n>> > +             git multi-pack-index write --incremental --bitmap &&\n>> > +             test_dir_is_empty objects/pack/multi-pack-index.d &&\n>>\n>> I was going to ask whether we wanted to test this case with and without\n>> the \"--bitmap\" option as well, and likewise recommend that tthis test go\n>> in t5334 instead. But such a pair of tests already exists in t5334, so I\n>> think we can safely drop this hunk.\n>>\n>> > +             echo blob | git hash-object -w --stdin >in &&\n>> > +             git pack-objects objects/pack/pack <in &&\n>> > +             git multi-pack-index write --incremental --bitmap &&\n>> > +             test_line_count = 1 objects/pack/multi-pack-index.d/multi-pack-index-chain &&\n>> > +             git multi-pack-index verify &&\n>> > +\n>> > +             echo another | git hash-object -w --stdin >in &&\n>> > +             git pack-objects objects/pack/pack <in &&\n>> > +             git multi-pack-index write --bitmap &&\n>> > +             test_path_is_file objects/pack/multi-pack-index &&\n>> > +             midx=\"$(midx_checksum objects)\" &&\n>> > +             test_path_is_file objects/pack/multi-pack-index-$midx.bitmap &&\n>> > +             git multi-pack-index verify\n>>\n>> These two blocks are testing normal MIDX operations that are well\n>> covered elsewhere in the test suite. I think we can drop these safely.\n>>\n>> > +     )\n>> > +'\n>> > +\n>> >  test_expect_success SHA1 'warn if a midx contains no oid' '\n>> >       cp \"$TEST_DIRECTORY\"/t5319/no-objects.midx $objdir/pack/multi-pack-index &&\n>> >       test_must_fail git multi-pack-index verify &&\n>> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n>> > index 86beab1dae..490008d1d7 100755\n>> > --- a/t/t5326-multi-pack-bitmaps.sh\n>> > +++ b/t/t5326-multi-pack-bitmaps.sh\n>> > @@ -305,7 +305,7 @@ test_midx_bitmap_cases () {\n>> >               )\n>> >       '\n>> >\n>> > -     test_expect_success 'no .bitmap is written without any objects' '\n>> > +     test_expect_success 'no MIDX or .bitmap is written without any objects' '\n>> >               rm -fr repo &&\n>> >               git init repo &&\n>> >               test_when_finished \"rm -fr repo\" &&\n>> > @@ -318,13 +318,14 @@ test_midx_bitmap_cases () {\n>> >                       pack-$empty.idx\n>> >                       EOF\n>> >\n>> > +                     ls $objdir/pack >files.expect &&\n>>\n>> Similar comments here. It should be fine to drop the assertion on\n>> files.expect, along with the content out stdout.\n>>\n>> >                       git multi-pack-index write --bitmap --stdin-packs \\\n>> > -                             <packs 2>err &&\n>> > +                             <packs >out 2>&1 &&\n>> >\n>> > -                     test_grep \"bitmap without any objects\" err &&\n>> > -\n>> > -                     test_path_is_file $midx &&\n>> > -                     test_path_is_missing $midx-$(midx_checksum $objdir).bitmap\n>> > +                     test_must_be_empty out &&\n>> > +                     test_path_is_missing $midx &&\n>> > +                     ls $objdir/pack >files.actual &&\n>> > +                     test_cmp files.expect files.actual\n>> >               )\n>> >       '\n>> >\n>> > diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh\n>> > index f0b82b5f65..fbcc19feeb 100755\n>> > --- a/t/t5334-incremental-multi-pack-index.sh\n>> > +++ b/t/t5334-incremental-multi-pack-index.sh\n>> > @@ -195,4 +195,79 @@ test_expect_success 'non-incremental write with existing incremental chain' '\n>> >       )\n>> >  '\n>> >\n>> > +test_expect_success 'skip initial MIDX layer with no objects' '\n>> > +     git init empty &&\n>> > +     (\n>> > +             cd empty &&\n>> > +             git config maintenance.auto false &&\n>> > +             git pack-objects $packdir/pack </dev/null &&\n>> > +\n>> > +             for bitmap in --bitmap --no-bitmap\n>> > +             do\n>> > +                     git multi-pack-index write --incremental \"$bitmap\" >out 2>&1 &&\n>> > +                     test_must_be_empty out &&\n>>\n>> Same comment about asserting the contents of stdout here as well. I am a\n>> little confused by this test, though, since there are no packs present\n>> in \"empty\". Shouldn't we be hitting the \"no pack files to index\" error\n>> here?\n>>\n>> > +                     test_dir_is_empty \"$midxdir\" || return 1\n>> > +             done &&\n>> > +\n>> > +             write_midx_layer &&\n>> > +             test_line_count = 1 \"$midx_chain\" &&\n>> > +             git multi-pack-index verify\n>>\n>> We can drop this last block as well.\n>>\n>> > +     )\n>> > +'\n>> > +\n>> > +test_expect_success 'skip MIDX layer with empty pack' '\n>>\n>> Perhaps s/skip/& intermediate/ to distinguish from the previous test?\n>>\n>> > +     git init empty-pack &&\n>> > +     (\n>> > +             cd empty-pack &&\n>> > +             git config maintenance.auto false &&\n>> > +             write_midx_layer &&\n>> > +\n>> > +             git pack-objects $packdir/pack </dev/null &&\n>> > +             cp \"$midx_chain\" chain.expect &&\n>> > +             ls \"$packdir\" \"$midxdir\" >files.expect &&\n>>\n>> I think testing that the MIDX chain file is unmodified makes sense, but\n>> no need to test the content of $packdir and $midxdir itself. If there is\n>> a reason to test those as well, please ensure to sort them first before\n>> comparison.\n>>\n>> > +\n>> > +             for bitmap in --bitmap --no-bitmap\n>> > +             do\n>> > +                     git multi-pack-index write --incremental \"$bitmap\" >out 2>&1 &&\n>> > +                     test_must_be_empty out &&\n>>\n>> Same comment as above.\n>>\n>> > +                     test_cmp chain.expect \"$midx_chain\" &&\n>> > +                     ls \"$packdir\" \"$midxdir\" >files.actual &&\n>> > +                     test_cmp files.expect files.actual || return 1\n>> > +             done &&\n>> > +\n>> > +             write_midx_layer &&\n>> > +             test_line_count = 2 \"$midx_chain\" &&\n>> > +             git multi-pack-index verify &&\n>> > +             git rev-list --test-bitmap 2.2\n>>\n>> Likewise.\n>>\n>> > +     )\n>> > +'\n>> > +\n>> > +test_expect_success 'skip MIDX layer with duplicate pack' '\n>>\n>> Same comments as above, though otherwise this test looks good.\n>>\n>> Thanks,\n>> Taylor\n"}]}