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

From: Jeff King <peff@peff.net>
Date: Jan 7, 2020, 15:36 UTC
Message-ID: <20200107153651.GB20591@coredump.intra.peff.net>
In-Reply-To: <5dd305d2f0de43a70b46336c8f1a62437e0511e1.1578408947.git.gitgitgadget@gmail.com>

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

Show 7 quoted lines

> 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/