Re: [PATCH 3/3] midx-write: include packs above custom incremental base

[Taylor Blau <ttaylorr@openai.com>]

On Thu, Aug 13, 2026 at 10:49:05AM +0200, Patrick Steinhardt wrote:
> On Fri, Jun 12, 2026 at 04:07:14PM -0400, Taylor Blau wrote:
> > diff --git a/midx-write.c b/midx-write.c
> > index aa438775ebd..c50fdb5c6d1 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,
> >  static int should_include_pack(const struct write_midx_context *ctx,
> >  			       const char *file_name)
> >  {
> > +	struct multi_pack_index *m = ctx->m;
> >  	/*
> > -	 * Note that at most one of ctx->m and ctx->to_include are set,
> > +	 * When writing incrementally, ctx->m may contain layers above
> > +	 * the selected base MIDX, which must be included in the new
> > +	 * layer.
> > +	 */
> > +	if (ctx->incremental)
> > +		m = ctx->base_midx;
> > +	/*
> > +	 * Note that at most one of m and ctx->to_include are set,
>
> Is that true? With "--stdin-packs --incremental --base=<foo>" I'd expect
> that we have both set now.

That invariant holds for `ctx->m`j, but not for the local m after the
assignment above. With '--stdin-packs', `write_midx_internal()` leaves
`ctx->m` unset, but can still set `ctx->base_midx` for an incremental
write.  Once we assign `m = ctx->base_midx`, both `m` and
`ctx->to_include` can indeed be non-NULL.

The filtering still does the right thing: packs covered by the selected
base are excluded, and the remaining packs are checked against the stdin
list. But the comment is wrong, so I'll fix it.

> Okay, previously we were always checking against `ctx->m`, so we
> would exclude packs that are contained in the current MIDX. And that
> includes the case where parts of the current MIDX are supposed to be
> thrown away because we want to write a new layer that excludes all
> layers starting at the base.

On the non- '--stdin-packs' path, yes. With '--stdin-packs', `ctx->m` is
`NULL` and the old code already checks `ctx->base_midx`. The problem
appears when the previous patch starts honoring '--base' on the ordinary
write path. Since `ctx->m` still refers to the entire existing chain, it
excludes packs from layers above the selected base.

> This is fixed by instead always comparing against the base MIDX in case
> "--incremental" was passed. When the user passes "--base=none" we don't
> have any base, and consequently we'd include all packs. Otherwise, we'll
> exclude all packs that are already covered by our base, but include all
> the other ones.
>
> That feels sensible to me.

Exactly.

Thanks,
Taylor