On Wed, Oct 29, 2025 at 07:13:33PM -0400, Taylor Blau wrote:

On Tue, Oct 28, 2025 at 12:08:35PM +0100, Patrick Steinhardt wrote:

quoted

@@ -4388,27 +4388,27 @@ static void add_unreachable_loose_objects(struct rev_info *revs)
-	while (p) {
-		if ((!p->pack_local || p->pack_keep ||
-				p->pack_keep_in_core) &&
-			find_pack_entry_one(oid, p)) {
+	repo_for_each_pack(the_repository, p) {
+		if ((!p->pack_local || p->pack_keep || p->pack_keep_in_core) &&
+		    find_pack_entry_one(oid, p)) {
 			last_found = p;
 			return 1;
 		}
-		if (p == last_found)
-			p = packfile_store_get_packs(packs);
-		else
-			p = p->next;
-		if (p == last_found)
-			p = p->next;
+
+		/*
+		 * We have already checked `last_found`, so there is no need to
+		 * re-check here.
+		 */
+		if (p == last_found && last_found != (void *)1)
+			continue;

Can 'p' ever be (void *)1 here? I would imagine not since this is coming
from repo_for_each_pack(), so I think it would suffice to limit this
conditional to just "if (p == last_found)".

Oh, you're right of course, will adapt.

Furthermore, do we even need the `(void *)1` thingy? I think it should
be perfectly fine to instead use a `NULL` pointer here. A valid pack
obviously cannot be a `NULL` pointer, so the sentinel feels kind of
pointless to me.

Otherwise looks good. I think you could make use of the kept_cache here
at least for the local-but-kept packs, but what you wrote is definitely
an improvement in readability.

Makes sense. I'll leave this out of this series though as a #leftoverbit
for a future patch series :)

Thanks!

Patrick