Re: [PATCH v2 2/5] pack-objects: refactor `read_packs_list_from_stdin()` to use `strmap`

[Derrick Stolee <stolee@gmail.com>]

On 3/25/2026 7:51 PM, Taylor Blau wrote:

> -static void read_packs_list_from_stdin(struct rev_info *revs)
> +struct stdin_pack_info {
> +	struct packed_git *p;
> +	enum {
> +		STDIN_PACK_INCLUDE = (1<<0),
> +		STDIN_PACK_EXCLUDE_CLOSED = (1<<1),
> +	} kind;
> +};

I kind of wish this enum wasn't anonymous. And it matters later.
Let's call this 'enum pack_input_kind' for now.

> +static void stdin_packs_read_input(struct rev_info *revs)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> -	struct string_list include_packs = STRING_LIST_INIT_DUP;
> -	struct string_list exclude_packs = STRING_LIST_INIT_DUP;
> -	struct string_list_item *item = NULL;
> +	struct strmap packs = STRMAP_INIT;
>  	struct packed_git *p;
>  
>  	while (strbuf_getline(&buf, stdin) != EOF) {
> -		if (!buf.len)
> +		struct stdin_pack_info *info;
> +		const char *key = buf.buf;
> +
> +		if (!*key)
>  			continue;
> +		if (*key == '^')
> +			key++;...
>  		if (*buf.buf == '^')
> -			string_list_append(&exclude_packs, buf.buf + 1);
> +			info->kind |= STDIN_PACK_EXCLUDE_CLOSED;
>  		else
> -			string_list_append(&include_packs, buf.buf);
> +			info->kind |= STDIN_PACK_INCLUDE;
>  

It took me a while to figure out what was going on with checking
*key == '^' and later checking *buf.buf == '^'. We should probably
combine them to the same condition:

	const char *key = buf.buf;
	enum pack_input_kind kind = STDIN_PACK_INCLUDE;

	if (*key == '^') {
		key++;
		kind |= STDIN_PACK_EXCLUDE_CLOSED;
	}

	info = strmap_get(&packs, key);
	if (!info) {
		CALLOC_ARRAY(info, 1);
		strmap_put(&packs, key, info);
		info->kind = kind;
	}

	strbuf_reset(&buf);

This feels easier to read, for me.

>  		strbuf_reset(&buf);
>  	}
>  
> -	string_list_sort_u(&include_packs, 0);
> -	string_list_sort_u(&exclude_packs, 0);
> -
>  	repo_for_each_pack(the_repository, p) {
> -		const char *pack_name = pack_basename(p);
> +		struct stdin_pack_info *info;
>  
> -		if ((item = string_list_lookup(&include_packs, pack_name))) {
> +		info = strmap_get(&packs, pack_basename(p));
> +		if (!info)
> +			continue;
> +
> +		if (info->kind & STDIN_PACK_INCLUDE) {
...
>  		}

> +		if (info->kind & STDIN_PACK_EXCLUDE_CLOSED) {

This does help confirm that a pack could be in both categories, so
using flag bits helps.

Thanks,
-Stolee