Re: [PATCH 11/16] repack-geometry: prepare for incremental MIDX repacking

[Taylor Blau <me@ttaylorr.com>]

On Thu, Apr 16, 2026 at 03:51:32PM -0700, Elijah Newren wrote:
> Hi,
>
> On Sun, Mar 29, 2026 at 2:41 PM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > ...If the tip layer has fewer packs
> > than the threshold, those packs are excluded entirely; otherwise only
> > packs in that layer participate in the geometric repack.
>
> "fewer packs than the threshold"; I'll come back to this below...
>
> >         repo_for_each_pack(existing->repo, p) {
> > +               if (geometry->midx_layer_threshold_set && m &&
> > +                   p->multi_pack_index) {
> > +                       /*
> > +                        * When writing MIDX layers incrementally,
> > +                        * ignore packs unless they are in the most
> > +                        * recent MIDX layer *and* there are at least
> > +                        * 'midx_layer_threshold' packs in that layer.
> > +                        *
> > +                        * Otherwise 'p' is either in an older layer, or
> > +                        * the youngest layer does not have enough packs
> > +                        * to consider its packs as candidates for
> > +                        * repacking. In either of those cases we want
> > +                        * to ignore the pack.
> > +                        */
> > +                       if (m->num_packs > geometry->midx_layer_threshold &&
> > +                           midx_layer_contains_pack(m, pack_basename(p)))
> > +                               ;
> > +                       else
> > +                               continue;
>
> Should the commit message use "no more packs than" rather than "fewer
> packs than"?  Alternatively, should this be >=  rather than >?  Or am
> I just mis-reading?

I think the goal was to have "fewer packs than" so the commit message is
right here and the implementation is wrong.

> Also, the empty-if looks a bit weird; would it make sense to invert
> the condition to get rid of the empty-if?  In other words, something
> like:
>
>     if (m->num_packs <= geometry->midx_layer_threshold ||
>        !midx_layer_contains_pack(m, pack_basename(p)))
>             continue;

I sometimes will use the empty-if style when the conditional is easier
to explain in the positive than the negative, but reading it out like
this I think that the conditional as written here is still clear. Thanks
for the suggestion!

With the above "no more than" vs "fewer than", I changed this as
follows:

--- 8< ---
diff --git a/repack-geometry.c b/repack-geometry.c
index 5b554da89f1..0469dc6496a 100644
--- a/repack-geometry.c
+++ b/repack-geometry.c
@@ -49,10 +49,8 @@ void pack_geometry_init(struct pack_geometry *geometry,
 			 * repacking. In either of those cases we want
 			 * to ignore the pack.
 			 */
-			if (m->num_packs > geometry->midx_layer_threshold &&
-			    midx_layer_contains_pack(m, pack_basename(p)))
-				;
-			else
+			if (m->num_packs < geometry->midx_layer_threshold ||
+			    !midx_layer_contains_pack(m, pack_basename(p)))
 				continue;
 		}
--- >8 ---

> > @@ -173,6 +196,20 @@ void pack_geometry_split(struct pack_geometry *geometry)
> >         geometry->promisor_split = compute_pack_geometry_split(geometry->promisor_pack,
> >                                                                geometry->promisor_pack_nr,
> >                                                                geometry->split_factor);
> > +       for (uint32_t i = 0; i < geometry->split; i++) {
> > +               struct packed_git *p = geometry->pack[i];
> > +               /*
> > +                * During incremental MIDX/bitmap repacking, any packs
> > +                * included in the rollup are either (a) not MIDX'd, or
> > +                * (b) contained in the tip layer iff it has more than
> > +                * the threshold number of packs.
>
> This wording looks consistent with the previous check, but not the
> commit message.  If the commit message is deemed correct, then you'd
> probably need to update not only the above comparison but this comment
> as well.

Great catch, I would have definitely missed this if you hadn't pointed
it out.

Thanks,
Taylor