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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Mar 30, 2021 at 11:45:19AM -0400, Jeff King wrote:
> > 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:
>
> >     @@ 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