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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 26, 2026 at 06:38:15PM -0500, Taylor Blau wrote:
> On Wed, Aug 26, 2026 at 02:37:23PM -0700, Junio C Hamano wrote:
> > Taylor Blau <ttaylorr@openai.com> writes:
> >
> > > `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.
> >
> > Has anything happened since we saw this comment on Aug 13th?
> 
> Not until you sent this message ;-).
> 
> I had a small reroll prepped that I had meant to send a couple of weeks
> ago but never got around to doing so. When I looked at it just now, I
> found that I wasn't quite satisfied with the range-diff in that the
> resulting block comment was somewhat confusing.
> 
> Instead of sending a new round immediately, let me instead share the
> comment that I wrote instead. Patrick (or others): does this comment
> seem clear, or do you think there are ways to tighten it up further?
> 
> --- 8< ---
> diff --git a/midx-write.c b/midx-write.c
> index 66da608370..ff94076104 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -143,15 +143,31 @@ static int should_include_pack(const struct write_midx_context *ctx,
>  		m = ctx->base_midx;
> 
>  	/*
> -	 * Note that m and ctx->to_include may both be set,
> -	 * so we are testing midx_contains_pack() and
> -	 * string_list_has_string() independently (guarded by the
> -	 * appropriate NULL checks).
> -	 *
> -	 * We could support passing to_include while reusing an existing
> -	 * MIDX, but don't currently since the reuse process drags
> -	 * forward all packs from an existing MIDX (without checking
> -	 * whether or not they appear in the to_include list).
> +	 * Note that it is OK for both ctx->base_midx and
> +	 * ctx->to_include may both be non-NULL, but at most one of
> +	 * ctx->m and ctx->to_include may be non-NULL.

That reads a bit off. Should that be "Note that it is OK for both ... to
be non-NULL" instead?

> +	 * When ctx->m is NULL we are writing a new MIDX without reusing
> +	 * any packs from the previous layer(s). In that case, we care
> +	 * that both:
> +	 *
> +	 *   - the new layer's base MIDX (ctx->base_midx) does not
> +	 *     already contain the pack we are considering, or the new
> +	 *     layer has no base (i.e., it is a non-incremental MIDX)
> +	 *
> +	 *   - the pack appears in ctx->to_include, or ctx->to_include
> +	 *     is NULL, meaning that we can include any pack provided
> +	 *     the above condition is met.
> +	 *
> +	 * When ctx->m is non-NULL, we are writing a new MIDX that will
> +	 * subsume ctx->m and thus includes its packs. In this case, we
> +	 * could support respecting ctx->to_include, but currently
> +	 * don't.
> +	 *
> +	 * The only caller of this function which permits
> +	 * ctx->to_include being non-NULL restricts setting ctx->m when
> +	 * this is the case. So in this setting it is impossible that
> +	 * both will be non-NULL.

I feel like this last paragraph could be dropped -- it's something that
we could mention as part of the commit message, but in this function
here I think it's very likely to go stale fast.

Other than that I think this is good. It's quite long, but I don't have
any good ideas for how to tighten this up significantly.

Thanks!

Patrick