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

On Tue, Mar 30, 2021 at 11:45:19AM -0400, Jeff King wrote:

quoted

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.

Thankfully this does have coverage via any test that passes
`--preferred-pack` (like the one below).

One minor observation:

quoted

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

I originally wrote it that way, but decided to write both expect and
actual to make debugging easier if this ever regresses. Not like it's
that hard to run the test-tool yourself in the trash directory, but
having a snapshot of that object from the MIDX's perspective might make
things a little easier.

Anyway, I agree with you that it doesn't probably matter a ton either
way.

-Peff

Thanks,
Taylor