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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 3, 2021, 15:30 UTC
Message-ID: <YD+rlaYGylVSkg1D@nand.local>
In-Reply-To: <20210302184033.12305-1-jonathantanmy@google.com>

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

Show 17 quoted lines

> > @@ -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