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

[Jeff King <peff@peff.net>]

On Tue, Mar 31, 2026 at 06:50:49PM -0400, Taylor Blau wrote:

> If we move to strset, we obviously still need to ensure that
> constructing .keep doesn't leak strings. The strset API has a
> strdup_strings option (via the strmap underneath), but does not
> explicitly free keys on clear.
> 
> As far as I can tell, the strset API is not designed to take ownership
> of the strings themselves, so to avoid leaks we would have to do
> something like:

It's based on strmap, which handles ownership. If you use STRMAP_INIT or
strmap_init(), then strdup_strings is set to 1.

Or do you mean that you can't transfer ownership into the strset from an
existing string? I think that is true, and if you are generating the
strings with xstrfmt() then yeah, you have two copies in the loop and
have to free one.

That is probably actually fine for the sizes we are talking about, but
if you want to care you can either do the reusable-strbuf thing, or
we can add a strset_add_nodup().

> @@ -815,6 +817,9 @@ void clear_incremental_midx_files_ext(struct odb_source *source, const char *ext
>  	for_each_file_in_pack_subdir(source->path, "multi-pack-index.d",
>  				     clear_midx_file_ext, &data);
> 
> +	hashmap_for_each_entry((struct hashmap*)&data.keep.map, &iter, e, ent) {
> +		free((void *)e->key);
> +	}
>  	strset_clear(&data.keep);
>  }
> --- >8 ---
> 
> which is... pretty gross. I think we may be better off sticking with a
> string_list for 'keep' here internally.

Yeah, this is horrible, and this kind of "we own it, but only kind of
and strdup_strings is not set" game has been the cause of many bugs on
the string-list side.

Having a "nodup" variant is less awful, but still error prone and leads
to complications. For example, if strset_add_nodup() sees a duplicate,
what does it do with the input?

So I would probably just either do:

  for (...) {
	item = xstrfmt(...);
	strset_add(&set, item);
	free(item);
  }

or if you care about the extra allocation, then:

  struct strbuf item = STRBUF_INIT;
  for (...) {
	strbuf_reset(&item);
	strbuf_addf(&item, ...);
	strset_add(&set, item.buf);
  }
  strbuf_release(&item);

-Peff