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

[Derrick Stolee <stolee@gmail.com>]

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

Thanks,
-Stolee