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

Michael Haggerty [off-list ref] writes:

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 ;-)