packfile: recover delta cycles through duplicate entries
98f8854c94 (index-pack: allow revisiting REF_DELTA chains, 2025-04-28) changed t5309's recoverable-cycle case to expect index-pack to accept the pack, but did not read the resulting objects. The .idx for a pack with duplicate OIDs retains every physical entry, with equal OIDs in a contiguous run. Ordinary REF_DELTA lookup selects one representation from such a run. If that choice closes a cycle, both type and content readers follow the same physical offsets indefinitely even though another representation is usable. Keep the ordinary walk, and recognize a cycle only when its existing stack shows an exact repeated pack offset. At that point, restart at the requested object and search duplicate representations depth-first. Treat each OID as a node, try each entry in its .idx run, and translate OFS_DELTA bases back to OIDs. A bitmap marks each OID group already visited. Reaching a full object records the exact offsets along the acyclic path so unpack_entry() can replay it; packed_to_object_type() needs only the resulting type. Thus, acyclic lookups continue to use the existing single-entry lookup. Duplicate-run scans, the visited bitmap, and OFS-to-OID translation remain confined to recovery after a proven cycle. Extend t5309 to read both type and content after indexing. Cover a root-level full duplicate, a mixed REF/OFS cycle, and a tail into a three-object cycle which must backtrack before taking an alternate REF_DELTA/OFS_DELTA path to a full base. Keep the fixtures hash-independent so the same cases run under SHA-1 and SHA-256. This adds the reader validation missing from the earlier acceptance test. Signed-off-by: Taylor Blau <ttaylorr@openai.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau committed
Jul 24, 2026 at 16:06 UTC
f4037afe80c7957ad6a071bfb0cd206eccfc7126
2 files changed
+341
-6
packfile.c
+199
@@ -10,6 +10,7 @@
10
#include "dir.h"
11
#include "packfile.h"
12
#include "delta.h"
13
+#include "ewah/ewok.h"
14
#include "hash-lookup.h"
15
#include "commit.h"
16
#include "object.h"
@@ -1077,6 +1078,160 @@ static int get_delta_base_oid(struct packed_git *p,
1078
return -1;
1079
}
1080
1081
+/*
1082
+ * Search duplicate representations for a chain ending in a full object.
1083
+ * Representations of the same OID are interchangeable as delta bases.
1084
+ *
1085
+ * Each path entry is the search frame for one OID. It walks that OID's
1086
+ * contiguous .idx entries and retains the chosen entry's .pack offset
1087
+ * for replay.
1088
+ */
1089
+struct delta_path_entry {
1090
+ off_t selected_offset; /* zero until a candidate is selected */
1091
+ uint32_t next; /* index position of the next candidate */
1092
+ uint32_t remaining; /* total number of candidates remaining */
1093
+};
1094
+
1095
+struct delta_path {
1096
+ struct delta_path_entry *entries;
1097
+ size_t nr, alloc;
1098
+};
1099
+
1100
+static int push_oid_group(struct packed_git *p,
1101
+ const struct object_id *oid,
1102
+ struct bitmap *visited, struct delta_path *path)
1103
+{
1104
+ struct object_id candidate;
1105
+ struct delta_path_entry *entry;
1106
+ uint32_t first_index_pos, group_index_pos, index_pos;
1107
+
1108
+ /*
1109
+ * Determine the range of index positions referring to duplicate
1110
+ * copies of the given object.
1111
+ *
1112
+ * Any position within that range is OK, since we will determine
1113
+ * the exact range below.
1114
+ */
1115
+ if (!bsearch_pack(oid, p, &group_index_pos))
1116
+ return 0;
1117
+ if (bitmap_get(visited, group_index_pos))
1118
+ return 0;
1119
+
1120
+ first_index_pos = group_index_pos;
1121
+ while (first_index_pos > 0) {
1122
+ if (nth_packed_object_id(&candidate, p, first_index_pos - 1) < 0)
1123
+ return -1;
1124
+ if (!oideq(&candidate, oid))
1125
+ break;
1126
+ first_index_pos--;
1127
+ }
1128
+
1129
+ for (index_pos = first_index_pos; index_pos < p->num_objects; index_pos++) {
1130
+ if (nth_packed_object_id(&candidate, p, index_pos) < 0)
1131
+ return -1;
1132
+ if (!oideq(&candidate, oid))
1133
+ break;
1134
+ }
1135
+
1136
+ bitmap_set(visited, group_index_pos);
1137
+
1138
+ ALLOC_GROW(path->entries, path->nr + 1, path->alloc);
1139
+ entry = &path->entries[path->nr++];
1140
+ entry->selected_offset = 0;
1141
+ entry->next = first_index_pos;
1142
+ entry->remaining = index_pos - first_index_pos;
1143
+
1144
+ return 1;
1145
+}
1146
+
1147
+static enum object_type find_delta_path(struct packed_git *p,
1148
+ struct pack_window **w_curs,
1149
+ off_t offset,
1150
+ struct delta_path *path)
1151
+{
1152
+ struct object_id oid;
1153
+ uint32_t pack_pos;
1154
+ struct bitmap *visited;
1155
+ enum object_type result = OBJ_BAD;
1156
+
1157
+ if (offset_to_pack_pos(p, offset, &pack_pos) < 0)
1158
+ return OBJ_BAD;
1159
+ if (nth_packed_object_id(&oid, p, pack_pos_to_index(p, pack_pos)) < 0)
1160
+ return OBJ_BAD;
1161
+
1162
+ visited = bitmap_new();
1163
+ if (push_oid_group(p, &oid, visited, path) != 1)
1164
+ goto done;
1165
+
1166
+ /*
1167
+ * Search depth-first for a chain ending in a full object. Each frame
1168
+ * tries every representation of one OID; a delta pushes its base OID,
1169
+ * while exhausting a frame backtracks to its parent.
1170
+ */
1171
+ while (path->nr) {
1172
+ struct delta_path_entry *entry = &path->entries[path->nr - 1];
1173
+ enum object_type candidate_type;
1174
+ off_t curpos;
1175
+ size_t size;
1176
+
1177
+ /*
1178
+ * This OID has no path to a full object. Let its parent try
1179
+ * another representation; exhausting the root fails the search.
1180
+ */
1181
+ if (!entry->remaining) {
1182
+ path->nr--;
1183
+ continue;
1184
+ }
1185
+
1186
+ entry->selected_offset =
1187
+ nth_packed_object_offset(p, entry->next++);
1188
+ entry->remaining--;
1189
+ curpos = entry->selected_offset;
1190
+ candidate_type = unpack_object_header(p, w_curs, &curpos, &size);
1191
+
1192
+ /*
1193
+ * A full object terminates the chain, and its type is
1194
+ * inherited by every delta above it. A delta continues
1195
+ * at its base; any other type rejects only this
1196
+ * representation.
1197
+ */
1198
+ switch (candidate_type) {
1199
+ case OBJ_COMMIT:
1200
+ case OBJ_TREE:
1201
+ case OBJ_BLOB:
1202
+ case OBJ_TAG:
1203
+ result = candidate_type;
1204
+ goto done;
1205
+ case OBJ_OFS_DELTA:
1206
+ case OBJ_REF_DELTA:
1207
+ break;
1208
+ default:
1209
+ /*
1210
+ * A bad or unknown type rejects only this copy;
1211
+ * another representation of the same OID may
1212
+ * still work.
1213
+ */
1214
+ continue;
1215
+ }
1216
+
1217
+ /*
1218
+ * Descend to this delta's base. A malformed reference
1219
+ * or a missing or already-visited base rejects this
1220
+ * copy. A newly pushed base is examined next; an index
1221
+ * error aborts the search.
1222
+ */
1223
+ if (get_delta_base_oid(p, w_curs, curpos, &oid, candidate_type,
1224
+ entry->selected_offset))
1225
+ continue;
1226
+ if (push_oid_group(p, &oid, visited, path) < 0)
1227
+ goto done;
1228
+ }
1229
+
1230
+done:
1231
+ bitmap_free(visited);
1232
+ return result;
1233
+}
1234
+
1235
static int retry_bad_packed_offset(struct repository *r,
1236
struct packed_git *p,
1237
off_t obj_offset)
@@ -1105,11 +1260,27 @@ static enum object_type packed_to_object_type(struct repository *r,
1260
{
1261
off_t small_poi_stack[POI_STACK_PREALLOC];
1262
off_t *poi_stack = small_poi_stack;
1263
+ off_t root_offset = obj_offset;
1264
int poi_stack_nr = 0, poi_stack_alloc = POI_STACK_PREALLOC;
1265
1266
while (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
1267
off_t base_offset;
1268
size_t size;
1269
+
1270
+ if (poi_stack_nr > 0 && poi_stack_nr % 2 == 0 &&
1271
+ obj_offset == poi_stack[poi_stack_nr / 2]) {
1272
+ struct delta_path path = { 0 };
1273
+ /*
1274
+ * Normal lookup returned to the same pack
1275
+ * entry. Restart from the requested object
1276
+ * using alternate representations.
1277
+ */
1278
+ type = find_delta_path(p, w_curs, root_offset, &path);
1279
+ free(path.entries);
1280
+ if (type == OBJ_BAD)
1281
+ goto unwind;
1282
+ break;
1283
+ }
1284
/* Push the object we're going to leave behind */
1285
if (poi_stack_nr >= poi_stack_alloc && poi_stack == small_poi_stack) {
1286
poi_stack_alloc = alloc_nr(poi_stack_nr);
@@ -1525,9 +1696,11 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
1696
{
1697
struct pack_window *w_curs = NULL;
1698
off_t curpos = obj_offset;
1699
+ off_t root_offset = obj_offset;
1700
void *data = NULL;
1701
size_t size;
1702
enum object_type type;
1703
+ struct delta_path path = { 0 };
1704
struct unpack_entry_stack_ent small_delta_stack[UNPACK_ENTRY_STACK_PREALLOC];
1705
struct unpack_entry_stack_ent *delta_stack = small_delta_stack;
1706
int delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC;
@@ -1553,6 +1726,22 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
1726
break;
1727
}
1728
1729
+ if (!path.nr &&
1730
+ delta_stack_nr > 0 && delta_stack_nr % 2 == 0 &&
1731
+ obj_offset == delta_stack[delta_stack_nr / 2].obj_offset) {
1732
+ /*
1733
+ * Normal lookup returned to the same pack
1734
+ * entry. Find an acyclic path if one exists,
1735
+ * discard this walk, and replay that path.
1736
+ */
1737
+ if (find_delta_path(p, &w_curs, root_offset,
1738
+ &path) == OBJ_BAD)
1739
+ break;
1740
+ delta_stack_nr = 0;
1741
+ curpos = obj_offset = path.entries[0].selected_offset;
1742
+ continue;
1743
+ }
1744
+
1745
if (do_check_packed_object_crc && p->index_version > 1) {
1746
uint32_t pack_pos, index_pos;
1747
off_t len;
@@ -1591,6 +1780,15 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
1780
break;
1781
}
1782
1783
+ /* Use the base chosen by recovery, not the one from normal lookup. */
1784
+ if (path.nr) {
1785
+ size_t path_pos = (size_t)delta_stack_nr + 1;
1786
+
1787
+ if (path_pos >= path.nr)
1788
+ BUG("alternate delta path ends in a delta");
1789
+ base_offset = path.entries[path_pos].selected_offset;
1790
+ }
1791
+
1792
/* push object, proceed to base */
1793
if (delta_stack_nr >= delta_stack_alloc
1794
&& delta_stack == small_delta_stack) {
@@ -1731,6 +1929,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
1929
1930
out:
1931
unuse_pack(&w_curs);
1932
+ free(path.entries);
1933
1934
if (delta_stack != small_delta_stack)
1935
free(delta_stack);
t/t5309-pack-delta-cycles.sh
+142
-6
@@ -9,6 +9,83 @@ test_description='test index-pack handling of delta cycles in packfiles'
9
A=$(test_oid packlib_7_0)
10
B=$(test_oid packlib_7_76)
11
12
+# Copy the entries from a complete pack without its header or trailer.
13
+pack_entries () {
14
+ entry_size=$(wc -c <"$1") &&
15
+ dd if="$1" bs=1 skip=12 \
16
+ count=$((entry_size - 12 - $(test_oid rawsz))) 2>/dev/null
17
+}
18
+
19
+# B as an OFS_DELTA against A at the given one-byte distance.
20
+pack_obj_b_ofs_a () {
21
+ pack_obj "$B" "$A" >b-ref.tmp &&
22
+ printf "\145" &&
23
+ printf "\\$(printf "%03o" "$1")" &&
24
+ dd if=b-ref.tmp bs=1 skip=$((1 + $(test_oid rawsz))) 2>/dev/null
25
+}
26
+
27
+# Return the base of the first one-byte-header REF_DELTA for the given OID.
28
+first_ref_base () {
29
+ idx=$(echo .git/objects/pack/*.idx) &&
30
+ offset=$(git show-index <"$idx" |
31
+ awk -v oid="$1" '$2 == oid { print $1; exit }') &&
32
+ dd if="${idx%.idx}.pack" bs=1 skip=$((offset + 1)) \
33
+ count=$(test_oid rawsz) 2>/dev/null |
34
+ test-tool hexdump |
35
+ tr -d " \n"
36
+}
37
+
38
+# The order of equal-OID entries in the .idx is unspecified. Retain a pack
39
+# which selects $1 as a delta against $2. Unless $3 is "-", also require the
40
+# first copy searched during recovery to be a REF_DELTA against $3.
41
+install_cycle () {
42
+ cycle_oid=$1 &&
43
+ cycle_base=$2 &&
44
+ first_base=$3 &&
45
+ shift 3 &&
46
+ for pack
47
+ do
48
+ clear_packs &&
49
+ git index-pack --fix-thin --stdin <"$pack" &&
50
+ selected_base=$(echo "$cycle_oid" |
51
+ git cat-file --batch-check="%(deltabase)") ||
52
+ return 1
53
+ if test "$selected_base" = "$cycle_base" &&
54
+ { test "$first_base" = "-" ||
55
+ test "$(first_ref_base "$cycle_oid")" = "$first_base"; }
56
+ then
57
+ return 0
58
+ fi
59
+ done
60
+ return 1
61
+}
62
+
63
+make_cycle_pack () {
64
+ cycle_pack=$1 &&
65
+ shift &&
66
+ test-tool -C alt-source pack-deltas --num-objects=6 >refs.tmp <<-EOF &&
67
+ REF_DELTA $T $X
68
+ REF_DELTA $X $1
69
+ REF_DELTA $Y $Z
70
+ REF_DELTA $Z $X
71
+ REF_DELTA $X $2
72
+ REF_DELTA $X $3
73
+ EOF
74
+ {
75
+ pack_header 8 &&
76
+ pack_entries refs.tmp &&
77
+ cat a-full &&
78
+ pack_obj_b_ofs_a "$a_full_size"
79
+ } >"$cycle_pack" &&
80
+ pack_trailer "$cycle_pack"
81
+}
82
+
83
+check_blob () {
84
+ test "$(git cat-file -t "$1")" = blob &&
85
+ git cat-file blob "$1" >actual &&
86
+ test_cmp_bin "$2" actual
87
+}
88
+
89
# double-check our hand-constucted packs
90
test_expect_success 'index-pack works with a single delta (A->B)' '
91
clear_packs &&
@@ -67,18 +144,77 @@ test_expect_success 'failover to an object in another pack' '
144
'
145
146
test_expect_success 'failover to a duplicate object in the same pack' '
70
- clear_packs &&
147
+ {
148
+ pack_header 3 &&
149
+ pack_obj $A &&
150
+ pack_obj $B $A &&
151
+ pack_obj $A $B
152
+ } >recoverable-1.pack &&
153
+ pack_trailer recoverable-1.pack &&
154
{
155
pack_header 3 &&
156
pack_obj $A $B &&
157
pack_obj $B $A &&
158
pack_obj $A
76
- } >recoverable.pack &&
77
- pack_trailer recoverable.pack &&
159
+ } >recoverable-2.pack &&
160
+ pack_trailer recoverable-2.pack &&
161
+
162
+ # The selected copy of A is part of the cycle, but the full copy
163
+ # lets both type and content lookups resolve it.
164
+ install_cycle "$A" "$B" - recoverable-1.pack recoverable-2.pack &&
165
+ printf "\7\0" >expect &&
166
+ check_blob "$A" expect
167
+'
168
+
169
+test_expect_success 'failover from a mixed REF/OFS cycle' '
170
+ pack_obj "$A" "$B" >a-ref &&
171
+ pack_obj "$B" >b-full &&
172
+ a_ref_size=$(wc -c <a-ref) &&
173
+ b_full_size=$(wc -c <b-full) &&
174
+
175
+ {
176
+ pack_header 3 &&
177
+ cat a-ref &&
178
+ cat b-full &&
179
+ pack_obj_b_ofs_a "$((a_ref_size + b_full_size))"
180
+ } >mixed-1.pack &&
181
+ pack_trailer mixed-1.pack &&
182
+ {
183
+ pack_header 3 &&
184
+ cat a-ref &&
185
+ pack_obj_b_ofs_a "$a_ref_size" &&
186
+ cat b-full
187
+ } >mixed-2.pack &&
188
+ pack_trailer mixed-2.pack &&
189
+
190
+ # The REF_DELTA for A selects the OFS_DELTA copy of B; the
191
+ # full B is its escape.
192
+ install_cycle "$B" "$A" - mixed-1.pack mixed-2.pack &&
193
+ printf "\7\0" >expect &&
194
+ check_blob "$A" expect
195
+'
196
79
- # This cycle does not fail since the existence of a full copy
80
- # of A in the pack allows us to resolve the cycle.
81
- git index-pack --fix-thin --stdin <recoverable.pack
197
+test_expect_success 'failover after a tail into a three-object delta cycle' '
198
+ git init alt-source &&
199
+ printf "\7\76" |
200
+ git -C alt-source hash-object -w --stdin >/dev/null &&
201
+ X=$(printf x | git -C alt-source hash-object -w --stdin) &&
202
+ Y=$(printf y | git -C alt-source hash-object -w --stdin) &&
203
+ Z=$(printf z | git -C alt-source hash-object -w --stdin) &&
204
+ printf "tail T\n" >tail &&
205
+ T=$(git -C alt-source hash-object -w --stdin <tail) &&
206
+
207
+ pack_obj "$A" >a-full &&
208
+ a_full_size=$(wc -c <a-full) &&
209
+ make_cycle_pack alternate-1.pack "$B" "$Y" "$Y" &&
210
+ make_cycle_pack alternate-2.pack "$Y" "$B" "$Y" &&
211
+ make_cycle_pack alternate-3.pack "$Y" "$Y" "$B" &&
212
+
213
+ # Lookup of T follows T->X->Y->Z->X. Recovery must exhaust that
214
+ # branch, then use X->B->A, whose final edge is an OFS_DELTA.
215
+ install_cycle "$X" "$Y" "$Y" \
216
+ alternate-1.pack alternate-2.pack alternate-3.pack &&
217
+ check_blob "$T" tail
218
'
219
220
test_expect_success 'index-pack works with thin pack A->B->C with B on disk' '