Re: [PATCH 05/16] midx: introduce `--checksum-only` for incremental MIDX writes

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 30, 2026 at 07:15:47PM -0400, Jeff King wrote:
> Your goal makes sense, but that is not at all what I would have expected
> a "--checksum-only" option to do. From the name I expected it to just
> compute or verify checksums and not write anything to disk. I dunno.
> Maybe it should be "--no-write-chain-file" or something?
>
> This is perhaps on the border of bike-shedding, and maybe the fact that
> it is an option of "multi-pack-index write" would be enough to clarify.

Heh :-).

I think that it definitely qualifies as bike-shedding, but I'm glad you
brought it up. I hadn't considered the name being potentially confusing,
but now that you mention it I can't un-see it.

Calling this something like '--no-write-chain-file' feels like a good
choice to me, and doesn't paint us into a corner if we want to later add
an option that does something like you describe above.

(As an aside, we already have 'git multi-pack-index verify', which is
probably the closest thing to what you're describing, so perhaps the
concern is lessened by that fact. But even if we already have that
functionality laying around, I think that "--no-write-chain-file" is a
more descriptive name for the command-line flag than "--checksum-only".)

> > @@ -239,6 +254,15 @@ static int cmd_multi_pack_index_compact(int argc, const char **argv,
> >  	if (argc != 2)
> >  		usage_with_options(builtin_multi_pack_index_compact_usage,
> >  				   options);
> > +
> > +	if (opts.flags & MIDX_WRITE_CHECKSUM_ONLY &&
> > +	    !(opts.flags & MIDX_WRITE_INCREMENTAL)) {
> > +		error(_("cannot use %s without %s"),
> > +		      "--checksum-only", "--incremental");
> > +		usage_with_options(builtin_multi_pack_index_compact_usage,
> > +				   options);
> > +	}
> > +
>
> OK. It's sad that we have to duplicate these bits between the write and
> compact operations, but I'm not sure how much sense it would make to
> factor out common options (both how much work it is, and how much
> duplication we could actually drop in practice).

I agree. I looked at unifying these a little further, but decided
against it as it ended up making things more complex than it was worth
IMHO.

> >  	if (ctx.incremental) {
> > -		FILE *chainf = fdopen_lock_file(&lk, "w");
> >  		struct strbuf final_midx_name = STRBUF_INIT;
> >  		struct multi_pack_index *m = ctx.base_midx;
> >
> > -		if (!chainf) {
> > -			error_errno(_("unable to open multi-pack-index chain file"));
> > -			goto cleanup;
> > +		if (!(opts->flags & MIDX_WRITE_CHECKSUM_ONLY)) {
> > +			FILE *chainf = fdopen_lock_file(&lk, "w");
> > +			if (!chainf) {
> > +				error_errno(_("unable to open multi-pack-index chain file"));
> > +				goto cleanup;
> > +			}
> >  		}
>
> OK, and this is the actual write which is skipped. I guess this could
> also be conditional on is_lock_file_locked(&lk), assuming we initialize
> it with LOCK_INIT. In some ways that seems easier to analyze to me,
> since you don't have to realize that "lk" is only active when
> CHECKSUM_ONLY is not set. But I don't think there's another reason we'd
> choose not to take a lock file, so I think it may be six of one and half
> a dozen of the other.

Yeah, I think this is more or less equivalent. In some ways I like the
explicitness of the conditional as it is currently written, but I think
your suggestion reads clearer.

It ends up producing nice code like:

    if (is_lock_file_locked(&lk)) {
        FILE *chainf = fdopen_lock_file(&lk, "w");
        /* ... */
    }

which is pleasing to me in the sense that it reads like "if the lockfile
is held, then do something with it".

I think there is a small risk of coupling the state of the lockfile with
the MIDX_NO_WRITE_CHAIN bit in the flags, but it's easily mitigated by
an ASSERT(opts->flags & MIDX_NO_WRITE_CHAIN) in the else branch of the
conditional above.

There are a few such conditionals, and repeatedly checking that the flag
bits are set is wasteful, so I check it only once in the big "if
(ctx->incremental)" block, and then once again in the top-level
write_midx_internal() function before we do the final commit and
cleanup.

Thanks,
Taylor