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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Mar 31, 2026 at 06:26:04PM -0400, Taylor Blau wrote:
> > 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.

...actually, I think this is a little more subtle than I was hoping for.

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:

--- 8< ---
diff --git a/midx.c b/midx.c
index 4976ed4fac3..1c17a8657ff 100644
--- a/midx.c
+++ b/midx.c
@@ -802,12 +802,14 @@ void clear_incremental_midx_files_ext(struct odb_source *source, const char *ext
 				      char **keep_hashes,
 				      uint32_t hashes_nr)
 {
-	struct clear_midx_data data = {
-		.keep = STRSET_INIT,
-		.ext = ext,
-	};
+	struct clear_midx_data data = { .ext = ext };
+	struct hashmap_iter iter;
+	struct strmap_entry *e;
+
 	uint32_t i;

+	strset_init_with_options(&data.keep, NULL, 0);
+
 	for (i = 0; i < hashes_nr; i++)
 		strset_add(&data.keep, xstrfmt("multi-pack-index-%s.%s",
 					       keep_hashes[i], ext));
@@ -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.

Thanks,
Taylor