Merge branch 'tb/pack-with-duplicates' into seen

The handling of packfiles with duplicate object entries has been
hardened.  Specifically, reverse index lookup, delta cycle
recovery, multi-pack-index verification, and pack reuse paths have
been updated to correctly handle or gracefully reject duplicate
entries.

* tb/pack-with-duplicates:
  pack-bitmap: handle duplicate pack entries during MIDX reuse
  test-tool bitmap: reject packs with duplicate objects
  midx: verify duplicate pack entries by OID and offset
  packfile: recover delta cycles through duplicate entries
  t5308: test reverse indexes with duplicate objects

builtin/pack-objects.c

@@ -1184,12 +1184,12 @@ static size_t write_reused_pack_verbatim(struct bitmapped_pack *reuse_packfile,
 	size_t pos = 0;
 	size_t end;
 
-	if (reuse_packfile->bitmap_pos) {
+	if (reuse_packfile->bitmap_pos ||
+	    reuse_packfile->bitmap_nr != reuse_packfile->p->num_objects) {
 		/*
-		 * We can't reuse whole chunks verbatim out of
+		 * We can't reuse whole chunks verbatim from non-preferred
-		 * non-preferred packs since we can't guarantee that
+		 * packs or packs with entries missing from the bitmap because
-		 * all duplicate objects were resolved in favor of
+		 * bitmap and pack positions may differ.
-		 * that pack.
 		 *
 		 * Even if we have a whole eword_t worth of bits that
 		 * could be reused, there may be objects between the
@@ -1198,7 +1198,7 @@ static size_t write_reused_pack_verbatim(struct bitmapped_pack *reuse_packfile,
 		 * pack, causing us to send duplicate or unwanted
 		 * objects.
 		 *
-		 * Handle non-preferred packs from within
+		 * Handle these packs from within
 		 * write_reused_pack(), which inspects and reuses
 		 * individual bits.
 		 */
@@ -1265,20 +1265,18 @@ static void write_reused_pack(struct bitmapped_pack *reuse_packfile,
 			if (pos + offset >= reuse_packfile->bitmap_pos + reuse_packfile->bitmap_nr)
 				goto done;
 
-			if (reuse_packfile->bitmap_pos) {
+			if (reuse_packfile->bitmap_pos ||
+			    reuse_packfile->bitmap_nr != reuse_packfile->p->num_objects) {
 				/*
-				 * When doing multi-pack reuse on a
+				 * Translate MIDX bitmap positions which do not
-				 * non-preferred pack, translate bit positions
+				 * correspond directly to physical pack positions.
-				 * from the MIDX pseudo-pack order back to their
-				 * pack-relative positions before attempting
-				 * reuse.
 				 */
 				struct multi_pack_index *m = reuse_packfile->from_midx;
 				uint32_t midx_pos;
 				off_t pack_ofs;
 
 				if (!m)
-					BUG("non-zero bitmap position without MIDX");
+					BUG("cannot translate bitmap position without MIDX");
 
 				midx_pos = pack_pos_to_midx(m, pos + offset);
 				pack_ofs = nth_midxed_offset(m, midx_pos);

midx.c

@@ -910,6 +910,48 @@ static int compare_pair_pos_vs_id(const void *_a, const void *_b)
 	return b->pack_int_id - a->pack_int_id;
 }
 
+/*
+ * Return whether the pack index contains an entry with both "oid" and
+ * "offset". A pack index may contain duplicate OIDs, so an arbitrary
+ * OID lookup is not enough to validate a particular offset.
+ *
+ * Do not use offset_to_pack_pos() here: it may consult an optional '.rev'
+ * file, which is verified separately, or build an in-memory reverse index
+ * that remains attached to the pack. Verify the MIDX directly against its
+ * source pack index instead.
+ */
+static int pack_index_has_oid_at_offset(struct packed_git *p,
+					const struct object_id *oid,
+					off_t offset)
+{
+	struct object_id candidate;
+	uint32_t pos, i;
+
+	if (!bsearch_pack(oid, p, &pos))
+		return 0;
+
+	if (nth_packed_object_offset(p, pos) == offset)
+		return 1;
+
+	for (i = pos; i > 0; i--) {
+		if (nth_packed_object_id(&candidate, p, i - 1) ||
+		    !oideq(&candidate, oid))
+			break;
+		if (nth_packed_object_offset(p, i - 1) == offset)
+			return 1;
+	}
+
+	for (i = pos + 1; i < p->num_objects; i++) {
+		if (nth_packed_object_id(&candidate, p, i) ||
+		    !oideq(&candidate, oid))
+			break;
+		if (nth_packed_object_offset(p, i) == offset)
+			return 1;
+	}
+
+	return 0;
+}
+
 /*
  * Limit calls to display_progress() for performance reasons.
  * The interval here was arbitrarily chosen.
@@ -1019,7 +1061,6 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags)
 	for (i = 0; i < m->num_objects + m->num_objects_in_base; i++) {
 		struct object_id oid;
 		struct pack_entry e;
-		off_t m_offset, p_offset;
 
 		if (i > 0 && pairs[i-1].pack_int_id != pairs[i].pack_int_id &&
 		    nth_midxed_pack(m, pairs[i-1].pack_int_id)) {
@@ -1044,12 +1085,16 @@ int verify_midx_file(struct odb_source_packed *source, unsigned flags)
 			break;
 		}
 
-		m_offset = e.offset;
+		/*
-		p_offset = find_pack_entry_one(&oid, e.p);
+		 * Check that the exact offset recorded in the MIDX
-
+		 * belongs to this OID. A pack index may contain
-		if (m_offset != p_offset)
+		 * duplicate OIDs, in which case an arbitrary OID lookup
-			midx_report(_("incorrect object offset for oid[%d] = %s: %"PRIx64" != %"PRIx64),
+		 * can return a different, equally valid copy than the
-				    pairs[i].pos, oid_to_hex(&oid), m_offset, p_offset);
+		 * one selected by the MIDX writer.
+		 */
+		if (!pack_index_has_oid_at_offset(e.p, &oid, e.offset))
+			midx_report(_("incorrect object offset for oid[%d] = %s: %"PRIx64),
+				    pairs[i].pos, oid_to_hex(&oid), e.offset);
 
 		midx_display_sparse_progress(progress, i + 1);
 	}

pack-bitmap.c

@@ -2382,7 +2382,8 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
 	struct pack_window *w_curs = NULL;
 	size_t pos = pack->bitmap_pos / BITS_IN_EWORD;
 
-	if (!pack->bitmap_pos) {
+	if (!pack->bitmap_pos &&
+	    pack->bitmap_nr == pack->p->num_objects) {
 		/*
 		 * If we're processing the first (in the case of a MIDX, the
 		 * preferred pack) or the only (in the case of single-pack
@@ -2398,6 +2399,9 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
 		 *   all ties are broken in favor of that pack (i.e. the one
 		 *   we're currently processing). So any duplicate bases will be
 		 *   resolved in favor of the pack we're processing.
+		 *
+		 * The range must also contain every physical pack entry so that
+		 * bitmap and pack positions correspond.
 		 */
 		while (pos < result->word_alloc &&
 		       pos < pack->bitmap_nr / BITS_IN_EWORD &&
@@ -2526,14 +2530,26 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
 	} else {
 		struct packed_git *pack;
 		uint32_t pack_int_id;
+		uint32_t bitmap_nr;
 
 		if (bitmap_is_midx(bitmap_git)) {
+			struct bitmapped_pack bitmapped_pack;
 			struct multi_pack_index *m = bitmap_git->midx;
 			uint32_t preferred_pack_pos;
 
 			while (m->base_midx)
 				m = m->base_midx;
 
+			if (!m->chunk_bitmapped_packs) {
+				/*
+				 * Without BTMP, determining the preferred
+				 * pack's range requires scanning the pseudo-pack.
+				 * Skip reuse and leave the bitmap result for
+				 * normal packing.
+				 */
+				return;
+			}
+
 			if (midx_preferred_pack(m, &preferred_pack_pos) < 0) {
 				warning(_("unable to compute preferred pack, disabling pack-reuse"));
 				return;
@@ -2541,6 +2557,12 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
 
 			pack = nth_midxed_pack(m, preferred_pack_pos);
 			pack_int_id = preferred_pack_pos;
+
+			if (nth_bitmapped_pack(m, &bitmapped_pack,
+					       pack_int_id) < 0)
+				return;
+			pack = bitmapped_pack.p;
+			bitmap_nr = bitmapped_pack.bitmap_nr;
 		} else {
 			pack = bitmap_git->pack;
 			/*
@@ -2553,13 +2575,14 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
 			 * that we do not expect to read this field.
 			 */
 			pack_int_id = -1;
+			bitmap_nr = pack->num_objects;
 		}
 
 		if (is_pack_valid(pack)) {
 			ALLOC_GROW(packs, packs_nr + 1, packs_alloc);
 			packs[packs_nr].p = pack;
 			packs[packs_nr].pack_int_id = pack_int_id;
-			packs[packs_nr].bitmap_nr = pack->num_objects;
+			packs[packs_nr].bitmap_nr = bitmap_nr;
 			packs[packs_nr].bitmap_pos = 0;
 			packs[packs_nr].from_midx = bitmap_git->midx;
 			packs_nr++;

packfile.c

@@ -10,6 +10,7 @@
 #include "dir.h"
 #include "packfile.h"
 #include "delta.h"
+#include "ewah/ewok.h"
 #include "hash-lookup.h"
 #include "commit.h"
 #include "object.h"
@@ -1058,6 +1059,160 @@ static int get_delta_base_oid(struct packed_git *p,
 		return -1;
 }
 
+/*
+ * Search duplicate representations for a chain ending in a full object.
+ * Representations of the same OID are interchangeable as delta bases.
+ *
+ * Each path entry is the search frame for one OID. It walks that OID's
+ * contiguous .idx entries and retains the chosen entry's .pack offset
+ * for replay.
+ */
+struct delta_path_entry {
+	off_t selected_offset; /* zero until a candidate is selected */
+	uint32_t next; /* index position of the next candidate */
+	uint32_t remaining; /* total number of candidates remaining */
+};
+
+struct delta_path {
+	struct delta_path_entry *entries;
+	size_t nr, alloc;
+};
+
+static int push_oid_group(struct packed_git *p,
+			  const struct object_id *oid,
+			  struct bitmap *visited, struct delta_path *path)
+{
+	struct object_id candidate;
+	struct delta_path_entry *entry;
+	uint32_t first_index_pos, group_index_pos, index_pos;
+
+	/*
+	 * Determine the range of index positions referring to duplicate
+	 * copies of the given object.
+	 *
+	 * Any position within that range is OK, since we will determine
+	 * the exact range below.
+	 */
+	if (!bsearch_pack(oid, p, &group_index_pos))
+		return 0;
+	if (bitmap_get(visited, group_index_pos))
+		return 0;
+
+	first_index_pos = group_index_pos;
+	while (first_index_pos > 0) {
+		if (nth_packed_object_id(&candidate, p, first_index_pos - 1) < 0)
+			return -1;
+		if (!oideq(&candidate, oid))
+			break;
+		first_index_pos--;
+	}
+
+	for (index_pos = first_index_pos; index_pos < p->num_objects; index_pos++) {
+		if (nth_packed_object_id(&candidate, p, index_pos) < 0)
+			return -1;
+		if (!oideq(&candidate, oid))
+			break;
+	}
+
+	bitmap_set(visited, group_index_pos);
+
+	ALLOC_GROW(path->entries, path->nr + 1, path->alloc);
+	entry = &path->entries[path->nr++];
+	entry->selected_offset = 0;
+	entry->next = first_index_pos;
+	entry->remaining = index_pos - first_index_pos;
+
+	return 1;
+}
+
+static enum object_type find_delta_path(struct packed_git *p,
+					struct pack_window **w_curs,
+					off_t offset,
+					struct delta_path *path)
+{
+	struct object_id oid;
+	uint32_t pack_pos;
+	struct bitmap *visited;
+	enum object_type result = OBJ_BAD;
+
+	if (offset_to_pack_pos(p, offset, &pack_pos) < 0)
+		return OBJ_BAD;
+	if (nth_packed_object_id(&oid, p, pack_pos_to_index(p, pack_pos)) < 0)
+		return OBJ_BAD;
+
+	visited = bitmap_new();
+	if (push_oid_group(p, &oid, visited, path) != 1)
+		goto done;
+
+	/*
+	 * Search depth-first for a chain ending in a full object. Each frame
+	 * tries every representation of one OID; a delta pushes its base OID,
+	 * while exhausting a frame backtracks to its parent.
+	 */
+	while (path->nr) {
+		struct delta_path_entry *entry = &path->entries[path->nr - 1];
+		enum object_type candidate_type;
+		off_t curpos;
+		size_t size;
+
+		/*
+		 * This OID has no path to a full object. Let its parent try
+		 * another representation; exhausting the root fails the search.
+		 */
+		if (!entry->remaining) {
+			path->nr--;
+			continue;
+		}
+
+		entry->selected_offset =
+			nth_packed_object_offset(p, entry->next++);
+		entry->remaining--;
+		curpos = entry->selected_offset;
+		candidate_type = unpack_object_header(p, w_curs, &curpos, &size);
+
+		/*
+		 * A full object terminates the chain, and its type is
+		 * inherited by every delta above it. A delta continues
+		 * at its base; any other type rejects only this
+		 * representation.
+		 */
+		switch (candidate_type) {
+		case OBJ_COMMIT:
+		case OBJ_TREE:
+		case OBJ_BLOB:
+		case OBJ_TAG:
+			result = candidate_type;
+			goto done;
+		case OBJ_OFS_DELTA:
+		case OBJ_REF_DELTA:
+			break;
+		default:
+			/*
+			 * A bad or unknown type rejects only this copy;
+			 * another representation of the same OID may
+			 * still work.
+			 */
+			continue;
+		}
+
+		/*
+		 * Descend to this delta's base. A malformed reference
+		 * or a missing or already-visited base rejects this
+		 * copy. A newly pushed base is examined next; an index
+		 * error aborts the search.
+		 */
+		if (get_delta_base_oid(p, w_curs, curpos, &oid, candidate_type,
+				       entry->selected_offset))
+			continue;
+		if (push_oid_group(p, &oid, visited, path) < 0)
+			goto done;
+	}
+
+done:
+	bitmap_free(visited);
+	return result;
+}
+
 static int retry_bad_packed_offset(struct repository *r,
 				   struct packed_git *p,
 				   off_t obj_offset)
@@ -1086,11 +1241,27 @@ static enum object_type packed_to_object_type(struct repository *r,
 {
 	off_t small_poi_stack[POI_STACK_PREALLOC];
 	off_t *poi_stack = small_poi_stack;
+	off_t root_offset = obj_offset;
 	int poi_stack_nr = 0, poi_stack_alloc = POI_STACK_PREALLOC;
 
 	while (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
 		off_t base_offset;
 		size_t size;
+
+		if (poi_stack_nr > 0 && poi_stack_nr % 2 == 0 &&
+		    obj_offset == poi_stack[poi_stack_nr / 2]) {
+			struct delta_path path = { 0 };
+			/*
+			 * Normal lookup returned to the same pack
+			 * entry. Restart from the requested object
+			 * using alternate representations.
+			 */
+			type = find_delta_path(p, w_curs, root_offset, &path);
+			free(path.entries);
+			if (type == OBJ_BAD)
+				goto unwind;
+			break;
+		}
 		/* Push the object we're going to leave behind */
 		if (poi_stack_nr >= poi_stack_alloc && poi_stack == small_poi_stack) {
 			poi_stack_alloc = alloc_nr(poi_stack_nr);
@@ -1506,9 +1677,11 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
 {
 	struct pack_window *w_curs = NULL;
 	off_t curpos = obj_offset;
+	off_t root_offset = obj_offset;
 	void *data = NULL;
 	size_t size;
 	enum object_type type;
+	struct delta_path path = { 0 };
 	struct unpack_entry_stack_ent small_delta_stack[UNPACK_ENTRY_STACK_PREALLOC];
 	struct unpack_entry_stack_ent *delta_stack = small_delta_stack;
 	int delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC;
@@ -1534,6 +1707,22 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
 			break;
 		}
 
+		if (!path.nr &&
+		    delta_stack_nr > 0 && delta_stack_nr % 2 == 0 &&
+		    obj_offset == delta_stack[delta_stack_nr / 2].obj_offset) {
+			/*
+			 * Normal lookup returned to the same pack
+			 * entry. Find an acyclic path if one exists,
+			 * discard this walk, and replay that path.
+			 */
+			if (find_delta_path(p, &w_curs, root_offset,
+					    &path) == OBJ_BAD)
+				break;
+			delta_stack_nr = 0;
+			curpos = obj_offset = path.entries[0].selected_offset;
+			continue;
+		}
+
 		if (do_check_packed_object_crc && p->index_version > 1) {
 			uint32_t pack_pos, index_pos;
 			off_t len;
@@ -1572,6 +1761,15 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
 			break;
 		}
 
+		/* Use the base chosen by recovery, not the one from normal lookup. */
+		if (path.nr) {
+			size_t path_pos = (size_t)delta_stack_nr + 1;
+
+			if (path_pos >= path.nr)
+				BUG("alternate delta path ends in a delta");
+			base_offset = path.entries[path_pos].selected_offset;
+		}
+
 		/* push object, proceed to base */
 		if (delta_stack_nr >= delta_stack_alloc
 		    && delta_stack == small_delta_stack) {
@@ -1712,6 +1910,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
 
 out:
 	unuse_pack(&w_curs);
+	free(path.entries);
 
 	if (delta_stack != small_delta_stack)
 		free(delta_stack);

t/helper/test-bitmap.c

@@ -50,6 +50,9 @@ static int add_packed_object(const struct object_id *oid,
 
 	oi.typep = &type;
 
+	if (packlist_find(packed, oid))
+		die("pack contains duplicate object %s", oid_to_hex(oid));
+
 	entry = packlist_alloc(packed, oid);
 	entry->idx.offset = nth_packed_object_offset(pack, pos);
 	if (packed_object_info(NULL, pack, entry->idx.offset, &oi) < 0)

t/helper/test-find-pack.c

@@ -11,12 +11,15 @@
  * Display the path(s), one per line, of the packfile(s) containing
  * the given object.
  *
+ * With '--show-offset', display the offset selected by
+ * find_pack_entry_one() instead of the packfile path.
+ *
  * If '--check-count <n>' is passed, then error out if the number of
  * packfiles containing the object is not <n>.
  */
 
 static const char *const find_pack_usage[] = {
-	"test-tool find-pack [--check-count <n>] <object>",
+	"test-tool find-pack [--check-count <n>] [--show-offset] <object>",
 	NULL
 };
 
@@ -24,11 +27,13 @@ int cmd__find_pack(int argc, const char **argv)
 {
 	struct object_id oid;
 	struct packed_git *p;
-	int count = -1, actual_count = 0;
+	int count = -1, actual_count = 0, show_offset = 0;
 	const char *prefix = setup_git_directory(the_repository);
 
 	struct option options[] = {
 		OPT_INTEGER('c', "check-count", &count, "expected number of packs"),
+		OPT_BOOL(0, "show-offset", &show_offset,
+			 "show matching pack offsets"),
 		OPT_END(),
 	};
 
@@ -40,8 +45,13 @@ int cmd__find_pack(int argc, const char **argv)
 		die("cannot parse %s as an object name", argv[0]);
 
 	repo_for_each_pack(the_repository, p) {
-		if (find_pack_entry_one(&oid, p)) {
+		off_t offset = find_pack_entry_one(&oid, p);
-			printf("%s\n", p->pack_name);
+
+		if (offset) {
+			if (show_offset)
+				printf("%"PRIuMAX"\n", (uintmax_t)offset);
+			else
+				printf("%s\n", p->pack_name);
 			actual_count++;
 		}
 	}

t/t5308-pack-detect-duplicates.sh

@@ -27,6 +27,11 @@ HI_SHA1=$EMPTY_BLOB
 # duplicate runs).
 MISSING_SHA1=$(test_oid missing_oid)
 
+# Three distinct objects for tests where physical pack order matters.
+A=$(test_oid packlib_7_0)
+B=$LO_SHA1
+C=$HI_SHA1
+
 # git will never intentionally create packfiles with
 # duplicate objects, so we have to construct them by hand.
 #
@@ -54,6 +59,13 @@ test_expect_success 'index-pack will allow duplicate objects by default' '
 	git index-pack --stdin <dups.pack
 '
 
+test_expect_success 'bitmap writer rejects duplicate objects' '
+	pack=$(ls .git/objects/pack/pack-*.pack) &&
+	test_must_fail test-tool bitmap write "$(basename "$pack")" \
+		</dev/null 2>err &&
+	test_grep "fatal: pack contains duplicate object" err
+'
+
 test_expect_success 'create batch-check test vectors' '
 	cat >input <<-EOF &&
 	$LO_SHA1
@@ -72,6 +84,54 @@ test_expect_success 'lookup in duplicated pack' '
 	test_cmp expect actual
 '
 
+test_expect_success 'verify MIDX containing duplicated pack objects' '
+	git multi-pack-index write &&
+	test-tool read-midx --show-objects .git/objects >midx-objects &&
+	midx_offset=$(
+		awk -v oid="$LO_SHA1" "\$1 == oid { print \$2 }" <midx-objects
+	) &&
+	lookup_offset=$(
+		test-tool find-pack --check-count=1 --show-offset "$LO_SHA1"
+	) &&
+	test "$midx_offset" -ne "$lookup_offset" &&
+	git multi-pack-index verify &&
+	git fsck --full
+'
+
+test_expect_success 'duplicate entries remain in pack reverse index' '
+	clear_packs &&
+	{
+		pack_header 4 &&
+		pack_obj $A &&
+		pack_obj $B &&
+		pack_obj $A &&
+		pack_obj $C
+	} >physical-order.pack &&
+	pack_trailer physical-order.pack &&
+
+	test_must_fail git index-pack --rev-index --stdin --strict \
+		<physical-order.pack 2>err &&
+	test_grep "appears twice in the pack" err &&
+
+	git index-pack --rev-index --stdin <physical-order.pack &&
+	git show-index <"$(ls .git/objects/pack/pack-*.idx)" >offsets.raw &&
+
+	sort -n offsets.raw | grep -A1 "$B" | cut -d" " -f1 >adjacent &&
+	echo $(($(tail -n1 adjacent) - $(head -n1 adjacent))) >expect &&
+	echo "$B" >in &&
+
+	GIT_TEST_REV_INDEX_DIE_IN_MEMORY=1 \
+		git cat-file --batch-check="%(objectsize:disk)" \
+		<in >actual.disk &&
+	GIT_TEST_REV_INDEX_DIE_ON_DISK=1 \
+		git -c pack.readReverseIndex=false \
+		cat-file --batch-check="%(objectsize:disk)" \
+		<in >actual.mem  &&
+
+	test_cmp expect actual.disk &&
+	test_cmp expect actual.mem
+'
+
 test_expect_success 'index-pack can reject packs with duplicates' '
 	clear_packs &&
 	create_pack dups.pack 2 &&

t/t5309-pack-delta-cycles.sh

@@ -9,6 +9,83 @@ test_description='test index-pack handling of delta cycles in packfiles'
 A=$(test_oid packlib_7_0)
 B=$(test_oid packlib_7_76)
 
+# Copy the entries from a complete pack without its header or trailer.
+pack_entries () {
+	entry_size=$(wc -c <"$1") &&
+	dd if="$1" bs=1 skip=12 \
+		count=$((entry_size - 12 - $(test_oid rawsz))) 2>/dev/null
+}
+
+# B as an OFS_DELTA against A at the given one-byte distance.
+pack_obj_b_ofs_a () {
+	pack_obj "$B" "$A" >b-ref.tmp &&
+	printf "\145" &&
+	printf "\\$(printf "%03o" "$1")" &&
+	dd if=b-ref.tmp bs=1 skip=$((1 + $(test_oid rawsz))) 2>/dev/null
+}
+
+# Return the base of the first one-byte-header REF_DELTA for the given OID.
+first_ref_base () {
+	idx=$(echo .git/objects/pack/*.idx) &&
+	offset=$(git show-index <"$idx" |
+		awk -v oid="$1" '$2 == oid { print $1; exit }') &&
+	dd if="${idx%.idx}.pack" bs=1 skip=$((offset + 1)) \
+		count=$(test_oid rawsz) 2>/dev/null |
+	test-tool hexdump |
+	tr -d " \n"
+}
+
+# The order of equal-OID entries in the .idx is unspecified. Retain a pack
+# which selects $1 as a delta against $2. Unless $3 is "-", also require the
+# first copy searched during recovery to be a REF_DELTA against $3.
+install_cycle () {
+	cycle_oid=$1 &&
+	cycle_base=$2 &&
+	first_base=$3 &&
+	shift 3 &&
+	for pack
+	do
+		clear_packs &&
+		git index-pack --fix-thin --stdin <"$pack" &&
+		selected_base=$(echo "$cycle_oid" |
+			git cat-file --batch-check="%(deltabase)") ||
+		return 1
+		if test "$selected_base" = "$cycle_base" &&
+		   { test "$first_base" = "-" ||
+		     test "$(first_ref_base "$cycle_oid")" = "$first_base"; }
+		then
+			return 0
+		fi
+	done
+	return 1
+}
+
+make_cycle_pack () {
+	cycle_pack=$1 &&
+	shift &&
+	test-tool -C alt-source pack-deltas --num-objects=6 >refs.tmp <<-EOF &&
+	REF_DELTA $T $X
+	REF_DELTA $X $1
+	REF_DELTA $Y $Z
+	REF_DELTA $Z $X
+	REF_DELTA $X $2
+	REF_DELTA $X $3
+	EOF
+	{
+		pack_header 8 &&
+		pack_entries refs.tmp &&
+		cat a-full &&
+		pack_obj_b_ofs_a "$a_full_size"
+	} >"$cycle_pack" &&
+	pack_trailer "$cycle_pack"
+}
+
+check_blob () {
+	test "$(git cat-file -t "$1")" = blob &&
+	git cat-file blob "$1" >actual &&
+	test_cmp_bin "$2" actual
+}
+
 # double-check our hand-constucted packs
 test_expect_success 'index-pack works with a single delta (A->B)' '
 	clear_packs &&
@@ -67,18 +144,77 @@ test_expect_success 'failover to an object in another pack' '
 '
 
 test_expect_success 'failover to a duplicate object in the same pack' '
-	clear_packs &&
+	{
+		pack_header 3 &&
+		pack_obj $A &&
+		pack_obj $B $A &&
+		pack_obj $A $B
+	} >recoverable-1.pack &&
+	pack_trailer recoverable-1.pack &&
 	{
 		pack_header 3 &&
 		pack_obj $A $B &&
 		pack_obj $B $A &&
 		pack_obj $A
-	} >recoverable.pack &&
+	} >recoverable-2.pack &&
-	pack_trailer recoverable.pack &&
+	pack_trailer recoverable-2.pack &&
 
-	# This cycle does not fail since the existence of a full copy
+	# The selected copy of A is part of the cycle, but the full copy
-	# of A in the pack allows us to resolve the cycle.
+	# lets both type and content lookups resolve it.
-	git index-pack --fix-thin --stdin <recoverable.pack
+	install_cycle "$A" "$B" - recoverable-1.pack recoverable-2.pack &&
+	printf "\7\0" >expect &&
+	check_blob "$A" expect
+'
+
+test_expect_success 'failover from a mixed REF/OFS cycle' '
+	pack_obj "$A" "$B" >a-ref &&
+	pack_obj "$B" >b-full &&
+	a_ref_size=$(wc -c <a-ref) &&
+	b_full_size=$(wc -c <b-full) &&
+
+	{
+		pack_header 3 &&
+		cat a-ref &&
+		cat b-full &&
+		pack_obj_b_ofs_a "$((a_ref_size + b_full_size))"
+	} >mixed-1.pack &&
+	pack_trailer mixed-1.pack &&
+	{
+		pack_header 3 &&
+		cat a-ref &&
+		pack_obj_b_ofs_a "$a_ref_size" &&
+		cat b-full
+	} >mixed-2.pack &&
+	pack_trailer mixed-2.pack &&
+
+	# The REF_DELTA for A selects the OFS_DELTA copy of B; the
+	# full B is its escape.
+	install_cycle "$B" "$A" - mixed-1.pack mixed-2.pack &&
+	printf "\7\0" >expect &&
+	check_blob "$A" expect
+'
+
+test_expect_success 'failover after a tail into a three-object delta cycle' '
+	git init alt-source &&
+	printf "\7\76" |
+		git -C alt-source hash-object -w --stdin >/dev/null &&
+	X=$(printf x | git -C alt-source hash-object -w --stdin) &&
+	Y=$(printf y | git -C alt-source hash-object -w --stdin) &&
+	Z=$(printf z | git -C alt-source hash-object -w --stdin) &&
+	printf "tail T\n" >tail &&
+	T=$(git -C alt-source hash-object -w --stdin <tail) &&
+
+	pack_obj "$A" >a-full &&
+	a_full_size=$(wc -c <a-full) &&
+	make_cycle_pack alternate-1.pack "$B" "$Y" "$Y" &&
+	make_cycle_pack alternate-2.pack "$Y" "$B" "$Y" &&
+	make_cycle_pack alternate-3.pack "$Y" "$Y" "$B" &&
+
+	# Lookup of T follows T->X->Y->Z->X. Recovery must exhaust that
+	# branch, then use X->B->A, whose final edge is an OFS_DELTA.
+	install_cycle "$X" "$Y" "$Y" \
+		alternate-1.pack alternate-2.pack alternate-3.pack &&
+	check_blob "$T" tail
 '
 
 test_expect_success 'index-pack works with thin pack A->B->C with B on disk' '

t/t5332-multi-pack-reuse.sh

@@ -4,6 +4,7 @@ test_description='pack-objects multi-pack reuse'
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bitmap.sh
+. "$TEST_DIRECTORY"/lib-pack.sh
 
 GIT_TEST_MULTI_PACK_INDEX=0
 GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=0
@@ -32,6 +33,14 @@ pack_position () {
 	grep "$1" objects | cut -d" " -f1
 }
 
+# B as an OFS_DELTA against A at the given one-byte distance.
+pack_obj_b_ofs_a () {
+	pack_obj "$B" "$A" >b-ref.tmp &&
+	printf "\145" &&
+	printf "\\$(printf "%03o" "$1")" &&
+	dd if=b-ref.tmp bs=1 skip=$((1 + $(test_oid rawsz))) 2>/dev/null
+}
+
 # test_pack_objects_reused_all <pack-reused> <packs-reused>
 test_pack_objects_reused_all () {
 	: >trace2.txt &&
@@ -288,4 +297,84 @@ test_expect_success 'duplicate objects with verbatim reuse' '
 	)
 '
 
+test_expect_success 'reuse with intra-pack duplicate objects' '
+	git init intra-pack-duplicate-objects &&
+	(
+		cd intra-pack-duplicate-objects &&
+
+		# Make enough objects to exercise whole-word reuse.
+		test_commit_bulk 20 &&
+		test_commit --printf A a "\7\0" &&
+		test_commit --printf B b "\7\76" &&
+
+		objects_nr=$(git rev-list --count --objects --all) &&
+		git rev-list --objects --all |
+		cut -d" " -f1 >objects &&
+		A=$(test_oid packlib_7_0) &&
+		B=$(test_oid packlib_7_76) &&
+		grep -v -e "^$A$" -e "^$B$" objects >rest &&
+		pack_obj "$A" >a-full &&
+		pack_obj "$B" >b-full &&
+		while read oid
+		do
+			pack_obj "$oid" || exit 1
+		done <rest >rest.entries &&
+		{
+			# Arrange the pack as A, B, A, C..., so that physical
+			# positions diverge from MIDX pseudo-pack order.
+			pack_header $((objects_nr + 1)) &&
+			cat a-full b-full a-full rest.entries
+		} >duplicate.pack &&
+		pack_trailer duplicate.pack &&
+		clear_packs &&
+		git index-pack --stdin <duplicate.pack &&
+
+		git multi-pack-index write --bitmap &&
+		git config pack.allowPackReuse single &&
+		test_pack_objects_reused_all "$objects_nr" 1 &&
+		rm -f got.idx &&
+		test_env GIT_TEST_MIDX_READ_BTMP=false \
+			test_pack_objects_reused_all 0 0
+	)
+'
+
+test_expect_success 'omit delta whose duplicate base is not selected' '
+	(
+		cd intra-pack-duplicate-objects &&
+
+		A=$(test_oid packlib_7_0) &&
+		B=$(test_oid packlib_7_76) &&
+		objects_nr=$(wc -l <objects) &&
+
+		a_size=$(wc -c <a-full) &&
+		test "$((2 * a_size))" -lt 128 &&
+
+		# The .idx order of duplicate OIDs is unspecified. Try a delta
+		# against each copy and keep the pack whose base was omitted.
+		for distance in "$((2 * a_size))" "$a_size"
+		do
+			base_offset=$((12 + 2 * a_size - distance)) &&
+			pack_obj_b_ofs_a "$distance" >b-delta &&
+			{
+				pack_header "$((objects_nr + 1))" &&
+				cat a-full a-full b-delta rest.entries
+			} >candidate.pack &&
+			pack_trailer candidate.pack &&
+			clear_packs &&
+			git index-pack --stdin <candidate.pack &&
+			git multi-pack-index write --bitmap || return 1
+
+			selected_offset=$(
+				test-tool read-midx --show-objects "$objdir" |
+				awk -v oid="$A" "\$1 == oid { print \$2 }"
+			) || return 1
+			test -n "$selected_offset" || return 1
+			test "$selected_offset" = "$base_offset" || break
+		done &&
+		test "$selected_offset" != "$base_offset" &&
+
+		test_pack_objects_reused_all "$((objects_nr - 1))" 1
+	)
+'
+
 test_done