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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jan 20, 2021 at 08:40:22AM -0500, Derrick Stolee wrote:
> 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().

Yep. I checked whether we should have written this as "if
(fill_midx_entry(...) < 0)", but fill_midx_entry returns a positive
number on error, so checking "!fill_midx_entry" is certainly what we
should be doing.

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

Yeah, that is what makes it tricky. Changing the code isn't so hard: a
new field that we check and do one of two things when we're breaking
ties.

But I think the cognitive load is high, and I'm not sure that the
benefit (skipping another linear pass through non-MIDX'd packs when
looking up an object in kept packs only _and_ that object is duplicated)
is worth the extra hassle with the MIDX code.

All of that said, I do think that it's worth revisiting this and giving
it some more thought after multi-pack bitmaps to see whether we feel the
same or not.

Thanks,
Taylor