Re: [PATCH v4 00/16] midx: implement a multi-pack reverse index

From: Jeff King <peff@peff.net>
Date: Mar 30, 2021, 15:45 UTC
Message-ID: <YGNHj6iRlCOjjCL8@coredump.intra.peff.net>
In-Reply-To: <cover.1617116623.git.me@ttaylorr.com>

On Tue, Mar 30, 2021 at 11:03:44AM -0400, Taylor Blau wrote:

> Here is another reroll of my series to implement a reverse index in
> preparation for multi-pack reachability bitmaps.

Thanks, this addresses all of my comments from the last round.

> This reroll differs only in the feedback I incorporated from Peff's review. They
> are mostly cosmetic; the most substantial change being that the --preferred-pack
> code now uses bsearch() to locate the name of the preferred pack (instead of
> implementing a binary search itself).

Yeah, I read over this part carefully, since it's actual new code (that isn't run yet!), but I think it is correct.

One minor observation:

Show 17 quoted lines

>     @@ t/t5319-multi-pack-index.sh: test_expect_success 'warn on improper hash version'
>      +			write --preferred-pack=test-BC-$bc.idx 2>err &&
>      +		test_must_be_empty err &&
>      +
>     ++		echo hi &&
>     ++		test-tool read-midx --show-objects objects >out &&
>     ++
>      +		ofs=$(git show-index <objects/pack/test-BC-$bc.idx | grep $b |
>      +			cut -d" " -f1) &&
>     -+		midx_expect_object_offset $b $ofs objects
>     ++		printf "%s %s\tobjects/pack/test-BC-%s.pack\n" \
>     ++			"$b" "$ofs" "$bc" >expect &&
>     ++		grep ^$b out >actual &&
>     ++
>     ++		test_cmp expect actual
>      +	)
>      +'

I'd probably have just skipped show-index entirely, and done:

  grep "^$b .* objects/pack/test-BC" actual

which expresses the intent ($b came from that pack). But I don't mind the more exacting version (and certainly it is not worth a re-roll even if you prefer mine).

-Peff