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

[Jeff King <peff@peff.net>]

On Fri, Apr 05, 2019 at 10:05:29AM +0200, René Scharfe wrote:

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

Doh, thank you for catching that. I made that switch at the last minute
because I didn't want to pay the malloc/free cost when we had no list to
compare to. I'm surprised it works at all. :-/

I guess it doesn't, from the other message in the thread.

> 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);
> }

Hmm, maybe. It does a binary search, so I'd have to scratch my head for
a minute of whether this loose comparison is correct. I think it is
because of that final strcmp.

-Peff