Re: [RFC PATCH 01/14] midx: use `string_list` for retained MIDX files

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

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

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