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

[Derrick Stolee <stolee@gmail.com>]

On 3/26/26 6:32 PM, Taylor Blau wrote:
> On Thu, Mar 26, 2026 at 03:11:06PM -0700, Junio C Hamano wrote:
>> Taylor Blau <me@ttaylorr.com> writes:

>> If these STDIN_PACK_* constants would ever appear _only_ within the
>> context of talking about the .kind member of the stdin_pack_info
>> struct and cannot possibly appear anywhere else, then there is no
>> point naming the enum.
> 
> Yup, I agree. I'm inclined to leave the enum anonymous for now, since
> the only place we would need a name for it is the suggestion Stolee made
> above, which I think does not correctly handle an edge case where packs
> are specified multiple times.

I see that I messed up where a '|=' should be and where a '=' should be.

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

	if (*key == '^') {
		key++;

		/* THIS ONE SHOULD BE EQUAL */
		kind = STDIN_PACK_EXCLUDE_CLOSED;
	}

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

		/* THIS ONE SHOULD BE ADDING THE FLAG */
		info->kind |= kind;
	}

	strbuf_reset(&buf);

Sorry that I was less careful with the code and hadn't tested it
locally. (I still haven't, but I still think it's worth a little
more attempt to benefit from this structure, especially with your
addition of handling '!' later.

> As it is currently, the enum is only used in the context of the .kind
> member of the stdin_pack_info struct, and we don't ever declare a
> int/unsigned variable to hold the kind outside of that context (which
> would be gross ;-)).

Once you start creating a type, that tends to create the desire to use
that type in a new way. My _preference_ is to name the type because it
unlocks new ways of working with the data without needing to rewrite
the definition.

If the small tweak to my version works, I do think that the readability
of the new organization would be worth it.

Thanks,
-Stolee