From: Jeff King <peff@peff.net>
To: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
"Johannes Sixt" <j6t@kdbg.org>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 0/4] Finish the conversion from die("BUG: ...") to BUG()
Date: Mon, 7 May 2018 05:01:10 -0400 [thread overview]
Message-ID: <20180507090109.GA367@sigill.intra.peff.net> (raw)
In-Reply-To: <cover.1525253892.git.johannes.schindelin@gmx.de>
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@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
next prev parent reply other threads:[~2018-05-07 9:01 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-12 14:19 [Git 2.13.0] BUG: setup_git_env called without repository Josh Hagins
2017-05-12 14:48 ` Johannes Schindelin
2017-05-15 0:22 ` Josh Hagins
2017-05-12 20:34 ` [PATCH] config: complain about --local outside of a git repo Jeff King
2017-05-12 22:31 ` Ævar Arnfjörð Bjarmason
2017-05-13 2:03 ` Jeff King
2017-05-13 3:24 ` [PATCH 0/3] BUG() and "config --local" outside of repo Jeff King
2017-05-13 3:28 ` [PATCH 1/3] usage.c: add BUG() function Jeff King
2017-05-13 3:55 ` Jeff King
2017-05-15 2:28 ` Junio C Hamano
2017-05-13 3:29 ` [PATCH 2/3] setup_git_env: convert die("BUG") to BUG() Jeff King
2017-05-13 3:29 ` [PATCH 3/3] config: complain about --local outside of a git repo Jeff King
2018-05-02 9:38 ` [PATCH v2 0/4] Finish the conversion from die("BUG: ...") to BUG() Johannes Schindelin
2018-05-02 9:38 ` [PATCH v2 1/4] test-tool: help verifying BUG() code paths Johannes Schindelin
2018-05-02 15:18 ` Duy Nguyen
2018-05-05 19:30 ` Johannes Schindelin
2018-05-02 9:38 ` [PATCH v2 2/4] run-command: use BUG() to report bugs, not die() Johannes Schindelin
2018-05-07 9:08 ` Jeff King
2018-05-02 9:38 ` [PATCH v2 3/4] Replace all die("BUG: ...") calls by BUG() ones Johannes Schindelin
2018-05-02 9:38 ` [PATCH v2 4/4] Convert remaining die*(BUG) messages Johannes Schindelin
2018-05-07 9:01 ` Jeff King [this message]
2017-05-13 0:04 ` [PATCH] config: complain about --local outside of a git repo Jonathan Nieder
2017-05-13 2:04 ` Jeff King
2017-05-15 0:31 ` Josh Hagins
2017-05-15 3:18 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180507090109.GA367@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=johannes.schindelin@gmx.de \
--cc=pclouds@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).