On Mon, Jul 25, 2016 at 02:52:24PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > 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.
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.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html