Re: [RFC PATCH 01/14] midx: use `string_list` for retained MIDX files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 26, 2026, 20:29 UTC
- Message-ID
- <xmqqldgf1c65.fsf@gitster.g>
- In-Reply-To
- <d64a799afd620363c1940d7c2e634e78ea553cb6.1771978829.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 9 quoted lines
> Both `clear_midx_files_ext()` and `clear_incremental_midx_files_ext()` > build a list of filenames to keep while pruning stale MIDX files. Today > they hand-roll an array instead of using a `string_list`, thus requiring > us to pass an additional length parameter, and makes lookups linear. > > Replace the bare array with a `string_list` which can be passed around > as a single parameter. Though it improves lookup performance, the > difference is likely immeasurable given how small the keep_hashes array > typically is.
And if it the lookup performance turns out to be an issue, we can switch to strmap or something more appropriate.
Show 106 quoted lines
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> midx.c | 56 ++++++++++++++++++++++----------------------------------
> 1 file changed, 22 insertions(+), 34 deletions(-)
>
> diff --git a/midx.c b/midx.c
> index c1b9658240d..c5e3553e2bb 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -755,8 +755,7 @@ int midx_checksum_valid(struct multi_pack_index *m)
> }
>
> struct clear_midx_data {
> - char **keep;
> - uint32_t keep_nr;
> + struct string_list keep;
> const char *ext;
> };
>
> @@ -764,15 +763,12 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len UNUS
> const char *file_name, void *_data)
> {
> struct clear_midx_data *data = _data;
> - uint32_t i;
>
> if (!(starts_with(file_name, "multi-pack-index-") &&
> ends_with(file_name, data->ext)))
> return;
> - for (i = 0; i < data->keep_nr; i++) {
> - if (!strcmp(data->keep[i], file_name))
> - return;
> - }
> + if (string_list_has_string(&data->keep, file_name))
> + return;
> if (unlink(full_path))
> die_errno(_("failed to remove %s"), full_path);
> }
> @@ -780,48 +776,40 @@ static void clear_midx_file_ext(const char *full_path, size_t full_path_len UNUS
> void clear_midx_files_ext(struct odb_source *source, const char *ext,
> const char *keep_hash)
> {
> - struct clear_midx_data data;
> - memset(&data, 0, sizeof(struct clear_midx_data));
> -
> - if (keep_hash) {
> - ALLOC_ARRAY(data.keep, 1);
> -
> - data.keep[0] = xstrfmt("multi-pack-index-%s.%s", keep_hash, ext);
> - data.keep_nr = 1;
> - }
> - data.ext = ext;
> -
> - for_each_file_in_pack_dir(source->path,
> - clear_midx_file_ext,
> - &data);
> + struct clear_midx_data data = {
> + .keep = STRING_LIST_INIT_NODUP,
> + .ext = ext,
> + };
>
> if (keep_hash)
> - free(data.keep[0]);
> - free(data.keep);
> + string_list_insert(&data.keep, xstrfmt("multi-pack-index-%s.%s",
> + keep_hash, ext));
> +
> + for_each_file_in_pack_dir(source->path, clear_midx_file_ext, &data);
> +
> + string_list_clear(&data.keep, 0);
> }
>
> 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;
> + struct clear_midx_data data = {
> + .keep = STRING_LIST_INIT_NODUP,
> + .ext = ext,
> + };
> uint32_t i;
>
> - memset(&data, 0, sizeof(struct clear_midx_data));
> -
> - ALLOC_ARRAY(data.keep, hashes_nr);
> for (i = 0; i < hashes_nr; i++)
> - data.keep[i] = xstrfmt("multi-pack-index-%s.%s", keep_hashes[i],
> - ext);
> - data.keep_nr = hashes_nr;
> - data.ext = ext;
> + string_list_append(&data.keep,
> + xstrfmt("multi-pack-index-%s.%s",
> + keep_hashes[i], ext));
> + string_list_sort(&data.keep);
>
> for_each_file_in_pack_subdir(source->path, "multi-pack-index.d",
> clear_midx_file_ext, &data);
>
> - for (i = 0; i < hashes_nr; i++)
> - free(data.keep[i]);
> - free(data.keep);
> + string_list_clear(&data.keep, 0);
> }
>
> void clear_midx_file(struct repository *r)