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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 25, 2016, 21:52 UTC
Message-ID: <xmqqbn1l4c87.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160725214113.GA13589@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

>   if (!*found_pack) {
>     ... first find! fill in found pack, etc ...
>   }
>   if (exclude)
> 	return 1;
>   if (incremental)
> 	return 0;
>   if (!ignore_packed_keep && !local)
> 	break; /* effectively return 1, but I think the break is more clear */
>   if (local && !p->pack_local)
> 	return 0;
>   if (ignore_packed_keep && p->pack_local && p->pack_keep)
> 	return 0;
>
> which just bumps it up. I don't think there is a way to make it more
> elegant, e.g., by only checking ignore_packed_keep once, because we have
> to distinguish between each condition being set independently, or the
> case where neither is set.
>
> So I stuck the new check at the end, because to me logically it was "can
> we break out of the loop instead of looking at p->next". But I agree it
> would be equivalent to place it before the related checks, and I don't
> mind doing that if you think it's more readable.

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.

Thanks.