Re: [PATCH 3/3] midx-write: include packs above custom incremental base
From: Patrick Steinhardt <hidden>
Date: 2026-08-31 06:12:29
On Wed, Aug 26, 2026 at 06:38:15PM -0500, Taylor Blau wrote:
quoted hunk ↗ jump to hunk
On Wed, Aug 26, 2026 at 02:37:23PM -0700, Junio C Hamano wrote:quoted
Taylor Blau [off-list ref] writes:quoted
`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