Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack

["René Scharfe" <l.s.r@web.de>]

Am 05.04.2019 um 01:25 schrieb Jeff King:
> 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

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

>   		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é