Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Stefan Beller <sbeller@google.com>,
	git@vger.kernel.org, git@jeffhostetler.com
Subject: Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO
Date: Wed, 22 Nov 2017 18:58:51 -0500	[thread overview]
Message-ID: <20171122235851.GE8577@sigill> (raw)
In-Reply-To: <20171122234532.GI11671@aiede.mtv.corp.google.com>

On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
> > I'm not sure what it's buying us over assert().
> 
> It lets you build with NDEBUG.

But why do you want to build with NDEBUG if nothing uses assert()? ;)

I'm being a little glib, but I think there really is a chicken-and-egg
question here. Why are people building with NDEBUG if they don't want to
disable asserts? Do they do it out of habit? A misguided sense of
performance optimization? Does it do something else I'm not aware of?

> It also goes through our own die()
> handler, which means that e.g. the error message gets propagated over
> remote transports.

BUG() doesn't go through our die handler. It hits vreportf(), which does
do some minor munging, but it always goes to stderr. Error message
propagation over the remote protocol happens either:

  1. From a parent process muxing our stderr onto the sideband.

  2. Via special wrappers like receive-pack's rp_error().

The only program I know that sets a custom die handler to change the
reporting is git-http-backend (and there it only applies to die("BUG"),
not BUG(), so with the latter you'd get a hard hangup instead of an
attempt at an http 500).

> Please please please, don't rely on side-effects from assert().  They
> will cause me to run into pain over time.  This issue alone might be
> worth banning use of assert() in the codebase, if we can't trust
> reviewers to catch problematic examples (fortunately reviewers have
> been pretty good about that so far, but banning it would free up their
> attention to focus on other aspects of a patch).

Yes, obviously side effects in an assert() are horrible. But I haven't
noticed that being a problem in our code base.

For the record, I'm totally fine with banning assert() in favor of a
custom equivalent. I just don't think we've seen any real problems with
assert in our codebase so far.

-Peff