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
