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.mfrwabt4hovujde2@xxxxxxxxxxxxxxxxxxxxx > > 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
