Re: [PATCH 10/33] refs: extract a function ref_resolves_to_object()

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2013, 18:08 UTC
Message-ID: <7vd2tumjqj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <516D1988.5060501@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 10 quoted lines

> In projects where I can choose the coding standard, I like to use extra
> whitespace to help the eye pick out the range of parentheses, like
>
>         if (!(
>                 (flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
>                 ref_resolves_to_object(entry)
>                 ))
>                 return 0;
>
> but I've never seen this style in the Git project so I haven't used it here.

I _think_ at least to me, it is not the textual grouping style but the "unless X we skip the rest" logic itself that confused me. It took the same number of minutes to grok the above as the original.

We skip the rest of this function unless the condition inside !() holds, and the condition is.... we are told to include broken ones, in which case we know we will do the remainder without checking anything else, or we do the checking and find that it is not broken.

I suspect the root cause of the confusion to force the double negation is INCLUDE_BROKEN; we have to express "when we are told to ignore broken one and this thing is broken, ignore it" without negation, i.e.

	if ( !(flags & INCLUDE_BROKEN) && is_broken(thing) )
		return;

which would have been much more pleasant to read if it were

	if ( (flags & IGNORE_BROKEN) && is_broken(thing) )
		return;

Unfortunately, we cannot change it to IGNORE_BROKEN and flip the meaning of the bit, because almost all callers do want to ignore broken ones.

Either way is fine by me, even though I find that !(A || B) needs a bit more mental exercise to grok than (!A && !B). Perhaps it is just me who is not so strong at math ;-)