Re: [PATCH 4/5] pack-objects: support excluded-open packs with --stdin-packs

[Taylor Blau <me@ttaylorr.com>]

On Sat, Mar 21, 2026 at 12:57:11PM -0400, Jeff King wrote:
> On Thu, Mar 19, 2026 at 06:24:25PM -0400, Taylor Blau wrote:
>
> > @@ -3750,8 +3756,6 @@ static int add_object_entry_from_pack(const struct object_id *oid,
> >  		return 0;
> >
> >  	ofs = nth_packed_object_offset(p, pos);
> > -	if (!want_object_in_pack(oid, 0, &p, &ofs))
> > -		return 0;
> >
> >  	if (p) {
> >  		struct object_info oi = OBJECT_INFO_INIT;
>
> I haven't read all of the patches carefully yet, but Coverity observed
> that without this call to want_object_in_pack(), we are left with "p"
> that came from the caller. If that "p" is ever NULL, then the
> nth_packed_object_offset() call above will segfault. But if it is not,
> then the "if (p)" below is pointless.
>
> AFAICT it will never be NULL, and the conditional is now pointless. But
> it wasn't before your patch, since want_object_in_pack() may set the
> found_pack to NULL if the object is not wanted.

I think that's right. Looking through the code again, the "if (p)"
conditional is indeed no longer useful, since the sole caller of this
function (the callback to `for_each_object_in_pack()`) will always pass
a non-NULL `p` pointer.

It's tempting to add something like:

    if (!p)
        BUG("add_object_entry_from_pack: expected non-NULL pack");

But I wonder if we should instead store the result of calling
`want_object_in_pack()` into a separate variable, only creating an
object entry if "want == 1". That would have the effect of *not* marking
objects as traversal tips if want_object_in_pack() makes `p` NULL.

I think that's OK, since what we really care about is having objects in
both included and excluded-open packs being processed as traversal tips.
Because we're enumerating only objects that are in included and
excluded-open packs:

 * Objects in excluded-open packs will cause `want_found_object()` to
   return 0, propagating that through `want_object_in_pack()` without
   setting "p" to NULL. We'll process these objects as traversal tips,
   but not create an object entry for them, which is what we want.

 * Objects in included packs will return -1 from `want_found_object()`,
   causing us to NULL out "p" and search through other packs. If we find
   another pack containing that object, then we'll set "p" to that pack
   and return 1, otherwise we'll return 0 if no such pack is found.

I think that's the right behavior, so doing something like this instead:

--- 8< ---
@@ -3742,6 +3749,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 {
 	off_t ofs;
 	enum object_type type = OBJ_NONE;
+	int want;

 	display_progress(progress_state, ++nr_seen);

@@ -3749,8 +3757,7 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		return 0;

 	ofs = nth_packed_object_offset(p, pos);
-	if (!want_object_in_pack(oid, 0, &p, &ofs))
-		return 0;
+	want = want_object_in_pack(oid, 0, &p, &ofs);

 	if (p) {
 		struct object_info oi = OBJECT_INFO_INIT;
@@ -3762,8 +3769,16 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		} else if (type == OBJ_COMMIT) {
 			struct rev_info *revs = _data;
 			/*
-			 * commits in included packs are used as starting points for the
-			 * subsequent revision walk
+			 * commits in included packs are used as starting points
+			 * for the subsequent revision walk
+			 *
+			 * Note that we do want to walk through commits that are
+			 * present in excluded-open ('!') packs to pick up any
+			 * objects reachable from them not present in the
+			 * excluded-closed ('^') packs.
+			 *
+			 * However, we'll only add those objects to the packing
+			 * list after checking `want_object_in_pack()` below.
 			 */
 			add_pending_oid(revs, NULL, oid, 0);
 		}
@@ -3771,7 +3786,8 @@ static int add_object_entry_from_pack(const struct object_id *oid,
 		stdin_packs_found_nr++;
 	}

-	create_object_entry(oid, type, 0, 0, 0, p, ofs);
+	if (want)
+		create_object_entry(oid, type, 0, 0, 0, p, ofs);

 	return 0;
 }
--- >8 ---

may be more readable. I can't shake the feeling that there is some
important case that this is missing, though, what do you think?

Thanks,
Taylor