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

[Jeff King <peff@peff.net>]

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.

>  				m = m->base_midx;
>  			}
>  
> -			keep_hashes[i] = xstrdup(hash_to_hex_algop(midx_hash,
> +			keep_hashes.v[i] = xstrdup(hash_to_hex_algop(midx_hash,
>  								   r->hash_algo));
> +			keep_hashes.nr++;

OK, this could be a push, too.

>  
>  			i = 0;
>  			for (m = ctx.m;
>  			     m && midx_hashcmp(m, ctx.compact_to, r->hash_algo);
>  			     m = m->base_midx) {
> -				keep_hashes[keep_hashes_nr - i - 1] =
> +				keep_hashes.v[keep_hashes_nr - i - 1] =
>  					xstrdup(midx_get_checksum_hex(m));
> +				keep_hashes.nr++;

But what is this? We're filling in from the back side of the array? I
mean...yeah, that's something that strvec_push() can't do. But I can't
help but feel like it might be simpler and more obvious to adjust the
iteration to build the array in order.

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).

>  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?

Also, I don't think using a strbuf really buys us anything. We are
reusing the strbuf for each format operation, but then we copy into the
string list anyway. So there is one allocation per loop iteration either
way.

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.

-Peff