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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Mar 31, 2026 at 07:17:13PM -0400, Jeff King wrote:
> > @@ -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.

Indeed ;-).

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

I think the latter of these two makes sense to me.

I'm not necessarily of the opinion that the strset API should *never*
have DUP/NODUP semantics like the string_list API does. But I do think
that we should punt on taking something like this up as long as
possible.

Thanks,
Taylor