This series adds a bug() (lower-case) function to go along with BUG(). As seen in 3-5/6 this makes it much easier to handle the cases such as parse-options.c where we'd like to call BUG(), but would like to first exhaustively accumulate the N issues we spot before doing so, and not merely BUG() out on the first one. Changes since v1 (https://lore.kernel.org/git/cover-0.5-00000000000-20220521T170939Z-avarab@xxxxxxxxx/): * Move the exit() wrapper to common-main.c, I tried to add a "common-exit.c" or just rename "common-main.c" to "common.c", but due to how the CMake build system declares it those changes would result in a lot of churn, so for now just adding it to common-main.c makes more sense. * Typo/grammar fixes in commit messages, as pointed out in review. * The BUG_if_bug() function is now optional, and the docs have been updated to reflect that. * The BUG_if_bug() function now takes a va_args like BUG() to indicate what the problem was. * Updated 3/6 to note that the exit(128) code changes with a migration to BUG(). * Fix logic error in 4/6: We now "break" after calling bug(), to behave as the previous code did. * Fix logic error in 5/6, which now makes use of the new argument(s) to BUG_if_bug(). * There was some suggestion of ejecting 6/6, but I've instead replaced it with the implementation Glen suggested in http://lore.kernel.org/git/kl6lk0a6mzp0.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx Ævar Arnfjörð Bjarmason (6): common-main.o: move non-trace2 exit() behavior out of trace2.c usage.c: add a non-fatal bug() function to go with BUG() parse-options.c: use new bug() API for optbug() parse-options.c: use optbug() instead of BUG() "opts" check receive-pack: use bug() and BUG_if_bug() cache-tree.c: use bug() and BUG_if_bug() .../technical/api-error-handling.txt | 19 +++++- Documentation/technical/api-trace2.txt | 4 +- builtin/receive-pack.c | 16 ++--- cache-tree.c | 8 +-- common-main.c | 30 ++++++++- git-compat-util.h | 24 ++++++- parse-options.c | 67 ++++++++++--------- t/helper/test-trace2.c | 22 +++++- t/t0210-trace2-normal.sh | 52 ++++++++++++++ trace2.c | 8 +-- trace2.h | 8 +-- usage.c | 30 +++++++-- 12 files changed, 217 insertions(+), 71 deletions(-) Range-diff against v1: -: ----------- > 1: d446e4679d4 common-main.o: move non-trace2 exit() behavior out of trace2.c 1: faa1c708a79 ! 2: 2d0527f86dc usage.c: add a non-fatal bug() function to go with BUG() @@ Commit message usage.c: add a non-fatal bug() function to go with BUG() 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 + runtime BUG(), but would like to defer the BUG() call because we're possibly accumulating more bug() callers to exhaustively indicate what went wrong. @@ Commit message 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(), + Any caller to bug() can 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 + to bug(), callers can also decide not to call BUG_if_bug() and leave + the resulting BUG() invocation until exit() time. There are currently + no bug() API users that don't call BUG_if_bug() themselves after a + for-loop, but allowing for not calling BUG_if_bug() keeps the API + flexible. 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 @@ Documentation/technical/api-error-handling.txt 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. ++ prints a "BUG" message instead of calling `abort()`. +++ ++A call to `bug()` will then result in a "real" call to the `BUG()` ++function, either explicitly by invoking `BUG_if_bug()` after call(s) ++to `bug()`, or implicitly at `exit()` time where we'll check if we ++encountered any outstanding `bug()` invocations. +++ ++If there were no prior calls to `bug()` before invoking `BUG_if_bug()` ++the latter is a NOOP. The `BUG_if_bug()` function takes the same ++arguments as `BUG()` itself. Calling `BUG_if_bug()` explicitly isn't ++necessary, but ensures that we die as soon as possible. ++ +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. @@ Documentation/technical/api-trace2.txt: completed.) ------------ { + ## common-main.c ## +@@ common-main.c: int main(int argc, const char **argv) + exit(result); + } + ++static void check_bug_if_BUG(void) ++{ ++ if (!bug_called_must_BUG) ++ return; ++ ++ /* BUG_vfl() calls exit(), which calls us again */ ++ bug_called_must_BUG = 0; ++ BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()"); ++} ++ + int common_exit(const char *file, int line, int code) + { + /* +@@ common-main.c: int common_exit(const char *file, int line, int code) + */ + code &= 0xff; + ++ check_bug_if_BUG(); + trace2_cmd_exit_fl(file, line, code); + + return code; + ## git-compat-util.h ## @@ git-compat-util.h: 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) */ @@ git-compat-util.h: static inline int regexec_buf(const regex_t *preg, const char +__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 { \ ++#define BUG_if_bug(...) do { \ + if (bug_called_must_BUG) { \ + bug_called_must_BUG = 0; \ -+ BUG_fl(__FILE__, __LINE__, "see bug() output above"); \ ++ BUG_fl(__FILE__, __LINE__, __VA_ARGS__); \ + } \ +} while (0) @@ t/helper/test-trace2.c: static int ut_007bug(int argc, const char **argv) +{ + bug("a bug message"); + bug("another bug message"); -+ BUG_if_bug(); ++ BUG_if_bug("an explicit BUG_if_bug() following bug() call(s) is nice, but not required"); + return 0; +} + @@ t/helper/test-trace2.c: static int ut_007bug(int argc, const char **argv) +{ + bug("a bug message"); + bug("another bug message"); ++ /* The BUG_if_bug(...) isn't here, but we'll spot bug() calls on exit()! */ + return 0; +} + @@ t/t0210-trace2-normal.sh: test_expect_success 'BUG messages are written to trace + cat >expect <<-\EOF && + a bug message + another bug message -+ see bug() output above ++ an explicit BUG_if_bug() following bug() call(s) is nice, but not required + EOF + sed "s/^.*: //" <err >actual && + test_cmp expect actual && @@ t/t0210-trace2-normal.sh: test_expect_success 'BUG messages are written to trace + cmd_name trace2 (trace2) + error a bug message + error another bug message -+ error see bug() output above ++ error an explicit BUG_if_bug() following bug() call(s) is nice, but not required + 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_expect_success 'bug messages without explicit 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 ++ had bug() call(s) in this process without explicit BUG_if_bug() + EOF + sed "s/^.*: //" <err >actual && + test_cmp expect actual && @@ t/t0210-trace2-normal.sh: test_expect_success 'BUG messages are written to trace + cmd_name trace2 (trace2) + error a bug message + error another bug message -+ error had bug() output above, in addition missed BUG_if_bug() call ++ error on exit(): had bug() call(s) in this process without explicit BUG_if_bug() + exit elapsed:_TIME_ code:99 + atexit elapsed:_TIME_ code:99 + EOF @@ t/t0210-trace2-normal.sh: test_expect_success 'BUG messages are written to trace # Now test without environment variables and get all Trace2 settings - ## trace2.c ## -@@ trace2.c: 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; - - ## usage.c ## @@ usage.c: void warning(const char *warn, ...) /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */ 2: a599cd015a3 ! 3: 4a7089fbbf2 parse-options.c: use new bug() API for optbug() @@ Commit message been using optbug() that aren't being changed here. That'll be done in a subsequent commit. This only changes the optbug() callers. + Since this will invoke BUG() the previous exit(128) code will be + changed, but in this case that's what we want, i.e. to have + encountering a BUG() return the specific "BUG" exit code. + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## parse-options.c ## @@ parse-options.c: static void parse_options_check(const struct option *opts) } - if (err) - exit(128); -+ BUG_if_bug(); ++ BUG_if_bug("invalid 'struct option'"); } static void parse_options_start_1(struct parse_opt_ctx_t *ctx, 3: 5a3e7609854 ! 4: 47d384d0ae5 parse-options.c: use optbug() instead of BUG() "opts" check @@ Commit message ## parse-options.c ## @@ parse-options.c: static void parse_options_check(const struct option *opts) + optbug(opts, "should not accept an argument"); break; case OPTION_CALLBACK: - if (!opts->callback && !opts->ll_callback) +- if (!opts->callback && !opts->ll_callback) - BUG("OPTION_CALLBACK needs one callback"); -+ optbug(opts, "OPTION_CALLBACK needs one callback"); - if (opts->callback && opts->ll_callback) +- if (opts->callback && opts->ll_callback) - BUG("OPTION_CALLBACK can't have two callbacks"); ++ if (!opts->callback && !opts->ll_callback) { ++ optbug(opts, "OPTION_CALLBACK needs one callback"); ++ break; ++ } ++ if (opts->callback && opts->ll_callback) { + optbug(opts, "OPTION_CALLBACK can't have two callbacks"); ++ break; ++ } break; case OPTION_LOWLEVEL_CALLBACK: - if (!opts->ll_callback) +- if (!opts->ll_callback) - BUG("OPTION_LOWLEVEL_CALLBACK needs a callback"); -+ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback"); - if (opts->callback) +- if (opts->callback) - BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback"); ++ if (!opts->ll_callback) { ++ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback"); ++ break; ++ } ++ if (opts->callback) { + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback"); ++ break; ++ } break; case OPTION_ALIAS: - BUG("OPT_ALIAS() should not remain at this point. " @@ parse-options.c: static void parse_options_check(const struct option *opts) + optbug(opts, "OPT_ALIAS() should not remain at this point. " + "Are you using parse_options_step() directly?\n" + "That case is not supported yet."); ++ break; default: ; /* ok. (usually accepts an argument) */ } 4: c590f4273c0 ! 5: fe5c3926675 receive-pack: use bug() and BUG_if_bug() @@ Commit message Amend code added in a6a84319686 (receive-pack.c: shorten the execute_commands loop over all commands, 2015-01-07) and amended to hard die in b6a4788586d (receive-pack.c: die instead of error in case - of possible future bug, 2015-01-07) to the new bug() function instead. + of possible future bug, 2015-01-07) to use the new bug() function + instead. Let's also rename the warn_if_*() function that code is in to BUG_if_*(), its name became outdated in b6a4788586d. @@ builtin/receive-pack.c: static int should_process_cmd(struct command *cmd) - cmd->ref_name); - checked_connectivity = 0; - } -+ if (!should_process_cmd(cmd) && si->shallow_ref[cmd->index]) ++ if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) + bug("connectivity check has not been run on ref %s", + cmd->ref_name); } - if (!checked_connectivity) - BUG("connectivity check skipped???"); -+ BUG_if_bug(); ++ BUG_if_bug("connectivity check skipped???"); } static void execute_commands_non_atomic(struct command *commands, 5: bb5a53f3b73 ! 6: cbbe0276966 cache-tree.c: use bug() and BUG_if_bug() @@ Commit message 19c6a4f8369 (merge-recursive: do not return NULL only to cause segfault, 2010-01-21) to use the new bug() function. - This gets the same job done with less code, this changes the output a - bit, but since we're emitting BUG output let's say it's OK to prefix - every line with the "unmerged index entry" message, instead of - optimizing for readability. doing it this way gets rid of any state - management in the loop itself in favor of BUG_if_bug(). + This gets the same job done with slightly less code, as we won't need + to prefix lines with "BUG: ". More importantly we'll now log the full + set of messages via trace2, before this we'd only log the one BUG() + invocation. + + While we're at it let's replace "There" with "there" in the message, + i.e. not start a message with a capital letter, per the + CodingGuidelines. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ cache-tree.c: struct tree* write_in_core_index_as_tree(struct repository *repo) if (ret == WRITE_TREE_UNMERGED_INDEX) { int i; - fprintf(stderr, "BUG: There are unmerged index entries:\n"); ++ bug("there are unmerged index entries:"); for (i = 0; i < index_state->cache_nr; i++) { const struct cache_entry *ce = index_state->cache[i]; if (ce_stage(ce)) - fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce), - (int)ce_namelen(ce), ce->name); -+ bug("unmerged index entry on in-memory index write: %d %.*s", -+ ce_stage(ce), (int)ce_namelen(ce), ce->name); ++ bug("%d %.*s", ce_stage(ce), ++ (int)ce_namelen(ce), ce->name); } - BUG("unmerged index entries when writing inmemory index"); -+ BUG_if_bug(); ++ bug("unmerged index entries when writing inmemory index"); } return lookup_tree(repo, &index_state->cache_tree->oid); -- 2.36.1.1100.g16130010d07
