On Wed, May 02, 2018 at 11:38:13AM +0200, Johannes Schindelin wrote:
> The BUG() macro was introduced in this patch series:
> https://public-inbox.org/git/20170513032414.mfrwabt4hovuj...@sigill.intra.peff.net
>
> The second patch in that series converted one caller from die("BUG: ")
> to use the BUG() macro.
>
> It seems that there was no concrete plan to address the same issue in
> the rest of the code base.
I had a plan; it was that people would convert these as they touched the
relevant areas. :)
I'm happy to see a mass-conversion, though.
> For that reason, the commit message contains the precise Unix shell
> invocation (GNU sed semantics, not BSD sed ones, because I know that the
> Git maintainer as well as the author of the patch introducing BUG() both
> use Linux and not macOS or any other platform that would offer a BSD
> sed). It should be straight-forward to handle merge
> conflicts/non-applying patches by simply re-running that command.
I suspect this could have been done with coccinelle, but it's definitely
not worth going back to re-do it now.
> Changes since v2:
>
> - Avoided the entire discussion about the previous 2/6 (now dropped)
> that prepared t1406 to handle SIGABRT by side-stepping the issue: the
> ref-store test helper will no longer call abort() in BUG() calls but
> exit with exit code 99 instead.
I actually think this should be a runtime flag in the environment, like
GIT_BUG_EXIT_CODE (and if not set, continue to abort). That can help if
you come across a BUG() in real Git code, but for some reason your
environment makes aborting a pain. For example, maybe you're debugging a
racy BUG() and don't want to generate a bunch of coredumps.
Or here's an interesting related case I came across a few months ago.
t6210 has a test that's known to segfault due to stack exhaustion. It's
marked test_expect_failure, so all is good, right? Normally, yes, but
when I run the test suite inside our local Jenkins setup, it detects a
segfault in _any_ child process of the test runner and aborts the whole
thing. This is great as a belt-and-suspenders if we miss an unexpected
segfault, but is obviously annoying in this case.
Triggering BUG()s in the test suite, even inside an expect_failure,
would introduce the same headache. It would be nice if I could just do:
GIT_BUG_EXIT_CODE=134 make test
to avoid it. Possibly expect_failure should even set that
automatically.
I also suspect that nobody really needs to set a specific exit code.
Using 134 is enough to avoid all of the unpleasantness of SIGABRT, but
still enough to trigger test_must_fail to distinguish it from a non-BUG
death. The callers that intentionally trigger bugs probably ought to be
using test_expect_code to make sure they are hitting a BUG() and not
some other death, anyway.
So we could probably simplify it to something like this:
diff --git a/usage.c b/usage.c
index cdd534c9df..50651bed40 100644
--- a/usage.c
+++ b/usage.c
@@ -221,7 +221,11 @@ static NORETURN void BUG_vfl(const char *file, int line,
const char *fmt, va_lis
snprintf(prefix, sizeof(prefix), "BUG: ");
vreportf(prefix, fmt, params);
- abort();
+
+ if (git_env_bool("GIT_BUG_ABORT"), 1)
+ abort();
+ else
+ exit(134);
}
#ifdef HAVE_VARIADIC_MACROS
-Peff
