Re: [PATCH 01/10] packfile: introduce 'find_kept_pack_entry()'

> On 1/19/2021 6:24 PM, Taylor Blau wrote:
> >  	for (m = r->objects->multi_pack_index; m; m = m->next) {
> > -		if (fill_midx_entry(r, oid, e, m))
> > +		if (!(fill_midx_entry(r, oid, e, m)))
>
> nit: we don't need extra parens around fill_midx_entry().

> > -		if (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {
> > -			list_move(&p->mru, &r->objects->packed_git_mru);
> > -			return 1;
> > +		if (p->multi_pack_index && !kept_only) {
> > +			/*
> > +			 * If this pack is covered by the MIDX, we'd have found
> > +			 * the object already in the loop above if it was here,
> > +			 * so don't bother looking.
> > +			 *
> > +			 * The exception is if we are looking only at kept
> > +			 * packs. An object can be present in two packs covered
> > +			 * by the MIDX, one kept and one not-kept. And as the
> > +			 * MIDX points to only one copy of each object, it might
> > +			 * have returned only the non-kept version above. We
> > +			 * have to check again to be thorough.
> > +			 */
> > +			continue;
> > +		}
> > +		if (!kept_only ||
> > +		    (((kept_only & ON_DISK_KEEP_PACKS) && p->pack_keep) ||
> > +		     ((kept_only & IN_CORE_KEEP_PACKS) && p->pack_keep_in_core))) {
> > +			if (fill_pack_entry(oid, e, p)) {
> > +				list_move(&p->mru, &r->objects->packed_git_mru);
> > +				return 1;
> > +			}
>
> Here is the meat of your patch. The comment helps a lot.
>
> This might have been easier if the MIDX had preferred kept packs
> over non-kept packs (before sorting by modified time). Perhaps
> the MIDX could get an extra field to say "I preferred kept packs"
> which would let us trust the MIDX return here without the pack
> loop.
>
> (Note: we can't just change the MIDX selection and then start
> trusting all MIDXs to have the right tie-breakers because of
> existing files in the wild.)