Re: [PATCH v2 15/15] pack-revindex: write multi-pack reverse indexes

On Tue, Mar 02, 2021 at 10:40:33AM -0800, Jonathan Tan wrote:

quoted

@@ -1018,6 +1080,14 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *

 	finalize_hashfile(f, midx_hash, CSUM_FSYNC | CSUM_HASH_IN_STREAM);
 	free_chunkfile(cf);
+
+	if (flags & MIDX_WRITE_REV_INDEX)
+		ctx.pack_order = midx_pack_order(&ctx);
+
+	if (flags & MIDX_WRITE_REV_INDEX)
+		write_midx_reverse_index(midx_name, midx_hash, &ctx);
+	clear_midx_files_ext(the_repository, ".rev", midx_hash);
+
 	commit_lock_file(&lk);

 cleanup:

Any reason why we're using 2 separate "if" statements?

Yeah. This first if statement will turn into:

  if (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))

so that the pack order is computed in either case (since both the
existing write_midx_reverse_index() and the eventual write_midx_bitmap()
will be able to use the pack order).

Arguably there is never a practical reason to write one without the
other (and writing a MIDX bitmap without a reverse index is a bug), so
perhaps these options should be consolidated.

But that's cleanup that I'd rather do after all of this has settled
(since it'd be weird to say: "here's the option to write bitmaps, except
we can't write multi-pack bitmaps yet, but setting it actually writes
this other thing").

Other than that, this patch and patch 14 look good. Besides all my minor
comments, I think the overall patch set is in good shape and ready to be
merged. It's great that we could reuse some of the individual-pack reverse
index concepts and code too.

Thanks, I am really glad that you had a chance to take a look at it. I
always find your review quite helpful.

Thanks,
Taylor