| Message ID | patch-1.5-faa1c708a79-20220521T170939Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usage API: add and use a bug() + BUG_if_bug() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Any caller to bug() should follow up such calls with BUG_if_bug(), > which will BUG() out (i.e. abort()) if there were any preceding calls > to bug(). As the tests and documentation here show we'll catch missing > BUG_if_bug() invocations in our exit() wrapper. ... > +- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but > + prints a "BUG" message instead of calling `abort()`. We then expect > + `BUG_if_bug()` to be called to `abort()` if there were any calls to > + `bug()`. If there weren't any a call to `BUG_if_bug()` is a NOOP. OK. So the expected pattern would be a series of calls to bug(), each guarded by its own condition, concluded by a call to BUG_if_bug() if (condition1) bug(...); if (condition2) bug(...); ... BUG_if_bug(); and when none of the guard conditions fired, BUG_if_bug() will become a no-op. > +/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */ > +extern int bug_called_must_BUG; I am not sure about the name, but in essense, each call to bug() ensures that this becomes true, so BUG_if_bug() can use it to see if it should abort(), right? > __attribute__((format (printf, 3, 4))) NORETURN > void BUG_fl(const char *file, int line, const char *fmt, ...); > #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__) > +__attribute__((format (printf, 3, 4))) > +void bug_fl(const char *file, int line, const char *fmt, ...); > +#define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__) > +#define BUG_if_bug() do { \ > + if (bug_called_must_BUG) { \ > + bug_called_must_BUG = 0; \ > + BUG_fl(__FILE__, __LINE__, "see bug() output above"); \ > + } \ > +} while (0) I have a feeling that "see bug() output above" should come from the caller of BUG_if_bug(). These bug() calls that are grouped together must have a shared overall theme, which may want to be stated in that final message. Other than these two small points, this does not look too bad ;-) Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> +/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */ >> +extern int bug_called_must_BUG; > > I am not sure about the name, ... I finally figured out why I found this name so disturbing. This is written from the viewpoint of somebody who is trying to catch a programmer's error of calling bug() without calling BUG_if_bug(); it is not named to help the users of API to understand it better. I wonder if it makes sense to make the call to BUG_if_bug() totally optional. The rule becomes slightly different: * You can call bug() zero or more times. It issues a fatal error message, and internally remembers the fact that we detected a programming error that invalidates the execution of this program, without immediately terminating the program. * When you exit() from the program, the runtime consults that "did we call bug()?" record and makes the program exit with known exit code (we could arrange it to abort() just like BUG, but I'd prefer controlled crash). * But it is inconvenient to always keep going, after you may have called one or more bug(), to the normal program completion. So there is a helper exit_if_called_bug(), which is an equivalent to checking the "did we call bug()?" record and calling exit(). By making BUG_if_bug() optional, we can lose a handful of lines from the test helper, because it makes it a non-issue to exit the program without calling it. Hmm?
On Wed, May 25 2022, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >>> +/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */ >>> +extern int bug_called_must_BUG; >> >> I am not sure about the name, ... > > I finally figured out why I found this name so disturbing. This is > written from the viewpoint of somebody who is trying to catch a > programmer's error of calling bug() without calling BUG_if_bug(); > it is not named to help the users of API to understand it better. I named it like that to indicate a "lesser bug", I think BUG_if_bug() came later, because ... > I wonder if it makes sense to make the call to BUG_if_bug() totally > optional. The rule becomes slightly different: > > * You can call bug() zero or more times. It issues a fatal error > message, and internally remembers the fact that we detected a > programming error that invalidates the execution of this program, > without immediately terminating the program. > > * When you exit() from the program, the runtime consults that "did > we call bug()?" record and makes the program exit with known exit > code (we could arrange it to abort() just like BUG, but I'd > prefer controlled crash). > > * But it is inconvenient to always keep going, after you may have > called one or more bug(), to the normal program completion. So > there is a helper exit_if_called_bug(), which is an equivalent to > checking the "did we call bug()?" record and calling exit(). > > By making BUG_if_bug() optional, we can lose a handful of lines from > the test helper, because it makes it a non-issue to exit the program > without calling it. I don't think we should do it like that and keep it a BUG() not to call BUG_if_bug() when we hit exit(), because e.g. in the case of parse-options.c once we have the a bad "struct options" we don't want to continue, as we might segfault, or have other bad behavior etc. So we'd like to BUG() out as soon as possible. That's how we use BUG() itself, i.e. we think the program execution is bad and we immediately abort(), the new bug() makes a small concession that we may be OK for a little while (e.g. while looping over the options), but would like to BUG() out immediately after that.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I don't think we should do it like that and keep it a BUG() not to call > BUG_if_bug() when we hit exit(), because e.g. in the case of > parse-options.c once we have the a bad "struct options" we don't want to > continue, as we might segfault, or have other bad behavior etc. So we'd > like to BUG() out as soon as possible. Oh, there is no question about that. When we detect that the program is in an inconsistent and unexpected state, we would want to bug out instead of continuing at some point, and that is why we would want to have BUG_if_bug(), or exit_if_called_bug(), as I called in the message you are reponding to. What I am getting at is that the code often or usually calls BUG_if_bug() is not a reason to require it to be called, especially if there are conditional calls to bug() near the end of the program. Imagine a program, after finishing to respond to the end-user request, before the end of the program, performing some self sanity checks with a series of "if (condition) bug()", and there is no more thing that needs to be done other than exiting after such check. I would have added such a call to sanity_check_refcnt() at the end of "git blame", for example, if the facility existed. Thanks.
On Thu, May 26 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I don't think we should do it like that and keep it a BUG() not to call >> BUG_if_bug() when we hit exit(), because e.g. in the case of >> parse-options.c once we have the a bad "struct options" we don't want to >> continue, as we might segfault, or have other bad behavior etc. So we'd >> like to BUG() out as soon as possible. > > Oh, there is no question about that. When we detect that the > program is in an inconsistent and unexpected state, we would want to > bug out instead of continuing at some point, and that is why we would > want to have BUG_if_bug(), or exit_if_called_bug(), as I called in > the message you are reponding to. > > What I am getting at is that the code often or usually calls > BUG_if_bug() is not a reason to require it to be called, especially > if there are conditional calls to bug() near the end of the program. > Imagine a program, after finishing to respond to the end-user > request, before the end of the program, performing some self sanity > checks with a series of "if (condition) bug()", and there is no more > thing that needs to be done other than exiting after such check. I > would have added such a call to sanity_check_refcnt() at the end of > "git blame", for example, if the facility existed. But you wouldn't want to just stick BUG_if_bug() at the end of those sanity checks? I suppose I can drop the paranoia of requiring it, but since all existing callers want to act that way, and I couldn't imagine a case where you didn't want that I made it act that way.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, May 26 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> I don't think we should do it like that and keep it a BUG() not to call >>> BUG_if_bug() when we hit exit(), because e.g. in the case of >>> parse-options.c once we have the a bad "struct options" we don't want to >>> continue, as we might segfault, or have other bad behavior etc. So we'd >>> like to BUG() out as soon as possible. >> >> Oh, there is no question about that. When we detect that the >> program is in an inconsistent and unexpected state, we would want to >> bug out instead of continuing at some point, and that is why we would >> want to have BUG_if_bug(), or exit_if_called_bug(), as I called in >> the message you are reponding to. >> >> What I am getting at is that the code often or usually calls >> BUG_if_bug() is not a reason to require it to be called, especially >> if there are conditional calls to bug() near the end of the program. >> Imagine a program, after finishing to respond to the end-user >> request, before the end of the program, performing some self sanity >> checks with a series of "if (condition) bug()", and there is no more >> thing that needs to be done other than exiting after such check. I >> would have added such a call to sanity_check_refcnt() at the end of >> "git blame", for example, if the facility existed. > > But you wouldn't want to just stick BUG_if_bug() at the end of those > sanity checks? > > I suppose I can drop the paranoia of requiring it, but since all > existing callers want to act that way, and I couldn't imagine a case > where you didn't want that I made it act that way. It just is one more thing that needs testing. But in any case, that was more or less a tongue-in-cheek suggestion and not a serious counter proposal. Let's drop it now and stop wasting our time. Thanks.
On 2022.05.21 19:14, Ævar Arnfjörð Bjarmason wrote: > Add a bug() function to use in cases where we'd like to indicate a > runtime BUG(), but would like to deref the BUG() call because we're Typo: I assume you meant s/deref/defer/ [snip] > diff --git a/trace2.c b/trace2.c > index e01cf77f1a8..d49d5d5a082 100644 > --- a/trace2.c > +++ b/trace2.c > @@ -211,6 +211,12 @@ int trace2_cmd_exit_fl(const char *file, int line, int code) > > code &= 0xff; > > + if (bug_called_must_BUG) { > + /* BUG_vfl() calls exit(), which calls us again */ > + bug_called_must_BUG = 0; > + BUG("had bug() output above, in addition missed BUG_if_bug() call"); > + } > + > if (!trace2_enabled) > return code; While it does seem that trace2_cmd_exit_fl() is the only reasonable place to put this cleanup code, perhaps it makes sense to rename this function and move it out of trace2.c, to more clearly show that it has multiple responsibilities now?
Josh Steadmon <steadmon@google.com> writes: > On 2022.05.21 19:14, Ævar Arnfjörð Bjarmason wrote: >> Add a bug() function to use in cases where we'd like to indicate a >> runtime BUG(), but would like to deref the BUG() call because we're > > Typo: I assume you meant s/deref/defer/ > > > [snip] > >> diff --git a/trace2.c b/trace2.c >> index e01cf77f1a8..d49d5d5a082 100644 >> --- a/trace2.c >> +++ b/trace2.c >> @@ -211,6 +211,12 @@ int trace2_cmd_exit_fl(const char *file, int line, int code) >> >> code &= 0xff; >> >> + if (bug_called_must_BUG) { >> + /* BUG_vfl() calls exit(), which calls us again */ >> + bug_called_must_BUG = 0; >> + BUG("had bug() output above, in addition missed BUG_if_bug() call"); >> + } >> + >> if (!trace2_enabled) >> return code; > > While it does seem that trace2_cmd_exit_fl() is the only reasonable > place to put this cleanup code, perhaps it makes sense to rename this > function and move it out of trace2.c, to more clearly show that it has > multiple responsibilities now? Sounds good. This is to exit() as common_main() is to main(), it seems to me?
On Thu, May 26 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I don't think we should do it like that and keep it a BUG() not to call >> BUG_if_bug() when we hit exit(), because e.g. in the case of >> parse-options.c once we have the a bad "struct options" we don't want to >> continue, as we might segfault, or have other bad behavior etc. So we'd >> like to BUG() out as soon as possible. > > Oh, there is no question about that. When we detect that the > program is in an inconsistent and unexpected state, we would want to > bug out instead of continuing at some point, and that is why we would > want to have BUG_if_bug(), or exit_if_called_bug(), as I called in > the message you are reponding to. > > What I am getting at is that the code often or usually calls > BUG_if_bug() is not a reason to require it to be called, especially > if there are conditional calls to bug() near the end of the program. > Imagine a program, after finishing to respond to the end-user > request, before the end of the program, performing some self sanity > checks with a series of "if (condition) bug()", and there is no more > thing that needs to be done other than exiting after such check. I > would have added such a call to sanity_check_refcnt() at the end of > "git blame", for example, if the facility existed. I'm re-rolling this and FWIW came up with this on top of the re-roll, but didn't include it. It could also call the find_alignment() and output(), but for this it seemed just leaving it at the bug() calls was sufficient: diff --git a/blame.c b/blame.c index da1052ac94b..84c112f76bd 100644 --- a/blame.c +++ b/blame.c @@ -1155,21 +1155,15 @@ static int compare_commits_by_reverse_commit_date(const void *a, */ static void sanity_check_refcnt(struct blame_scoreboard *sb) { - int baa = 0; struct blame_entry *ent; - for (ent = sb->ent; ent; ent = ent->next) { + for (ent = sb->ent; ent; ent = ent->next) /* Nobody should have zero or negative refcnt */ - if (ent->suspect->refcnt <= 0) { - fprintf(stderr, "%s in %s has negative refcnt %d\n", - ent->suspect->path, - oid_to_hex(&ent->suspect->commit->object.oid), - ent->suspect->refcnt); - baa = 1; - } - } - if (baa) - sb->on_sanity_fail(sb, baa); + if (ent->suspect->refcnt <= 0) + bug("%s in %s has negative refcnt %d", + ent->suspect->path, + oid_to_hex(&ent->suspect->commit->object.oid), + ent->suspect->refcnt); } /* diff --git a/blame.h b/blame.h index 38bde535b3d..f110bf3c40e 100644 --- a/blame.h +++ b/blame.h @@ -154,7 +154,6 @@ struct blame_scoreboard { int debug; /* callbacks */ - void(*on_sanity_fail)(struct blame_scoreboard *, int); void(*found_guilty_entry)(struct blame_entry *, void *); void *found_guilty_entry_data; diff --git a/builtin/blame.c b/builtin/blame.c index e33372c56b0..70f31e94d38 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -655,14 +655,6 @@ static void find_alignment(struct blame_scoreboard *sb, int *option) abbrev = auto_abbrev + 1; } -static void sanity_check_on_fail(struct blame_scoreboard *sb, int baa) -{ - int opt = OUTPUT_SHOW_SCORE | OUTPUT_SHOW_NUMBER | OUTPUT_SHOW_NAME; - find_alignment(sb, &opt); - output(sb, opt); - die("Baa %d!", baa); -} - static unsigned parse_score(const char *arg) { char *end; @@ -1151,7 +1143,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix) sb.copy_score = blame_copy_score; sb.debug = DEBUG_BLAME; - sb.on_sanity_fail = &sanity_check_on_fail; sb.show_root = show_root; sb.xdl_opts = xdl_opts;
diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt index 8be4f4d0d6a..f4dffea4af0 100644 --- a/Documentation/technical/api-error-handling.txt +++ b/Documentation/technical/api-error-handling.txt @@ -1,12 +1,27 @@ Error reporting in git ====================== -`BUG`, `die`, `usage`, `error`, and `warning` report errors of +`BUG`, `bug`, `die`, `usage`, `error`, and `warning` report errors of various kinds. - `BUG` is for failed internal assertions that should never happen, i.e. a bug in git itself. +- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but + prints a "BUG" message instead of calling `abort()`. We then expect + `BUG_if_bug()` to be called to `abort()` if there were any calls to + `bug()`. If there weren't any a call to `BUG_if_bug()` is a NOOP. ++ +This is for the convenience of APIs who'd like to potentially report +more than one "bug", such as the optbug() validation in +parse-options.c. ++ +We call `BUG_if_bug()` ourselves at `exit()` time (via a wrapper, not +`atexit()`), which guarantees that we'll catch cases where we forgot +to invoke `BUG_if_bug()` after calls to `bug()`. Thus calling +`BUG_if_bug()` isn't strictly necessary, but ensures that we die as +soon as possible. + - `die` is for fatal application errors. It prints a message to the user and exits with status 128. diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt index f4a8a690878..77a150b30ee 100644 --- a/Documentation/technical/api-trace2.txt +++ b/Documentation/technical/api-trace2.txt @@ -465,8 +465,8 @@ completed.) ------------ `"error"`:: - This event is emitted when one of the `BUG()`, `error()`, `die()`, - `warning()`, or `usage()` functions are called. + This event is emitted when one of the `BUG()`, `bug()`, `error()`, + `die()`, `warning()`, or `usage()` functions are called. + ------------ { diff --git a/git-compat-util.h b/git-compat-util.h index 58fd813bd01..23b36053af4 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1269,9 +1269,21 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size, /* usage.c: only to be used for testing BUG() implementation (see test-tool) */ extern int BUG_exit_code; +/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */ +extern int bug_called_must_BUG; + __attribute__((format (printf, 3, 4))) NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...); #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__) +__attribute__((format (printf, 3, 4))) +void bug_fl(const char *file, int line, const char *fmt, ...); +#define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__) +#define BUG_if_bug() do { \ + if (bug_called_must_BUG) { \ + bug_called_must_BUG = 0; \ + BUG_fl(__FILE__, __LINE__, "see bug() output above"); \ + } \ +} while (0) #ifdef __APPLE__ #define FSYNC_METHOD_DEFAULT FSYNC_METHOD_WRITEOUT_ONLY diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c index 59b124bb5f1..9d2ad744840 100644 --- a/t/helper/test-trace2.c +++ b/t/helper/test-trace2.c @@ -198,7 +198,7 @@ static int ut_006data(int argc, const char **argv) return 0; } -static int ut_007bug(int argc, const char **argv) +static int ut_007BUG(int argc, const char **argv) { /* * Exercise BUG() to ensure that the message is printed to trace2. @@ -206,6 +206,21 @@ static int ut_007bug(int argc, const char **argv) BUG("the bug message"); } +static int ut_008bug(int argc, const char **argv) +{ + bug("a bug message"); + bug("another bug message"); + BUG_if_bug(); + return 0; +} + +static int ut_009bug_BUG(int argc, const char **argv) +{ + bug("a bug message"); + bug("another bug message"); + return 0; +} + /* * Usage: * test-tool trace2 <ut_name_1> <ut_usage_1> @@ -222,7 +237,9 @@ static struct unit_test ut_table[] = { { ut_004child, "004child", "[<child_command_line>]" }, { ut_005exec, "005exec", "<git_command_args>" }, { ut_006data, "006data", "[<category> <key> <value>]+" }, - { ut_007bug, "007bug", "" }, + { ut_007BUG, "007bug", "" }, + { ut_008bug, "008bug", "" }, + { ut_009bug_BUG, "009bug_BUG","" }, }; /* clang-format on */ diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh index 37c359bd5a2..7c0e0017ad3 100755 --- a/t/t0210-trace2-normal.sh +++ b/t/t0210-trace2-normal.sh @@ -168,6 +168,58 @@ test_expect_success 'BUG messages are written to trace2' ' test_cmp expect actual ' +test_expect_success 'bug messages with BUG_if_bug() are written to trace2' ' + test_when_finished "rm trace.normal actual expect" && + test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \ + test-tool trace2 008bug 2>err && + cat >expect <<-\EOF && + a bug message + another bug message + see bug() output above + EOF + sed "s/^.*: //" <err >actual && + test_cmp expect actual && + + perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual && + cat >expect <<-EOF && + version $V + start _EXE_ trace2 008bug + cmd_name trace2 (trace2) + error a bug message + error another bug message + error see bug() output above + exit elapsed:_TIME_ code:99 + atexit elapsed:_TIME_ code:99 + EOF + test_cmp expect actual +' + +test_expect_success 'bug messages without BUG_if_bug() are written to trace2' ' + test_when_finished "rm trace.normal actual expect" && + test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \ + test-tool trace2 009bug_BUG 2>err && + cat >expect <<-\EOF && + a bug message + another bug message + had bug() output above, in addition missed BUG_if_bug() call + EOF + sed "s/^.*: //" <err >actual && + test_cmp expect actual && + + perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual && + cat >expect <<-EOF && + version $V + start _EXE_ trace2 009bug_BUG + cmd_name trace2 (trace2) + error a bug message + error another bug message + error had bug() output above, in addition missed BUG_if_bug() call + exit elapsed:_TIME_ code:99 + atexit elapsed:_TIME_ code:99 + EOF + test_cmp expect actual +' + sane_unset GIT_TRACE2_BRIEF # Now test without environment variables and get all Trace2 settings diff --git a/trace2.c b/trace2.c index e01cf77f1a8..d49d5d5a082 100644 --- a/trace2.c +++ b/trace2.c @@ -211,6 +211,12 @@ int trace2_cmd_exit_fl(const char *file, int line, int code) code &= 0xff; + if (bug_called_must_BUG) { + /* BUG_vfl() calls exit(), which calls us again */ + bug_called_must_BUG = 0; + BUG("had bug() output above, in addition missed BUG_if_bug() call"); + } + if (!trace2_enabled) return code; diff --git a/usage.c b/usage.c index b738dd178b3..c37b94f1a40 100644 --- a/usage.c +++ b/usage.c @@ -290,18 +290,24 @@ void warning(const char *warn, ...) /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */ int BUG_exit_code; -static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params) +static void BUG_vfl_common(const char *file, int line, const char *fmt, + va_list params) { char prefix[256]; - va_list params_copy; - static int in_bug; - - va_copy(params_copy, params); /* truncation via snprintf is OK here */ snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line); vreportf(prefix, fmt, params); +} + +static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params) +{ + va_list params_copy; + static int in_bug; + + va_copy(params_copy, params); + BUG_vfl_common(file, line, fmt, params); if (in_bug) abort(); @@ -322,6 +328,20 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...) va_end(ap); } +int bug_called_must_BUG; +void bug_fl(const char *file, int line, const char *fmt, ...) +{ + va_list ap, cp; + + bug_called_must_BUG = 1; + + va_copy(cp, ap); + va_start(ap, fmt); + BUG_vfl_common(file, line, fmt, ap); + va_end(ap); + trace2_cmd_error_va(fmt, cp); +} + #ifdef SUPPRESS_ANNOTATED_LEAKS void unleak_memory(const void *ptr, size_t len) {
Add a bug() function to use in cases where we'd like to indicate a runtime BUG(), but would like to deref the BUG() call because we're possibly accumulating more bug() callers to exhaustively indicate what went wrong. We already have this sort of facility in various parts of the codebase, just in the form of ad-hoc re-inventions of the functionality that this new API provides. E.g. this will be used to replace optbug() in parse-options.c, and the 'error("BUG:[...]' we do in a loop in builtin/receive-pack.c. Unlike the code this replaces we'll log to trace2 with this new bug() function (as with other usage.c functions, including BUG()), we'll also be able to avoid calls to xstrfmt() in some cases, as the bug() function itself accepts variadic sprintf()-like arguments. Any caller to bug() should follow up such calls with BUG_if_bug(), which will BUG() out (i.e. abort()) if there were any preceding calls to bug(). As the tests and documentation here show we'll catch missing BUG_if_bug() invocations in our exit() wrapper. I'd previously proposed this as part of another series[1], in that use-case we ended thinking a BUG() would be better (and eventually 96e41f58fe1 (fsck: report invalid object type-path combinations, 2021-10-01) ended up with neither). Here though we'll end up converting various existing code that was already doing what we're doing better with this new API. 1. https://lore.kernel.org/git/YGRXomWwRYPdXZi3@coredump.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- .../technical/api-error-handling.txt | 17 +++++- Documentation/technical/api-trace2.txt | 4 +- git-compat-util.h | 12 +++++ t/helper/test-trace2.c | 21 +++++++- t/t0210-trace2-normal.sh | 52 +++++++++++++++++++ trace2.c | 6 +++ usage.c | 30 +++++++++-- 7 files changed, 132 insertions(+), 10 deletions(-)