[PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 31, 2022, 16:58 UTC
- Message-ID
- <cover-v2-0.6-00000000000-20220531T164806Z-avarab@gmail.com>
- In-Reply-To
- <cover-0.5-00000000000-20220521T170939Z-avarab@gmail.com>
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@gmail.com/):
* 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@chooglen-macbookpro.roam.corp.google.com
Æ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@gmail.com>
## 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@gmail.com>
@@ 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