From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Bagas Sanjaya" <bagasdotme@gmail.com>,
"Abhradeep Chakraborty" <chakrabortyabhradeep79@gmail.com>,
"Josh Steadmon" <steadmon@google.com>,
"Glen Choo" <chooglen@google.com>,
"Andrei Rybak" <rybak.a.v@gmail.com>,
"Emily Shaffer" <emilyshaffer@google.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug()
Date: Thu, 2 Jun 2022 14:25:31 +0200 [thread overview]
Message-ID: <cover-v3-0.6-00000000000-20220602T122106Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-v2-0.6-00000000000-20220531T164806Z-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 v2[1]:
* Fix up commentary in 1/6 to address Junio's suggestions, and
de-duplicate previously added commentary (just the one existing
comment in git-compat-util.h sufficed).
* The BUG() function itself now clears bug_called_must_BUG, as
suggested by Glen. This simplifies things, and allows "BUG()" to be
called after a sequence of "BUG()" instead of "BUG_if_bug()". That
pattern is now used in 6/6.
I pondered adding a message that BUG() would emit in that case,
i.e. to say that we'd previously called bug() and were now calling
BUG(), but left it. I think the result in 6/6 is better as a
result.
* Used "else if" in 4/6 and got rid of "break" to exhaustively log
errors.
* Got rid of a "below the fold" part of a commit message.
1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20220531T164806Z-avarab@gmail.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 | 24 +++++-
Documentation/technical/api-trace2.txt | 4 +-
builtin/receive-pack.c | 16 ++--
cache-tree.c | 6 +-
common-main.c | 28 ++++++-
git-compat-util.h | 14 +++-
parse-options.c | 53 +++++++------
t/helper/test-trace2.c | 29 ++++++-
t/t0210-trace2-normal.sh | 76 +++++++++++++++++++
trace2.c | 8 +-
trace2.h | 8 +-
usage.c | 33 ++++++--
12 files changed, 230 insertions(+), 69 deletions(-)
Range-diff against v2:
1: d446e4679d4 ! 1: 9c4f8d840e9 common-main.o: move non-trace2 exit() behavior out of trace2.c
@@ Commit message
## common-main.c ##
@@ common-main.c: int main(int argc, const char **argv)
+
result = cmd_main(argc, argv);
- /*
-- * We define exit() to call trace2_cmd_exit_fl() in
-- * git-compat-util.h. Whether we reach this or exit()
-+ * We define exit() to call common_exit(), which will in turn
-+ * call trace2_cmd_exit_fl(). Whether we reach this or exit()
- * elsewhere we'll always run our trace2 exit handler.
- */
- exit(result);
- }
++ /* Not exit(3), but a wrapper calling our common_exit() */
++ exit(result);
++}
+
++/* 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;
+
+ trace2_cmd_exit_fl(file, line, code);
+
+ return code;
-+}
+ }
## git-compat-util.h ##
-@@ git-compat-util.h: static inline int is_missing_file_error(int errno_)
-
- int cmd_main(int, const char **);
-
-+/**
-+ * The exit() function is defined as common_exit() in
-+ * git-compat-util.h.
-+ *
-+ * Intercepting the exit() allows us to hook in at-exit behavior, such
-+ * emitting trace2 logging before calling the real exit().
-+ */
-+int common_exit(const char *file, int line, int code);
-+
- /*
+@@ git-compat-util.h: 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)))
/*
2: 2d0527f86dc ! 2: 033f373acaa usage.c: add a non-fatal bug() function to go with BUG()
@@ Commit message
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
- 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>
## Documentation/technical/api-error-handling.txt ##
@@ Documentation/technical/api-error-handling.txt
+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.
@@ common-main.c: int main(int argc, const char **argv)
+{
+ 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()");
+}
+
+ /* We wrap exit() to call common_exit() in git-compat-util.h */
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;
@@ git-compat-util.h: static inline int regexec_buf(const regex_t *preg, const char
+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; \
++ if (bug_called_must_BUG) \
+ BUG_fl(__FILE__, __LINE__, __VA_ARGS__); \
-+ } \
+} while (0)
#ifdef __APPLE__
@@ t/helper/test-trace2.c: static int ut_007bug(int argc, const char **argv)
+ /* 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:
@@ t/helper/test-trace2.c: static struct unit_test ut_table[] = {
+ { ut_007BUG, "007bug", "" },
+ { ut_008bug, "008bug", "" },
+ { ut_009bug_BUG, "009bug_BUG","" },
++ { ut_010bug_BUG, "010bug_BUG","" },
};
/* clang-format on */
@@ t/t0210-trace2-normal.sh: test_expect_success 'BUG messages are written to trace
+ 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
@@ usage.c: void warning(const char *warn, ...)
if (in_bug)
abort();
-@@ usage.c: NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
+@@ usage.c: 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);
}
3: 4a7089fbbf2 = 3: 3d8a8e95f4d parse-options.c: use new bug() API for optbug()
4: 47d384d0ae5 ! 4: 58eafa2e04a 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");
- 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) {
++ else 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");
-- 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) {
+ if (opts->callback)
+- BUG("OPTION_LOWLEVEL_CALLBACK needs no high level 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. "
5: fe5c3926675 = 5: 05fb94aee6e receive-pack: use bug() and BUG_if_bug()
6: cbbe0276966 ! 6: 754a66be365 cache-tree.c: use bug() and BUG_if_bug()
@@ Commit message
set of messages via trace2, before this we'd only log the one BUG()
invocation.
+ We don't replace the last "BUG()" invocation with "BUG_if_bug()", as
+ in this case we're sure that we called bug() earlier, so there's no
+ need to make it a conditional.
+
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.
@@ cache-tree.c: struct tree* write_in_core_index_as_tree(struct repository *repo)
+ 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 inmemory index");
+ BUG("unmerged index entries when writing inmemory index");
}
-
- return lookup_tree(repo, &index_state->cache_tree->oid);
--
2.36.1.1103.gb3ecdfb3e6a
next prev parent reply other threads:[~2022-06-02 12:25 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-21 17:14 [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug() Ævar Arnfjörð Bjarmason
2022-05-21 17:14 ` [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
2022-05-25 20:55 ` Junio C Hamano
2022-05-26 4:17 ` Junio C Hamano
2022-05-26 7:59 ` Ævar Arnfjörð Bjarmason
2022-05-26 16:55 ` Junio C Hamano
2022-05-26 18:29 ` Ævar Arnfjörð Bjarmason
2022-05-26 18:55 ` Junio C Hamano
2022-05-31 16:52 ` Ævar Arnfjörð Bjarmason
2022-05-27 17:03 ` Josh Steadmon
2022-05-27 19:05 ` Junio C Hamano
2022-05-21 17:14 ` [PATCH 2/5] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
2022-05-27 17:03 ` Josh Steadmon
2022-05-21 17:14 ` [PATCH 3/5] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
2022-05-25 21:05 ` Junio C Hamano
2022-05-21 17:14 ` [PATCH 4/5] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
2022-05-25 21:15 ` Junio C Hamano
2022-05-27 17:04 ` Josh Steadmon
2022-05-27 22:53 ` Andrei Rybak
2022-05-21 17:14 ` [PATCH 5/5] cache-tree.c: " Ævar Arnfjörð Bjarmason
2022-05-27 21:29 ` Glen Choo
2022-05-27 17:02 ` [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug() Josh Steadmon
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
2022-06-01 18:37 ` Junio C Hamano
2022-05-31 16:58 ` [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
2022-05-31 18:18 ` Glen Choo
2022-06-01 18:50 ` Junio C Hamano
2022-05-31 16:58 ` [PATCH v2 3/6] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
2022-05-31 17:38 ` Josh Steadmon
2022-06-01 18:55 ` Junio C Hamano
2022-05-31 16:58 ` [PATCH v2 5/6] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
2022-06-01 18:52 ` Junio C Hamano
2022-05-31 17:39 ` [PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug() Josh Steadmon
2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason [this message]
2022-06-02 12:25 ` [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
2022-06-03 1:19 ` Junio C Hamano
2022-06-07 20:09 ` Josh Steadmon
2022-06-07 21:12 ` Ævar Arnfjörð Bjarmason
2022-06-07 22:05 ` Josh Steadmon
2022-06-02 12:25 ` [PATCH v3 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 3/6] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 4/6] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 5/6] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
2022-06-02 19:54 ` Junio C Hamano
2022-06-02 16:56 ` [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug() Glen Choo
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-v3-0.6-00000000000-20220602T122106Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=bagasdotme@gmail.com \
--cc=chakrabortyabhradeep79@gmail.com \
--cc=chooglen@google.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=rybak.a.v@gmail.com \
--cc=steadmon@google.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.
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 NNTP newsgroup(s).