Merge branch 'tb/prepare-midx-pack-cleanup' into jch

Improvement on Multi-pack-index API.

Comments?

* tb/prepare-midx-pack-cleanup:
  midx: return a `packed_git` pointer from `prepare_midx_pack()`
  midx-write.c: extract inner loop from fill_packs_from_midx()
  midx-write.c: guard against incremental MIDXs in want_included_pack()
  midx: access pack names through `nth_midxed_pack_name()`

Author: gitster

midx-write.c

@@ -938,44 +938,52 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,
 	return result;
 }
 
+static int fill_packs_from_midx_1(struct write_midx_context *ctx,
+				  struct multi_pack_index *m,
+				  int prepare_packs)
+{
+	for (uint32_t i = 0; i < m->num_packs; i++) {
+		struct packed_git *p = NULL;
+
+		ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);
+		if (prepare_packs) {
+			p = prepare_midx_pack(ctx->repo, m,
+					      m->num_packs_in_base + i);
+			if (!p) {
+				error(_("could not load pack"));
+				return 1;
+			}
+
+			if (open_pack_index(p))
+				die(_("could not open index for %s"),
+				    p->pack_name);
+		}
+
+		fill_pack_info(&ctx->info[ctx->nr++], p, m->pack_names[i],
+			       m->num_packs_in_base + i);
+	}
+
+	return 0;
+}
+
 static int fill_packs_from_midx(struct write_midx_context *ctx,
 				const char *preferred_pack_name, uint32_t flags)
 {
 	struct multi_pack_index *m;
+	int prepare_packs;
 
-	for (m = ctx->m; m; m = m->base_midx) {
-		uint32_t i;
-
-		for (i = 0; i < m->num_packs; i++) {
-			ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);
-
-			/*
-			 * If generating a reverse index, need to have
-			 * packed_git's loaded to compare their
-			 * mtimes and object count.
-			 *
-			 * If a preferred pack is specified, need to
-			 * have packed_git's loaded to ensure the chosen
-			 * preferred pack has a non-zero object count.
-			 */
-			if (flags & MIDX_WRITE_REV_INDEX ||
-			    preferred_pack_name) {
-				if (prepare_midx_pack(ctx->repo, m,
-						      m->num_packs_in_base + i)) {
-					error(_("could not load pack"));
-					return 1;
-				}
-
-				if (open_pack_index(m->packs[i]))
-					die(_("could not open index for %s"),
-					    m->packs[i]->pack_name);
-			}
+	/*
+	 * If generating a reverse index, need to have packed_git's
+	 * loaded to compare their mtimes and object count.
+	 */
+	prepare_packs = !!(flags & MIDX_WRITE_REV_INDEX || preferred_pack_name);
 
-			fill_pack_info(&ctx->info[ctx->nr++], m->packs[i],
-				       m->pack_names[i],
-				       m->num_packs_in_base + i);
-		}
+	for (m = ctx->m; m; m = m->base_midx) {
+		int ret = fill_packs_from_midx_1(ctx, m, prepare_packs);
+		if (ret)
+			return ret;
 	}
+
 	return 0;
 }
 
@@ -1578,20 +1586,19 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
 					  _("Finding and deleting unreferenced packfiles"),
 					  m->num_packs);
 	for (i = 0; i < m->num_packs; i++) {
+		struct packed_git *p;
 		char *pack_name;
 		display_progress(progress, i + 1);
 
 		if (count[i])
 			continue;
 
-		if (prepare_midx_pack(r, m, i))
-			continue;
-
-		if (m->packs[i]->pack_keep || m->packs[i]->is_cruft)
+		p = prepare_midx_pack(r, m, i);
+		if (!p || p->pack_keep || p->is_cruft)
 			continue;
 
-		pack_name = xstrdup(m->packs[i]->pack_name);
-		close_pack(m->packs[i]);
+		pack_name = xstrdup(p->pack_name);
+		close_pack(p);
 
 		string_list_insert(&packs_to_drop, m->pack_names[i]);
 		unlink_pack_path(pack_name, 0);
@@ -1636,9 +1643,12 @@ static int want_included_pack(struct repository *r,
 			      uint32_t pack_int_id)
 {
 	struct packed_git *p;
-	if (prepare_midx_pack(r, m, pack_int_id))
+
+	ASSERT(m && !m->base_midx);
+
+	p = prepare_midx_pack(r, m, pack_int_id);
+	if (!p)
 		return 0;
-	p = m->packs[pack_int_id];
 	if (!pack_kept_objects && p->pack_keep)
 		return 0;
 	if (p->is_cruft)
@@ -1655,6 +1665,8 @@ static void fill_included_packs_all(struct repository *r,
 	uint32_t i;
 	int pack_kept_objects = 0;
 
+	ASSERT(m && !m->base_midx);
+
 	repo_config_get_bool(r, "repack.packkeptobjects", &pack_kept_objects);
 
 	for (i = 0; i < m->num_packs; i++) {
@@ -1675,17 +1687,18 @@ static void fill_included_packs_batch(struct repository *r,
 	struct repack_info *pack_info;
 	int pack_kept_objects = 0;
 
+	ASSERT(m && !m->base_midx);
+
 	CALLOC_ARRAY(pack_info, m->num_packs);
 
 	repo_config_get_bool(r, "repack.packkeptobjects", &pack_kept_objects);
 
 	for (i = 0; i < m->num_packs; i++) {
-		pack_info[i].pack_int_id = i;
-
-		if (prepare_midx_pack(r, m, i))
-			continue;
+		struct packed_git *p = prepare_midx_pack(r, m, i);
 
-		pack_info[i].mtime = m->packs[i]->mtime;
+		pack_info[i].pack_int_id = i;
+		if (p)
+			pack_info[i].mtime = p->mtime;
 	}
 
 	for (i = 0; i < m->num_objects; i++) {

midx.c

@@ -451,50 +451,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m,
 	return pack_int_id - m->num_packs_in_base;
 }
 
-int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
-		      uint32_t pack_int_id)
+struct packed_git *prepare_midx_pack(struct repository *r,
+				     struct multi_pack_index *m,
+				     uint32_t pack_int_id)
 {
-	struct strbuf pack_name = STRBUF_INIT;
-	struct strbuf key = STRBUF_INIT;
-	struct packed_git *p;
-
-	pack_int_id = midx_for_pack(&m, pack_int_id);
-
-	if (m->packs[pack_int_id] == MIDX_PACK_ERROR)
-		return 1;
-	if (m->packs[pack_int_id])
-		return 0;
-
-	strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir,
-		    m->pack_names[pack_int_id]);
-
-	/* pack_map holds the ".pack" name, but we have the .idx */
-	strbuf_addbuf(&key, &pack_name);
-	strbuf_strip_suffix(&key, ".idx");
-	strbuf_addstr(&key, ".pack");
-	p = hashmap_get_entry_from_hash(&r->objects->pack_map,
-					strhash(key.buf), key.buf,
-					struct packed_git, packmap_ent);
-	if (!p) {
-		p = add_packed_git(r, pack_name.buf, pack_name.len, m->local);
-		if (p) {
-			install_packed_git(r, p);
-			list_add_tail(&p->mru, &r->objects->packed_git_mru);
+	uint32_t pack_pos = midx_for_pack(&m, pack_int_id);
+
+	if (!m->packs[pack_pos]) {
+		struct strbuf pack_name = STRBUF_INIT;
+		struct strbuf key = STRBUF_INIT;
+		struct packed_git *p;
+
+		strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir,
+			    m->pack_names[pack_pos]);
+
+		/* pack_map holds the ".pack" name, but we have the .idx */
+		strbuf_addbuf(&key, &pack_name);
+		strbuf_strip_suffix(&key, ".idx");
+		strbuf_addstr(&key, ".pack");
+		p = hashmap_get_entry_from_hash(&r->objects->pack_map,
+						strhash(key.buf), key.buf,
+						struct packed_git, packmap_ent);
+		if (!p) {
+			p = add_packed_git(r, pack_name.buf, pack_name.len,
+					   m->local);
+			if (p) {
+				install_packed_git(r, p);
+				list_add_tail(&p->mru,
+					      &r->objects->packed_git_mru);
+			}
 		}
-	}
 
-	strbuf_release(&pack_name);
-	strbuf_release(&key);
+		strbuf_release(&pack_name);
+		strbuf_release(&key);
 
-	if (!p) {
-		m->packs[pack_int_id] = MIDX_PACK_ERROR;
-		return 1;
+		m->packs[pack_pos] = p ? p : MIDX_PACK_ERROR;
+		if (p)
+			p->multi_pack_index = 1;
 	}
 
-	p->multi_pack_index = 1;
-	m->packs[pack_int_id] = p;
-
-	return 0;
+	if (m->packs[pack_pos] == MIDX_PACK_ERROR)
+		return NULL;
+	return m->packs[pack_pos];
 }
 
 struct packed_git *nth_midxed_pack(struct multi_pack_index *m,
@@ -506,6 +504,13 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m,
 	return m->packs[local_pack_int_id];
 }
 
+const char *nth_midxed_pack_name(struct multi_pack_index *m,
+				 uint32_t pack_int_id)
+{
+	uint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);
+	return m->pack_names[local_pack_int_id];
+}
+
 #define MIDX_CHUNK_BITMAPPED_PACKS_WIDTH (2 * sizeof(uint32_t))
 
 int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,
@@ -516,10 +521,11 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,
 	if (!m->chunk_bitmapped_packs)
 		return error(_("MIDX does not contain the BTMP chunk"));
 
-	if (prepare_midx_pack(r, m, pack_int_id))
-		return error(_("could not load bitmapped pack %"PRIu32), pack_int_id);
+	bp->p = prepare_midx_pack(r, m, pack_int_id);
+	if (!bp->p)
+		return error(_("could not load bitmapped pack %"PRIu32),
+			     pack_int_id);
 
-	bp->p = m->packs[local_pack_int_id];
 	bp->bitmap_pos = get_be32((char *)m->chunk_bitmapped_packs +
 				  MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id);
 	bp->bitmap_nr = get_be32((char *)m->chunk_bitmapped_packs +
@@ -616,9 +622,9 @@ int fill_midx_entry(struct repository *r,
 	midx_for_object(&m, pos);
 	pack_int_id = nth_midxed_pack_int_id(m, pos);
 
-	if (prepare_midx_pack(r, m, pack_int_id))
+	p = prepare_midx_pack(r, m, pack_int_id);
+	if (!p)
 		return 0;
-	p = m->packs[pack_int_id - m->num_packs_in_base];
 
 	/*
 	* We are about to tell the caller where they can locate the
@@ -919,7 +925,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 						  _("Looking for referenced packfiles"),
 						  m->num_packs + m->num_packs_in_base);
 	for (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {
-		if (prepare_midx_pack(r, m, i))
+		if (!prepare_midx_pack(r, m, i))
 			midx_report("failed to load pack in position %d", i);
 
 		display_progress(progress, i + 1);

midx.h

@@ -104,9 +104,13 @@ void get_split_midx_filename_ext(const struct git_hash_algo *hash_algo,
 struct multi_pack_index *load_multi_pack_index(struct repository *r,
 					       const char *object_dir,
 					       int local);
-int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);
+struct packed_git *prepare_midx_pack(struct repository *r,
+				     struct multi_pack_index *m,
+				     uint32_t pack_int_id);
 struct packed_git *nth_midxed_pack(struct multi_pack_index *m,
 				   uint32_t pack_int_id);
+const char *nth_midxed_pack_name(struct multi_pack_index *m,
+				 uint32_t pack_int_id);
 int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,
 		       struct bitmapped_pack *bp, uint32_t pack_int_id);
 int bsearch_one_midx(const struct object_id *oid, struct multi_pack_index *m,

pack-bitmap.c

@@ -502,9 +502,9 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	}
 
 	for (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {
-		if (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {
+		if (!prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {
 			warning(_("could not open pack %s"),
-				bitmap_git->midx->pack_names[i]);
+				nth_midxed_pack_name(bitmap_git->midx, i));
 			goto cleanup;
 		}
 	}
@@ -2475,7 +2475,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
 			struct bitmapped_pack pack;
 			if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {
 				warning(_("unable to load pack: '%s', disabling pack-reuse"),
-					bitmap_git->midx->pack_names[i]);
+					nth_midxed_pack_name(bitmap_git->midx, i));
 				free(packs);
 				return;
 			}

t/helper/test-read-midx.c

@@ -53,8 +53,9 @@ static int read_midx_file(const char *object_dir, const char *checksum,
 	printf("\nnum_objects: %d\n", m->num_objects);
 
 	printf("packs:\n");
-	for (i = 0; i < m->num_packs; i++)
-		printf("%s\n", m->pack_names[i]);
+	for (i = m->num_packs_in_base; i < m->num_packs + m->num_packs_in_base;
+	     i++)
+		printf("%s\n", nth_midxed_pack_name(m, i));
 
 	printf("object-dir: %s\n", m->object_dir);
 
@@ -108,7 +109,7 @@ static int read_midx_preferred_pack(const char *object_dir)
 		return 1;
 	}
 
-	printf("%s\n", midx->pack_names[preferred_pack]);
+	printf("%s\n", nth_midxed_pack_name(midx, preferred_pack));
 	close_midx(midx);
 	return 0;
 }