Merge branch 'tb/pseudo-merge-bitmap-fixes'

We created a useless pseudo-merge reachability bitmap that is about
0 commits, and attempted to include commits that are not in packs,
which made no sense.  These bugs have been corrected.

* tb/pseudo-merge-bitmap-fixes:
  pseudo-merge.c: ensure pseudo-merge groups are closed
  pseudo-merge.c: do not generate empty pseudo-merge commits
  t/t5333-pseudo-merge-bitmaps.sh: demonstrate empty pseudo-merge groups
  pack-bitmap-write.c: select pseudo-merges even for small bitmaps
  pack-bitmap: drop redundant args from `bitmap_writer_finish()`
  pack-bitmap: drop redundant args from `bitmap_writer_build()`
  pack-bitmap: drop redundant args from `bitmap_writer_build_type_index()`
  pack-bitmap: initialize `bitmap_writer_init()` with packing_data

Author: gitster

builtin/pack-objects.c

@@ -1342,10 +1342,10 @@ static void write_pack_file(void)
 
 			if (write_bitmap_index) {
 				bitmap_writer_init(&bitmap_writer,
-						   the_repository);
+						   the_repository, &to_pack);
 				bitmap_writer_set_checksum(&bitmap_writer, hash);
 				bitmap_writer_build_type_index(&bitmap_writer,
-					&to_pack, written_list, nr_written);
+							       written_list);
 			}
 
 			if (cruft)
@@ -1367,10 +1367,10 @@ static void write_pack_file(void)
 				bitmap_writer_select_commits(&bitmap_writer,
 							     indexed_commits,
 							     indexed_commits_nr);
-				if (bitmap_writer_build(&bitmap_writer, &to_pack) < 0)
+				if (bitmap_writer_build(&bitmap_writer) < 0)
 					die(_("failed to write bitmap index"));
 				bitmap_writer_finish(&bitmap_writer,
-						     written_list, nr_written,
+						     written_list,
 						     tmpname.buf, write_bitmap_options);
 				bitmap_writer_free(&bitmap_writer);
 				write_bitmap_index = 0;

midx-write.c

@@ -858,10 +858,9 @@ static int write_midx_bitmap(const char *midx_name,
 	for (i = 0; i < pdata->nr_objects; i++)
 		index[i] = &pdata->objects[i].idx;
 
-	bitmap_writer_init(&writer, the_repository);
+	bitmap_writer_init(&writer, the_repository, pdata);
 	bitmap_writer_show_progress(&writer, flags & MIDX_PROGRESS);
-	bitmap_writer_build_type_index(&writer, pdata, index,
-				       pdata->nr_objects);
+	bitmap_writer_build_type_index(&writer, index);
 
 	/*
 	 * bitmap_writer_finish expects objects in lex order, but pack_order
@@ -880,13 +879,12 @@ static int write_midx_bitmap(const char *midx_name,
 		index[pack_order[i]] = &pdata->objects[i].idx;
 
 	bitmap_writer_select_commits(&writer, commits, commits_nr);
-	ret = bitmap_writer_build(&writer, pdata);
+	ret = bitmap_writer_build(&writer);
 	if (ret < 0)
 		goto cleanup;
 
 	bitmap_writer_set_checksum(&writer, midx_hash);
-	bitmap_writer_finish(&writer, index, pdata->nr_objects, bitmap_name,
-			     options);
+	bitmap_writer_finish(&writer, index, bitmap_name, options);
 
 cleanup:
 	free(index);

pack-bitmap-write.c

@@ -41,13 +41,15 @@ static inline int bitmap_writer_nr_selected_commits(struct bitmap_writer *writer
 	return writer->selected_nr - writer->pseudo_merges_nr;
 }
 
-void bitmap_writer_init(struct bitmap_writer *writer, struct repository *r)
+void bitmap_writer_init(struct bitmap_writer *writer, struct repository *r,
+			struct packing_data *pdata)
 {
 	memset(writer, 0, sizeof(struct bitmap_writer));
 	if (writer->bitmaps)
 		BUG("bitmap writer already initialized");
 	writer->bitmaps = kh_init_oid_map();
 	writer->pseudo_merge_commits = kh_init_oid_map();
+	writer->to_pack = pdata;
 
 	string_list_init_dup(&writer->pseudo_merge_groups);
 
@@ -99,23 +101,21 @@ void bitmap_writer_show_progress(struct bitmap_writer *writer, int show)
  * Build the initial type index for the packfile or multi-pack-index
  */
 void bitmap_writer_build_type_index(struct bitmap_writer *writer,
-				    struct packing_data *to_pack,
-				    struct pack_idx_entry **index,
-				    uint32_t index_nr)
+				    struct pack_idx_entry **index)
 {
 	uint32_t i;
 
 	writer->commits = ewah_new();
 	writer->trees = ewah_new();
 	writer->blobs = ewah_new();
 	writer->tags = ewah_new();
-	ALLOC_ARRAY(to_pack->in_pack_pos, to_pack->nr_objects);
+	ALLOC_ARRAY(writer->to_pack->in_pack_pos, writer->to_pack->nr_objects);
 
-	for (i = 0; i < index_nr; ++i) {
+	for (i = 0; i < writer->to_pack->nr_objects; ++i) {
 		struct object_entry *entry = (struct object_entry *)index[i];
 		enum object_type real_type;
 
-		oe_set_in_pack_pos(to_pack, entry, i);
+		oe_set_in_pack_pos(writer->to_pack, entry, i);
 
 		switch (oe_type(entry)) {
 		case OBJ_COMMIT:
@@ -126,7 +126,7 @@ void bitmap_writer_build_type_index(struct bitmap_writer *writer,
 			break;
 
 		default:
-			real_type = oid_object_info(to_pack->repo,
+			real_type = oid_object_info(writer->to_pack->repo,
 						    &entry->idx.oid, NULL);
 			break;
 		}
@@ -569,8 +569,7 @@ static void store_selected(struct bitmap_writer *writer,
 	kh_value(writer->bitmaps, hash_pos) = stored;
 }
 
-int bitmap_writer_build(struct bitmap_writer *writer,
-			struct packing_data *to_pack)
+int bitmap_writer_build(struct bitmap_writer *writer)
 {
 	struct bitmap_builder bb;
 	size_t i;
@@ -581,17 +580,15 @@ int bitmap_writer_build(struct bitmap_writer *writer,
 	uint32_t *mapping;
 	int closed = 1; /* until proven otherwise */
 
-	writer->to_pack = to_pack;
-
 	if (writer->show_progress)
 		writer->progress = start_progress("Building bitmaps",
 						  writer->selected_nr);
 	trace2_region_enter("pack-bitmap-write", "building_bitmaps_total",
 			    the_repository);
 
-	old_bitmap = prepare_bitmap_git(to_pack->repo);
+	old_bitmap = prepare_bitmap_git(writer->to_pack->repo);
 	if (old_bitmap)
-		mapping = create_bitmap_mapping(old_bitmap, to_pack);
+		mapping = create_bitmap_mapping(old_bitmap, writer->to_pack);
 	else
 		mapping = NULL;
 
@@ -697,6 +694,10 @@ void bitmap_writer_select_commits(struct bitmap_writer *writer,
 	if (indexed_commits_nr < 100) {
 		for (i = 0; i < indexed_commits_nr; ++i)
 			bitmap_writer_push_commit(writer, indexed_commits[i], 0);
+
+		select_pseudo_merges(writer, indexed_commits,
+				     indexed_commits_nr);
+
 		return;
 	}
 
@@ -1001,7 +1002,6 @@ void bitmap_writer_set_checksum(struct bitmap_writer *writer,
 
 void bitmap_writer_finish(struct bitmap_writer *writer,
 			  struct pack_idx_entry **index,
-			  uint32_t index_nr,
 			  const char *filename,
 			  uint16_t options)
 {
@@ -1034,12 +1034,13 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 	dump_bitmap(f, writer->tags);
 
 	if (options & BITMAP_OPT_LOOKUP_TABLE)
-		CALLOC_ARRAY(offsets, index_nr);
+		CALLOC_ARRAY(offsets, writer->to_pack->nr_objects);
 
 	for (i = 0; i < bitmap_writer_nr_selected_commits(writer); i++) {
 		struct bitmapped_commit *stored = &writer->selected[i];
 		int commit_pos = oid_pos(&stored->commit->object.oid, index,
-					 index_nr, oid_access);
+					 writer->to_pack->nr_objects,
+					 oid_access);
 
 		if (commit_pos < 0)
 			BUG(_("trying to write commit not in index"));
@@ -1055,7 +1056,7 @@ void bitmap_writer_finish(struct bitmap_writer *writer,
 		write_lookup_table(writer, f, offsets);
 
 	if (options & BITMAP_OPT_HASH_CACHE)
-		write_hash_cache(f, index, index_nr);
+		write_hash_cache(f, index, writer->to_pack->nr_objects);
 
 	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
 			  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);

pack-bitmap.h

@@ -123,14 +123,13 @@ struct bitmap_writer {
 	unsigned char pack_checksum[GIT_MAX_RAWSZ];
 };
 
-void bitmap_writer_init(struct bitmap_writer *writer, struct repository *r);
+void bitmap_writer_init(struct bitmap_writer *writer, struct repository *r,
+			struct packing_data *pdata);
 void bitmap_writer_show_progress(struct bitmap_writer *writer, int show);
 void bitmap_writer_set_checksum(struct bitmap_writer *writer,
 				const unsigned char *sha1);
 void bitmap_writer_build_type_index(struct bitmap_writer *writer,
-				    struct packing_data *to_pack,
-				    struct pack_idx_entry **index,
-				    uint32_t index_nr);
+				    struct pack_idx_entry **index);
 int bitmap_writer_has_bitmapped_object_id(struct bitmap_writer *writer,
 					  const struct object_id *oid);
 void bitmap_writer_push_commit(struct bitmap_writer *writer,
@@ -147,11 +146,9 @@ struct ewah_bitmap *pseudo_merge_bitmap_for_commit(struct bitmap_index *bitmap_g
 void bitmap_writer_select_commits(struct bitmap_writer *writer,
 				  struct commit **indexed_commits,
 				  unsigned int indexed_commits_nr);
-int bitmap_writer_build(struct bitmap_writer *writer,
-			struct packing_data *to_pack);
+int bitmap_writer_build(struct bitmap_writer *writer);
 void bitmap_writer_finish(struct bitmap_writer *writer,
 			  struct pack_idx_entry **index,
-			  uint32_t index_nr,
 			  const char *filename,
 			  uint16_t options);
 void bitmap_writer_free(struct bitmap_writer *writer);

pseudo-merge.c

@@ -218,6 +218,8 @@ static int find_pseudo_merge_group_for_ref(const char *refname,
 	c = lookup_commit(the_repository, oid);
 	if (!c)
 		return 0;
+	if (!packlist_find(writer->to_pack, oid))
+		return 0;
 
 	has_bitmap = bitmap_writer_has_bitmapped_object_id(writer, oid);
 
@@ -358,8 +360,10 @@ static void select_pseudo_merges_1(struct bitmap_writer *writer,
 			p = commit_list_append(c, p);
 		} while (j % group->stable_size);
 
-		bitmap_writer_push_commit(writer, merge, 1);
-		writer->pseudo_merges_nr++;
+		if (merge->parents) {
+			bitmap_writer_push_commit(writer, merge, 1);
+			writer->pseudo_merges_nr++;
+		}
 	}
 
 	/* make up to group->max_merges pseudo merges for unstable commits */
@@ -399,8 +403,9 @@ static void select_pseudo_merges_1(struct bitmap_writer *writer,
 			p = commit_list_append(c, p);
 		}
 
-		bitmap_writer_push_commit(writer, merge, 1);
-		writer->pseudo_merges_nr++;
+		if (merge->parents) {
+			bitmap_writer_push_commit(writer, merge, 1);
+			writer->pseudo_merges_nr++; }
 		if (end >= matches->unstable_nr)
 			break;
 	}

t/t5333-pseudo-merge-bitmaps.sh

@@ -390,4 +390,60 @@ test_expect_success 'pseudo-merge reuse' '
 	)
 '
 
+test_expect_success 'empty pseudo-merge group' '
+	git init pseudo-merge-empty-group &&
+	(
+		cd pseudo-merge-empty-group &&
+
+		# Ensure that a pseudo-merge group with no unstable
+		# commits does not generate an empty pseudo-merge
+		# bitmap.
+		git config bitmapPseudoMerge.empty.pattern refs/ &&
+
+		test_commit base &&
+		git repack -adb &&
+
+		test-tool bitmap dump-pseudo-merges >merges &&
+		test_line_count = 1 merges &&
+
+		test 0 -eq "$(grep -c commits=0 <merges)"
+	)
+'
+
+test_expect_success 'pseudo-merge closure' '
+	git init pseudo-merge-closure &&
+	(
+		cd pseudo-merge-closure &&
+
+		test_commit A &&
+		git repack -d &&
+
+		test_commit B &&
+
+		# Note that the contents of A is packed, but B is not. A
+		# (and the objects reachable from it) are thus visible
+		# to the MIDX, but the same is not true for B and its
+		# objects.
+		#
+		# Ensure that we do not attempt to create a pseudo-merge
+		# for B, depsite it matching the below pseudo-merge
+		# group pattern, as doing so would result in a failure
+		# to write a non-closed bitmap.
+		git config bitmapPseudoMerge.test.pattern refs/ &&
+		git config bitmapPseudoMerge.test.threshold now &&
+
+		git multi-pack-index write --bitmap &&
+
+		test-tool bitmap dump-pseudo-merges >pseudo-merges &&
+		test_line_count = 1 pseudo-merges &&
+
+		git rev-parse A >expect &&
+
+		test-tool bitmap list-commits >actual &&
+		test_cmp expect actual &&
+		test-tool bitmap dump-pseudo-merge-commits 0 >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done