Re: [PATCH 2/3] graph: replace assert() with graph_assert() macro

[Jeff King <peff@peff.net>]

On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> The assert() macro is sometimes compiled out. Instead, switch these into
> BUG() statements using our own custom macro.
> 
> Reported-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

I can buy the argument that compiling with and without NDEBUG can lead
to confusion. But if that is the case, wouldn't it be so for all of the
assert() calls, not just ones in the graph code?

Previous discussions[1] seemed to conclude that having a kernel-style
BUG_ON() is probably the right way forward. I.e., replace this:

> +#define graph_assert(exp) if (!(exp)) { BUG("assert failed: "#exp""); }

with something similar in git-compat-util.h. Even if we don't convert
everybody to it immediately, it would be available for use.

At any rate, I think this patch (and the third one) can be post-v2.25.
But we'd want the first one before the release.

-Peff

[1] https://lore.kernel.org/git/20171122223827.26773-1-sbeller@google.com/