Re: [PATCH 04/16] midx: use `strvec` for `keep_hashes`

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 30, 2026 at 07:01:30PM -0400, Jeff King wrote:
> On Sun, Mar 29, 2026 at 05:41:00PM -0400, Taylor Blau wrote:
>
> > -	CALLOC_ARRAY(keep_hashes, keep_hashes_nr);
> > +	strvec_init_alloc(&keep_hashes, keep_hashes_nr);
> >
> >  	if (ctx.incremental) {
> >  		FILE *chainf = fdopen_lock_file(&lk, "w");
> > @@ -1760,39 +1760,45 @@ static int write_midx_internal(struct write_midx_opts *opts)
> >  			for (i = 0; i < num_layers_before_from; i++) {
> >  				uint32_t j = num_layers_before_from - i - 1;
> >
> > -				keep_hashes[j] = xstrdup(midx_get_checksum_hex(m));
> > +				keep_hashes.v[j] = xstrdup(midx_get_checksum_hex(m));
> > +				keep_hashes.nr++;
>
> Gross, we are just manipulating the innards of the strvec ourselves?
>
> Is it really worth doing this (and adding init_alloc()) versus just:
>
>   strvec_init(&keep_hashes);
>   for (...)
> 	strvec_push(midx_get_checksum_hex(m));
>
> ? That's amortized linear-time, and it's not like the number of midx
> layers is going to be large anyway.

Yeah, this is all pretty disgusting. It's in service of what you noted
below where we need to fill in the array out-of-order, but that's really
bending strvec around MIDX-specific awkwardness, which I dislike.

> I dunno. Maybe that is hard to do. But if so, I question whether moving
> to a strvec is worth it here, since we are not treating it as an opaque
> type anymore. And it is not buying us much to use it (we get to pass one
> parameter versus two, though that is easily solved with a struct, and we
> get to use _clear() instead of our own free loop).

I think it is worth it to move to a strvec here to avoid having to
manage our own memory and pass the length around separately, but I
dislike the way that I did it in this patch.

I tried adjusting this patch to juggle the MIDX layers in a way that
would allow us to just push to the strvec. It ended up being less
awkward/difficult than I thought, so I think we should go with that and
drop the previous patch.

> >  void clear_incremental_midx_files_ext(struct odb_source *source, const char *ext,
> > -				      char **keep_hashes,
> > -				      uint32_t hashes_nr)
> > +				      const struct strvec *keep_hashes)
> >  {
> >  	struct clear_midx_data data = {
> > -		.keep = STRING_LIST_INIT_NODUP,
> > +		.keep = STRING_LIST_INIT_DUP,
> >  		.ext = ext,
> >  	};
> > -	uint32_t i;
> >
> > -	for (i = 0; i < hashes_nr; i++)
> > -		string_list_append(&data.keep,
> > -				   xstrfmt("multi-pack-index-%s.%s",
> > -					   keep_hashes[i], ext));
> > -	string_list_sort(&data.keep);
> > +	if (keep_hashes) {
> > +		struct strbuf buf = STRBUF_INIT;
> > +		for (size_t i = 0; i < keep_hashes->nr; i++) {
> > +			strbuf_reset(&buf);
> > +
> > +			strbuf_addf(&buf, "multi-pack-index-%s.%s",
> > +				    keep_hashes->v[i], ext);
> > +			string_list_append(&data.keep, buf.buf);
> > +		}
> > +
> > +		string_list_sort(&data.keep);
> > +		strbuf_release(&buf);
> > +	}
>
> This hunk was unexpected. We move from xstrfmt() to a strbuf, but does
> that have anything to do with the rest of the patch?

Yeah, this looks like it was left over from when I was writing this
patch and trying out different approaches, and I didn't notice that I
left it in when sending this patch out.

> Also also, the original was leaking the strings, right? The string_list
> was initialized as NODUP, but we assigned allocate xstrfmt() results to
> it. But because of the nodup, string_list_clear() won't free them.
> It should have been:
>
>   .keep = STRING_LIST_INIT_DUP,
>   [...]
>   string_list_append_nodup(&data.keep, xstfmrt(...));
>   [...]
>   string_list_clear(&data.keep);
>
> in patch 2.

Good catch, that's right, but partially obviated by the fact that we're moving to
strset here.

Thanks,
Taylor