diff options
| -rw-r--r-- | Documentation/technical/api-error-handling.txt | 24 | ||||
| -rw-r--r-- | Documentation/technical/api-trace2.txt | 4 | ||||
| -rw-r--r-- | builtin/receive-pack.c | 16 | ||||
| -rw-r--r-- | cache-tree.c | 8 | ||||
| -rw-r--r-- | common-main.c | 28 | ||||
| -rw-r--r-- | git-compat-util.h | 14 | ||||
| -rw-r--r-- | parse-options.c | 53 | ||||
| -rw-r--r-- | t/helper/test-trace2.c | 29 | ||||
| -rwxr-xr-x | t/t0210-trace2-normal.sh | 76 | ||||
| -rw-r--r-- | trace2.c | 8 | ||||
| -rw-r--r-- | trace2.h | 8 | ||||
| -rw-r--r-- | usage.c | 33 |
12 files changed, 231 insertions, 70 deletions
diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt index 8be4f4d0d6..70bf1d3e52 100644 --- a/Documentation/technical/api-error-handling.txt +++ b/Documentation/technical/api-error-handling.txt @@ -1,12 +1,34 @@ 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()`. ++ +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. ++ +If you know you had prior calls to `bug()` then calling `BUG()` itself +is equivalent to calling `BUG_if_bug()`, the latter being a wrapper +calling `BUG()` if we've set a flag indicating that we've called +`bug()`. ++ +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. + - `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 f4a8a69087..77a150b30e 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/builtin/receive-pack.c b/builtin/receive-pack.c index e6e1d636f9..31b48e728b 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1810,21 +1810,17 @@ static int should_process_cmd(struct command *cmd) return !cmd->error_string && !cmd->skip_update; } -static void warn_if_skipped_connectivity_check(struct command *commands, +static void BUG_if_skipped_connectivity_check(struct command *commands, struct shallow_info *si) { struct command *cmd; - int checked_connectivity = 1; for (cmd = commands; cmd; cmd = cmd->next) { - if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) { - error("BUG: connectivity check has not been run on ref %s", - cmd->ref_name); - checked_connectivity = 0; - } + 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("connectivity check skipped???"); } static void execute_commands_non_atomic(struct command *commands, @@ -2005,7 +2001,7 @@ static void execute_commands(struct command *commands, execute_commands_non_atomic(commands, si); if (shallow_update) - warn_if_skipped_connectivity_check(commands, si); + BUG_if_skipped_connectivity_check(commands, si); } static struct command **queue_command(struct command **tail, diff --git a/cache-tree.c b/cache-tree.c index a92c23eec3..ff794d940f 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -722,14 +722,14 @@ struct tree* write_in_core_index_as_tree(struct repository *repo) { ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL); 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("%d %.*s", ce_stage(ce), + (int)ce_namelen(ce), ce->name); } - BUG("unmerged index entries when writing inmemory index"); + BUG("unmerged index entries when writing in-core index"); } return lookup_tree(repo, &index_state->cache_tree->oid); diff --git a/common-main.c b/common-main.c index 29fb7452f8..c531372f3f 100644 --- a/common-main.c +++ b/common-main.c @@ -55,10 +55,30 @@ int main(int argc, const char **argv) result = cmd_main(argc, argv); + /* Not exit(3), but a wrapper calling our common_exit() */ + exit(result); +} + +static void check_bug_if_BUG(void) +{ + if (!bug_called_must_BUG) + return; + BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()"); +} + +/* We wrap exit() to call common_exit() in git-compat-util.h */ +int common_exit(const char *file, int line, int code) +{ /* - * We define exit() to call trace2_cmd_exit_fl() in - * git-compat-util.h. Whether we reach this or exit() - * elsewhere we'll always run our trace2 exit handler. + * For non-POSIX systems: Take the lowest 8 bits of the "code" + * to e.g. turn -1 into 255. On a POSIX system this is + * redundant, see exit(3) and wait(2), but as it doesn't harm + * anything there we don't need to guard this with an "ifdef". */ - exit(result); + code &= 0xff; + + check_bug_if_BUG(); + trace2_cmd_exit_fl(file, line, code); + + return code; } diff --git a/git-compat-util.h b/git-compat-util.h index d9457b9aae..fd36d3bfdc 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1326,9 +1326,19 @@ 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_fl(__FILE__, __LINE__, __VA_ARGS__); \ +} while (0) #ifndef FSYNC_METHOD_DEFAULT #ifdef __APPLE__ @@ -1459,8 +1469,8 @@ int cmd_main(int, const char **); * Intercept all calls to exit() and route them to trace2 to * optionally emit a message before calling the real exit(). */ -int trace2_cmd_exit_fl(const char *file, int line, int code); -#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code))) +int common_exit(const char *file, int line, int code); +#define exit(code) exit(common_exit(__FILE__, __LINE__, (code))) /* * You can mark a stack variable with UNLEAK(var) to avoid it being diff --git a/parse-options.c b/parse-options.c index 6e57744fd2..edf55d3ef5 100644 --- a/parse-options.c +++ b/parse-options.c @@ -14,15 +14,15 @@ enum opt_parsed { OPT_UNSET = 1<<1, }; -static int optbug(const struct option *opt, const char *reason) +static void optbug(const struct option *opt, const char *reason) { - if (opt->long_name) { - if (opt->short_name) - return error("BUG: switch '%c' (--%s) %s", - opt->short_name, opt->long_name, reason); - return error("BUG: option '%s' %s", opt->long_name, reason); - } - return error("BUG: switch '%c' %s", opt->short_name, reason); + if (opt->long_name && opt->short_name) + bug("switch '%c' (--%s) %s", opt->short_name, + opt->long_name, reason); + else if (opt->long_name) + bug("option '%s' %s", opt->long_name, reason); + else + bug("switch '%c' %s", opt->short_name, reason); } static const char *optname(const struct option *opt, enum opt_parsed flags) @@ -441,28 +441,27 @@ static void check_typos(const char *arg, const struct option *options) static void parse_options_check(const struct option *opts) { - int err = 0; char short_opts[128]; memset(short_opts, '\0', sizeof(short_opts)); for (; opts->type != OPTION_END; opts++) { if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) && (opts->flags & PARSE_OPT_OPTARG)) - err |= optbug(opts, "uses incompatible flags " - "LASTARG_DEFAULT and OPTARG"); + optbug(opts, "uses incompatible flags " + "LASTARG_DEFAULT and OPTARG"); if (opts->short_name) { if (0x7F <= opts->short_name) - err |= optbug(opts, "invalid short name"); + optbug(opts, "invalid short name"); else if (short_opts[opts->short_name]++) - err |= optbug(opts, "short name already used"); + optbug(opts, "short name already used"); } if (opts->flags & PARSE_OPT_NODASH && ((opts->flags & PARSE_OPT_OPTARG) || !(opts->flags & PARSE_OPT_NOARG) || !(opts->flags & PARSE_OPT_NONEG) || opts->long_name)) - err |= optbug(opts, "uses feature " - "not supported for dashless options"); + optbug(opts, "uses feature " + "not supported for dashless options"); switch (opts->type) { case OPTION_COUNTUP: case OPTION_BIT: @@ -471,33 +470,33 @@ static void parse_options_check(const struct option *opts) case OPTION_NUMBER: if ((opts->flags & PARSE_OPT_OPTARG) || !(opts->flags & PARSE_OPT_NOARG)) - err |= optbug(opts, "should not accept an argument"); + optbug(opts, "should not accept an argument"); break; case OPTION_CALLBACK: if (!opts->callback && !opts->ll_callback) - BUG("OPTION_CALLBACK needs one callback"); - if (opts->callback && opts->ll_callback) - BUG("OPTION_CALLBACK can't have two callbacks"); + optbug(opts, "OPTION_CALLBACK needs one callback"); + else if (opts->callback && opts->ll_callback) + optbug(opts, "OPTION_CALLBACK can't have two callbacks"); break; case OPTION_LOWLEVEL_CALLBACK: if (!opts->ll_callback) - BUG("OPTION_LOWLEVEL_CALLBACK needs a callback"); + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback"); if (opts->callback) - BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback"); + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback"); break; case OPTION_ALIAS: - BUG("OPT_ALIAS() should not remain at this point. " - "Are you using parse_options_step() directly?\n" - "That case is not supported yet."); + 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) */ } if (opts->argh && strcspn(opts->argh, " _") != strlen(opts->argh)) - err |= optbug(opts, "multi-word argh should use dash to separate words"); + optbug(opts, "multi-word argh should use dash to separate words"); } - if (err) - exit(128); + BUG_if_bug("invalid 'struct option'"); } static void parse_options_start_1(struct parse_opt_ctx_t *ctx, diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c index 59b124bb5f..180c7f53f3 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,28 @@ 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("an explicit BUG_if_bug() following bug() call(s) is nice, but not required"); + return 0; +} + +static int ut_009bug_BUG(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; +} + +static int ut_010bug_BUG(int argc, const char **argv) +{ + bug("a bug message"); + BUG("a BUG message"); +} + /* * Usage: * test-tool trace2 <ut_name_1> <ut_usage_1> @@ -222,7 +244,10 @@ 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","" }, + { ut_010bug_BUG, "010bug_BUG","" }, }; /* clang-format on */ diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh index 37c359bd5a..80e76a4695 100755 --- a/t/t0210-trace2-normal.sh +++ b/t/t0210-trace2-normal.sh @@ -168,6 +168,82 @@ 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 + an explicit BUG_if_bug() following bug() call(s) is nice, but not required + 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 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 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() call(s) in this process without explicit BUG_if_bug() + 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 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 + test_cmp expect actual +' + +test_expect_success 'bug messages followed by 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 010bug_BUG 2>err && + cat >expect <<-\EOF && + a bug message + a BUG message + 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 010bug_BUG + cmd_name trace2 (trace2) + error a bug message + error a BUG message + 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 @@ -202,17 +202,15 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv) argv); } -int trace2_cmd_exit_fl(const char *file, int line, int code) +void trace2_cmd_exit_fl(const char *file, int line, int code) { struct tr2_tgt *tgt_j; int j; uint64_t us_now; uint64_t us_elapsed_absolute; - code &= 0xff; - if (!trace2_enabled) - return code; + return; trace_git_fsync_stats(); trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT); @@ -226,8 +224,6 @@ int trace2_cmd_exit_fl(const char *file, int line, int code) if (tgt_j->pfn_exit_fl) tgt_j->pfn_exit_fl(file, line, us_elapsed_absolute, code); - - return code; } void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt, @@ -101,14 +101,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv); /* * Emit an 'exit' event. - * - * Write the exit-code that will be passed to exit() or returned - * from main(). - * - * Use this prior to actually calling exit(). - * See "#define exit()" in git-compat-util.h */ -int trace2_cmd_exit_fl(const char *file, int line, int code); +void trace2_cmd_exit_fl(const char *file, int line, int code); #define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code))) @@ -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(); @@ -317,11 +323,28 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...) { va_list ap; + + bug_called_must_BUG = 0; + va_start(ap, fmt); BUG_vfl(file, line, fmt, ap); 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) { |