On Tue, Mar 02, 2021 at 10:40:33AM -0800, Jonathan Tan wrote: > > @@ -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
