Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack
- From
René Scharfe <l.s.r@web.de>
- Date
- Apr 5, 2019, 08:05 UTC
- Message-ID
- <726ad2d6-ab39-8f5e-1b2d-f991c8765a08@web.de>
- In-Reply-To
- <20190404232546.GD21839@sigill.intra.peff.net>
Am 05.04.2019 um 01:25 schrieb Jeff King:
Show 30 quoted lines
> When we have a .midx that covers many packfiles, we try to avoid opening > the .idx for those packfiles. However, there are a few problems with the > filename comparison we use: > > - we ask midx_contains_pack() about the .pack name, not the .idx name. > But it compares to the latter. > > - we compute the basename of the pack using strrchr() to find the > final slash. But that leaves an extra "/" at the start of our > string; we need to advance past it. > > That also raises the question of what to do when the name does not > have a slash at all. This should generally not happen (we always > find files in "pack/"), but it doesn't hurt to be defensive here. > > The tests don't notice because there's nothing about opening those .idx > files that would cause us to give incorrect output. It's just a little > slower. The new test checks this case by corrupting the covered .idx, > and then making sure we don't complain about it. > > Signed-off-by: Jeff King <peff@peff.net> > --- > packfile.c | 17 ++++++++++++++--- > t/t5319-multi-pack-index.sh | 14 ++++++++++++++ > 2 files changed, 28 insertions(+), 3 deletions(-) > > diff --git a/packfile.c b/packfile.c > index 054269ae5d..e7ca135ed5 100644 > --- a/packfile.c > +++ b/packfile.c
Show 6 quoted lines
> @@ -486,15 +496,16 @@ static int open_packed_git_1(struct packed_git *p)
> ssize_t read_result;
> const unsigned hashsz = the_hash_algo->rawsz;
>
> - if (!p->index_data) {
> + if (!p->index_data && the_repository->objects->multi_pack_index) {So if there is no multi_pack_index, we skip this block now...
Show 14 quoted lines
> struct multi_pack_index *m;
> - const char *pack_name = strrchr(p->pack_name, '/');
> + char *idx_name = pack_name_to_idx(pack_basename(p));
>
> for (m = the_repository->objects->multi_pack_index;
> m; m = m->next) {
> - if (midx_contains_pack(m, pack_name))
> + if (midx_contains_pack(m, idx_name))
> break;
> }
> + free(idx_name);
>
> if (!m && open_pack_index(p))
> return error("packfile %s index unavailable", p->pack_name);... which also means this open_pack_index() call isn't done anymore if there's no .midx file at all. You don't mention this change in the commit message; is it intended?
And I wonder if it would be easier overall to let midx_contains_pack() accept .pack names in addition to .idx names. Perhaps with something like this?
int cmp_idx_or_pack_name(const char *idx_or_pack_name, const char *idx_name)
{
while (*idx_name && *idx_name == *idx_or_pack_name) {
idx_name++;
idx_or_pack_name++;
}
if (!strcmp(idx_name, ".idx") && !strcmp(idx_or_pack_name, ".pack"))
return 0;
return strcmp(idx_or_pack_name, idx_name);
}René