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 08:29:57PM -0400, Derrick Stolee wrote:
> 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;

Right, though the problem is not that we're setting the wrong flag bits
(though I agree in the previous version of this suggestion that we
should have been OR-ing them in), but that we're not setting any flag
bits if the same pack is specified multiple times.

Applying the following:

--- 8< ---
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 52bad8cea90..37c69f307d2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3833,13 +3833,15 @@ static void show_commit_pack_hint(struct commit *commit, void *data)

 }

+enum stdin_pack_info_kind {
+	STDIN_PACK_INCLUDE = (1<<0),
+	STDIN_PACK_EXCLUDE_CLOSED = (1<<1),
+	STDIN_PACK_EXCLUDE_OPEN = (1<<2),
+};
+
 struct stdin_pack_info {
 	struct packed_git *p;
-	enum {
-		STDIN_PACK_INCLUDE = (1<<0),
-		STDIN_PACK_EXCLUDE_CLOSED = (1<<1),
-		STDIN_PACK_EXCLUDE_OPEN = (1<<2),
-	} kind;
+	enum stdin_pack_info_kind kind;
 };

 static int pack_mtime_cmp(const void *_a, const void *_b)
@@ -3927,26 +3929,26 @@ static void stdin_packs_read_input(struct rev_info *revs,

 	while (strbuf_getline(&buf, stdin) != EOF) {
 		struct stdin_pack_info *info;
+		enum stdin_pack_info_kind kind = STDIN_PACK_INCLUDE;
 		const char *key = buf.buf;

 		if (!*key)
 			continue;
-		if (*key == '^' ||
-		    (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW))
+		else if (*key == '^')
+			kind = STDIN_PACK_EXCLUDE_CLOSED;
+		else if (*key == '!' && mode == STDIN_PACKS_MODE_FOLLOW)
+			kind = STDIN_PACK_EXCLUDE_OPEN;
+
+		if (kind != STDIN_PACK_INCLUDE)
 			key++;

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

-		if (*buf.buf == '^')
-			info->kind |= STDIN_PACK_EXCLUDE_CLOSED;
-		else if (*buf.buf == '!' && mode == STDIN_PACKS_MODE_FOLLOW)
-			info->kind |= STDIN_PACK_EXCLUDE_OPEN;
-		else
-			info->kind |= STDIN_PACK_INCLUDE;
+			info->kind |= kind;
+		}

 		strbuf_reset(&buf);
 	}
--- >8 ---

fails t5331.8, which verifies that pack-objects correctly handles the
same pack being specified as both included and excluded.

But if you do the following on top of the above:

--- 8< ---
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 37c69f307d2..b6e4f950a67 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3946,10 +3946,10 @@ static void stdin_packs_read_input(struct rev_info *revs,
 		if (!info) {
 			CALLOC_ARRAY(info, 1);
 			strmap_put(&packs, key, info);
-
-			info->kind |= kind;
 		}

+		info->kind |= kind;
+
 		strbuf_reset(&buf);
 	}
--- >8 ---

Then that works as expected. I agree that the end-result is a little
easier to read, so I'll squash this into the subsequent round.

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

I agree! Thanks again for the suggestion.

Thanks,
Taylor