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

[Taylor Blau <ttaylorr@openai.com>]

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.
+	 *
+	 * 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.
 	 *
 	 * If we added support for that, these next two conditional
 	 * should be performed independently (likely checking
--- >8 ---

Thanks,
Taylor