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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Mar 26, 2026 at 04:40:00PM -0400, Derrick Stolee wrote:
> 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.

Hmm. I don't feel strongly about this, but I'm not sure I follow the
reasoning here. The enum is truly only meant to be used within the
context of a stdin_pack_info struct, so it felt natural to keep it
anonymous above.

I'm happy to change this if you feel strongly about it, but TBH I am not
sure I see the benefit of doing so.

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

I agree that the above is a little easier to read, but I'm not sure it
handles the case of specifying the same pack multiple times. I had
originally written it in a similar way as what you suggested above, but
it breaks if I write something like:

    cat <<EOF | git pack-objects --stdin
    pack-XYZ.pack
    ^pack-XYZ.pack
    EOF

It should produce a pack with no objects, but I think the code above
would effectively ignore the second line because we already have a
strmap entry for pack-XYZ.pack so we never set the additional flag bits.

Thanks,
Taylor