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

* 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

Author: gitster

builtin/pack-objects.c

@@ -1188,12 +1188,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
-		 * non-preferred packs since we can't guarantee that
-		 * all duplicate objects were resolved in favor of
-		 * that pack.
+		 * We can't reuse whole chunks verbatim from non-preferred
+		 * packs or packs with entries missing from the bitmap because
+		 * bitmap and pack positions may differ.
 		 *
 		 * Even if we have a whole eword_t worth of bits that
 		 * could be reused, there may be objects between the
@@ -1202,7 +1202,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.
 		 */
@@ -1269,20 +1269,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
-				 * non-preferred pack, translate bit positions
-				 * from the MIDX pseudo-pack order back to their
-				 * pack-relative positions before attempting
-				 * reuse.
+				 * Translate MIDX bitmap positions which do not
+				 * correspond directly to physical pack positions.
 				 */
 				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

@@ -906,6 +906,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.
@@ -1015,7 +1057,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)) {
@@ -1040,12 +1081,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);
-
-		if (m_offset != p_offset)
-			midx_report(_("incorrect object offset for oid[%d] = %s: %"PRIx64" != %"PRIx64),
-				    pairs[i].pos, oid_to_hex(&oid), m_offset, p_offset);
+		/*
+		 * Check that the exact offset recorded in the MIDX
+		 * belongs to this OID. A pack index may contain
+		 * duplicate OIDs, in which case an arbitrary OID lookup
+		 * can return a different, equally valid copy than the
+		 * 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

@@ -2388,7 +2388,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 (allow_ref_delta && !pack->bitmap_pos) {
+	if (allow_ref_delta && !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
@@ -2406,6 +2407,9 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git
 		 * When REF_DELTAs are allowed, we can therefore reuse whole
 		 * words at a time without inspecting object headers. Otherwise,
 		 * inspect each object below to avoid reusing a REF_DELTA entry.
+		 *
+		 * 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 &&
@@ -2536,21 +2540,39 @@ 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;
 			}
 
 			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;
 			/*
@@ -2563,13 +2585,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++;