Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self

[Junio C Hamano <gitster@pobox.com>]

Elijah Newren <newren@gmail.com> writes:

> "if (condition) BUG()" is invalid; it needs more arguments.  "if
> (condition) BUG(something)" requires a separate "something", which
> requires awkward additional wording and/or is needlessly duplicative.

Ah, obviously we differ on that point.

I consider it an advantage that <something> can be more descriptive
in a developer friendly way than <condition> expression alone.  If
you wrote

	if (istate->cache_nr < i)
		BUG("ran past the end of the istate->cache[] array?");

you can spot a bug of the assertion, because the human-readable part
tells us the intention that the condition is supposed to be the
usual array bounds check, and should be checking with "<=".  In
contrast

	BUG_ON(istate->cache_nr < i);

does not give us the extra information we can use to double check
what the developer who wrote it, who may not be with us anymore,
wanted to really check.  Is it a misspelt "<=", or does this code
path use 'i' not just as a variable to iterate over the array but
also allows it to go one past its end, so "<" is the condition it
really wants to validate?

> If you don't want to see assert in the codebase because of NDEBUG,
> then obviously we'd leave NDEBUG out of BUG_ON().

Absolutely, because the largest problem with assert() is that the
condition can be compiled away while the compiler does not help
ensure that the condition part is free of side effects.  If we drop
NDEBUG, that problem goes away.

> Such a BUG_ON() would be an improvement because it maintains the
> ergonomics of assert() while avoiding the potential mistake of
> accidentally including something with side-effects.  

Yes, we are half on the same page (as I disagree that assert() that
does not give a room to explain why we are checking the condition is
ergonomic at all) and agree that discarding NDEBUG gives us safer
variant than assert().

> "If (condition)
> BUG(something)" doesn't preserve the ergonomics and is thus worse,

Again, this is philosophical difference between us.  It gives us the
same safety as NDEBUG-less BUG_ON() but allows us to be more
descriptive to help developers.  While I understand sometimes the
condition may be self evident (especially while developing new code,
where the developer thought the problem long and deep) and people
may find it tedious to have to explain the reasoning behind the
check, I find it highly problematic not to give room to explain to
those who want to.  And /* comments */ is not the same thing, as I
think it is important to explain your BUG() well, just like it is
good practice to explain your commit well in the log message.

One thing I view as a downside of "if (condition) BUG(message)" is
that it does not make it clear syntactically that (condition) is
about assertion, and you can write

	if (condition) {
		do something;
		BUG(message);
	}

which probably is not what you want.  I am OK with

	BUG_ON(<condition>, <message>);

which would make such a construct impossible.

Thanks.