* [PATCH 1/2] replay: fail gracefully when a merge input is unreadable
2026-08-18 22:34 [PATCH 0/2] Objects treated as missing despite being present, due to race with geometric repacking Elijah Newren via GitGitGadget
@ 2026-08-18 22:34 ` Elijah Newren via GitGitGadget
2026-08-18 22:34 ` [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack Elijah Newren via GitGitGadget
1 sibling, 0 replies; 3+ messages in thread
From: Elijah Newren via GitGitGadget @ 2026-08-18 22:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
When objects involved in the merge cannot be read, the merge machinery
will return early with result.clean = -1, and result.tree left as NULL.
pick_regular_commit() tested only "if (!result->clean)", ignoring the
case where "clean < 0". That causes the code to try to use
result->tree, resulting in a SIGSEGV.
Handle clean < 0 explicitly; the merge machinery will already have printed
messages such as "Could not read <object>" and "collecting merge info
failed for trees...", so we don't need to add much detail beyond the
fact that the merge failed.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
replay.c | 7 +++++++
t/t3650-replay-basics.sh | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/replay.c b/replay.c
index 463c900d6c..33e21b2032 100644
--- a/replay.c
+++ b/replay.c
@@ -327,6 +327,13 @@ static struct commit *pick_regular_commit(struct repository *repo,
merge_opt->ancestor = NULL;
merge_opt->branch2 = NULL;
+ if (result->clean < 0) {
+ error(_("merge of %s onto %s failed"),
+ oid_to_hex(&pickme->object.oid),
+ oid_to_hex(&replayed_base->object.oid));
+ return NULL;
+ }
+
if (!result->clean)
return NULL;
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 3353bc4a4d..d66b8edb95 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -565,4 +565,39 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
test_grep "cannot be used with multiple revision ranges" err
'
+test_expect_success 'replay fails without segfault when objects are missing' '
+ test_when_finished "rm -fr unreadable" &&
+ git init unreadable &&
+ (
+ cd unreadable &&
+
+ test_write_lines l1 l2 l3 l4 l5 l6 l7 l8 >f &&
+ git add f &&
+ git commit -m base &&
+ git branch base &&
+
+ test_write_lines l1 l2 l3 l4 l5 l6 l7 CHANGED >f &&
+ git commit -am side &&
+ git branch side &&
+
+ git switch -c onto base &&
+ test_write_lines CHANGED l2 l3 l4 l5 l6 l7 l8 >f &&
+ git commit -am onto &&
+
+ # The replay works while every object is readable.
+ git replay --onto onto base..side &&
+
+ # Removing the onto tree makes parse_tree() fail during the
+ # incore merge, driving clean < 0 with a NULL result tree.
+ onto_tree=$(git rev-parse onto^{tree}) &&
+ obj=$(test_oid_to_path "$onto_tree") &&
+ mv .git/objects/${obj} saved-tree &&
+
+ # Ensure replay gracefully handles the missing object
+ test_must_fail git replay --onto onto base..side 2>err &&
+ test_grep ! "[Ss]egmentation" err &&
+ test_grep "Could not read\|collecting merge info failed" err
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] packfile: recover when a multi-pack-index names a removed pack
2026-08-18 22:34 [PATCH 0/2] Objects treated as missing despite being present, due to race with geometric repacking Elijah Newren via GitGitGadget
2026-08-18 22:34 ` [PATCH 1/2] replay: fail gracefully when a merge input is unreadable Elijah Newren via GitGitGadget
@ 2026-08-18 22:34 ` Elijah Newren via GitGitGadget
1 sibling, 0 replies; 3+ messages in thread
From: Elijah Newren via GitGitGadget @ 2026-08-18 22:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
When a geometric repack runs concurrently with other git processes, it
can write a new pack and multi-pack-index and then delete older packs
that the new one subsumes. One or more of those older packs may have
been indexed by the previous multi-pack-index. A process that already
had the previous multi-pack-index open keeps using it, and that stale
index still records the removed pack(s) as owning some objects.
Because a multi-pack-index attributes each object to exactly one pack,
an object that exists in multiple covered packs is served only through
its recorded owner. If that owner is the pack a concurrent repack just
removed, find_pack_entry() cannot serve the object: fill_midx_entry()
routes the lookup to the missing pack (prepare_midx_pack() fails), and
the regular pack fallback deliberately skips every multi-pack-index
covered pack. The object is reported missing even though a perfectly
good copy survives in another covered pack -- for example a large "base"
pack that geometric repacking intentionally kept.
The false negative is not limited to one caller. Any reader
(cat-file, rev-list, pack-objects, ...) can spuriously fail with
"unable to read object", and callers that only ask whether an object
exists get a wrong answer too, since the OBJECT_INFO_QUICK path never
retries. Writers that merge in-core, such as "git replay", are hit
hardest: merge-ort treats the unreadable tree as a premature abort, sets
result.clean < 0, and returns without a result tree.
Teach find_pack_entry() to recover. After the normal multi-pack-index
lookup and the regular pack fallback both miss, check whether the object
is nonetheless present in a covered multi-pack-index (bsearch_midx()).
If it is, its recorded owner must have become unavailable, so scan that
index's packs directly for a surviving copy. The bsearch gate keeps
genuine misses (i.e. objects absent from the index) on the fast path, and
because the recovery lives in find_pack_entry() itself it also fixes the
OBJECT_INFO_QUICK callers that never reprepare.
This recovers the object without touching the multi-pack-index itself.
Reloading the stale index would be a more complete fix but would be much
more involved: other code (pack bitmaps, object name disambiguation)
borrows and caches the "struct multi_pack_index *" across object reads,
so freeing it underneath them would be a use-after-free. Refreshing the
index with proper invalidation of those borrowers is left for future
work.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
odb/source-packed.c | 29 +++++++++++++++++++++++++++
t/t5319-multi-pack-index.sh | 40 +++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+)
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0890704e76..de96215069 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -31,6 +31,35 @@ static int find_pack_entry(struct odb_source_packed *store,
}
}
+ /*
+ * Recovery for a concurrent-repack race: a MIDX can name an owning
+ * pack for an object that a simultaneous repack has since deleted,
+ * even though the object still exists in another pack the same MIDX
+ * covers (e.g. a kept base pack that geometric repack did not rewrite).
+ * If the object is present in a MIDX yet none of the paths above could
+ * serve it, its recorded owning pack has become unavailable. The
+ * regular fallback above deliberately skips MIDX-covered packs, so
+ * scan this MIDX's packs directly to find the surviving copy. The
+ * bsearch gate keeps genuine misses (objects absent from the MIDX) on
+ * the fast path.
+ */
+ if (store->midx) {
+ struct multi_pack_index *m = store->midx;
+ uint32_t midx_pos, i;
+
+ if (bsearch_midx(oid, m, &midx_pos)) {
+ for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
+ struct packed_git *p;
+
+ if (prepare_midx_pack(m, i))
+ continue;
+ p = nth_midxed_pack(m, i);
+ if (p && packfile_fill_entry(p, oid, e))
+ return 1;
+ }
+ }
+ }
+
return 0;
}
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 68143cb5b7..2b8ff6f3ed 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1393,4 +1393,44 @@ test_expect_success 'pack.preferBitmapTips interprets patterns as hierarchy' '
)
'
+test_expect_success 'lookup recovers object whose midx-owning pack was removed' '
+ test_when_finished "rm -fr repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ # "keep" ends up only in the big pack; "dup" is deliberately
+ # placed in two packs so the midx has to choose an owner.
+ test_commit keep &&
+ echo duplicated-content >dup &&
+ git add dup &&
+ git commit -m dup &&
+ dup_oid=$(git rev-parse HEAD:dup) &&
+
+ # Roll every object, including dup, into a single big pack.
+ git repack -adq &&
+
+ # Build a second, "moderate" pack that also contains dup, so dup
+ # now lives in two packs that the midx will cover.
+ moderate=$(echo "$dup_oid" |
+ git pack-objects --quiet $objdir/pack/pack) &&
+
+ # Attribute dup to the moderate pack in the midx.
+ git multi-pack-index write \
+ --preferred-pack="pack-$moderate.idx" &&
+
+ # Simulate a concurrent "git repack" retiring the moderate pack:
+ # its files disappear, but the now-stale midx still names it as
+ # the owner of dup. A valid copy of dup survives in the big pack.
+ rm -f $objdir/pack/pack-$moderate.* &&
+
+ # The midx routes the lookup to the deleted pack, and the regular
+ # pack fallback skips midx-covered packs, so without recovery dup
+ # would appear missing even though it is physically present.
+ echo blob >expect &&
+ git cat-file -t "$dup_oid" >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 3+ messages in thread