Re: [PATCH 1/2] pack-objects: break out of want_object loop early

Jeff King [off-list ref] writes:

quoted

I do not mind too much about having to check two bools twice.  But
given that the reason why I was confused was because I didn't see
why we need to pass the two "return 0" conditions at least once
before we decide that we do not need the "return 0" thing at all,
and started constructing a case where this might break by writing
"Suppose you have two packs, one remote and one local in packed_git
list in this order, and ..." before I realized that the new "early
break" can be hoisted up like the above, I definitely feel that "we
found one, and we aren't conditionally pretending that this thing
does not need to be packed at all, so return early and say we want
to pack it" is easier to understand before the two existing "if"
statements.

Ah, right. Now you had me second-guessing for a moment that there might
be a bad case in hoisting it up where we would want to return 0 but
would break out early to the "return 1".

But it cannot be the case, because the break is mutually exclusive with
the two conditions.

Here is what I amended looks like (with s/local/non-local/ in the
log message).

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a2f8cfd..a46bf5b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c

@@ -977,6 +977,21 @@ static int want_object_in_pack(const unsigned char *sha1,
 				return 1;
 			if (incremental)
 				return 0;
+
+			/*
+			 * When asked to do --local (do not include an
+			 * object that appears in a pack we borrow
+			 * from elsewhere) or --honor-pack-keep (do not
+			 * include an object that appears in a pack marked
+			 * with .keep), we need to make sure no copy of this
+			 * object come from in _any_ pack that causes us to
+			 * omit it, and need to complete this loop.  When
+			 * neither option is in effect, we know the object
+			 * we just found is going to be packed, so break
+			 * out of the loop to return 1 now.
+			 */
+			if (!ignore_packed_keep && !local)
+				break;
 			if (local && !p->pack_local)
 				return 0;
 			if (ignore_packed_keep && p->pack_local && p->pack_keep)