All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug()
@ 2022-05-21 17:14 Æ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
                   ` (6 more replies)
  0 siblings, 7 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-21 17:14 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Ævar Arnfjörð Bjarmason

This series adds a bug() (lower-case) function to go along with
BUG(). As seen in 2-5/5 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.

I have more fixes for parse-options.c queued up on top of this
locally, including a fix for one (tiny) recent-ish regression, but
found that it was much easier to do so with this new API, as we'll now
be able to freely use normal sprintf() formats in these cases, instead
of xstrfmt() (where we'd also memory leak).

Ævar Arnfjörð Bjarmason (5):
  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          | 17 +++++-
 Documentation/technical/api-trace2.txt        |  4 +-
 builtin/receive-pack.c                        | 16 +++---
 cache-tree.c                                  |  7 ++-
 git-compat-util.h                             | 12 +++++
 parse-options.c                               | 50 +++++++++---------
 t/helper/test-trace2.c                        | 21 +++++++-
 t/t0210-trace2-normal.sh                      | 52 +++++++++++++++++++
 trace2.c                                      |  6 +++
 usage.c                                       | 30 +++++++++--
 10 files changed, 165 insertions(+), 50 deletions(-)

-- 
2.36.1.960.g7a4e2fc85c9


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  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 ` Ævar Arnfjörð Bjarmason
  2022-05-25 20:55   ` Junio C Hamano
  2022-05-27 17:03   ` Josh Steadmon
  2022-05-21 17:14 ` [PATCH 2/5] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-21 17:14 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Ævar Arnfjörð Bjarmason

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(-)

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)
 {
-- 
2.36.1.960.g7a4e2fc85c9


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH 2/5] parse-options.c: use new bug() API for optbug()
  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-21 17:14 ` Æ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
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-21 17:14 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Ævar Arnfjörð Bjarmason

When we run into bugs in parse-options.c usage it's good to be able to
note all the issues we ran into before dying. This use-case is why we
have the optbug() function introduced in 1e5ce570ca3 (parse-options:
clearer reporting of API misuse, 2010-12-02)

Let's change this code to use the new bug() API introduced in the
preceding commit, which cuts down on the verbosity of
parse_options_check().

There are existing uses of BUG() in adjacent code that should have
been using optbug() that aren't being changed here. That'll be done in
a subsequent commit. This only changes the optbug() callers.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 6e57744fd22..7fff588a45f 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,7 +470,7 @@ 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)
@@ -494,10 +493,9 @@ static void parse_options_check(const struct option *opts)
 		}
 		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();
 }
 
 static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
-- 
2.36.1.960.g7a4e2fc85c9


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH 3/5] parse-options.c: use optbug() instead of BUG() "opts" check
  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-21 17:14 ` [PATCH 2/5] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
@ 2022-05-21 17:14 ` Æ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
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-21 17:14 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Ævar Arnfjörð Bjarmason

Change the assertions added in bf3ff338a25 (parse-options: stop
abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
instead of BUG(). At this point we're looping over individual options,
so if we encounter any issues we'd like to report the offending option.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 7fff588a45f..5875936898f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -474,20 +474,20 @@ static void parse_options_check(const struct option *opts)
 			break;
 		case OPTION_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)
-				BUG("OPTION_CALLBACK can't have two callbacks");
+				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.");
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
-- 
2.36.1.960.g7a4e2fc85c9


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH 4/5] receive-pack: use bug() and BUG_if_bug()
  2022-05-21 17:14 [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug() Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2022-05-21 17:14 ` [PATCH 3/5] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
@ 2022-05-21 17:14 ` Ævar Arnfjörð Bjarmason
  2022-05-25 21:15   ` Junio C Hamano
                     ` (2 more replies)
  2022-05-21 17:14 ` [PATCH 5/5] cache-tree.c: " Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  6 siblings, 3 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-21 17:14 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Ævar Arnfjörð Bjarmason

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.

Let's also rename the warn_if_*() function that code is in to
BUG_if_*(), its name became outdated in b6a4788586d.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/receive-pack.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ad20b41e3c8..d1b3e5c419e 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();
 }
 
 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,
-- 
2.36.1.960.g7a4e2fc85c9


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH 5/5] cache-tree.c: use bug() and BUG_if_bug()
  2022-05-21 17:14 [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug() Ævar Arnfjörð Bjarmason
                   ` (3 preceding siblings ...)
  2022-05-21 17:14 ` [PATCH 4/5] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
@ 2022-05-21 17:14 ` Æ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
  6 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-21 17:14 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Ævar Arnfjörð Bjarmason

Change "BUG" output originally added in a97e4075a16 (Keep
rename/rename conflicts of intermediate merges while doing recursive
merge, 2007-03-31), and later made to say it was a "BUG" in
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().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 cache-tree.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 6752f69d515..9e96097500d 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -692,14 +692,13 @@ 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");
 		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("unmerged index entries when writing inmemory index");
+		BUG_if_bug();
 	}
 
 	return lookup_tree(repo, &index_state->cache_tree->oid);
-- 
2.36.1.960.g7a4e2fc85c9


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  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-27 17:03   ` Josh Steadmon
  1 sibling, 1 reply; 49+ messages in thread
From: Junio C Hamano @ 2022-05-25 20:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty

Æ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.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 3/5] parse-options.c: use optbug() instead of BUG() "opts" check
  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
  0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-05-25 21:05 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the assertions added in bf3ff338a25 (parse-options: stop
> abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
> instead of BUG(). At this point we're looping over individual options,
> so if we encounter any issues we'd like to report the offending option.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  parse-options.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 7fff588a45f..5875936898f 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -474,20 +474,20 @@ static void parse_options_check(const struct option *opts)
>  			break;
>  		case OPTION_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)
> -				BUG("OPTION_CALLBACK can't have two callbacks");
> +				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;

I wonder if we want to somehow warn developers against careless
conversion in the documentation.

For example, when rewriting BUG()s in the original

	if (!ptr)
		BUG("unexpected NULL in ptr");
	if (!strcmp(ptr, "(null)"))
		BUG("unexpected (null) in ptr");

into bug() followed by BUG_if_bug(), a mechanical rewrite

	if (!ptr)
		bug("unexpected NULL in ptr");
	if (!strcmp(ptr, "(null)"))
		bug("unexpected (null) in ptr");
	BUG_if_bug();

may not be correctly work, if evaluation of the latter condition is
not possible when an earlier condition holds true.  The code
structure (e.g. "if/if" -> "if/else if") might have to change in
some cases.  The conditions may have to be changed in other cases.


I think the changes made this in patch are good transformations to
use the bug()/BUG_if_bug(), of course.

>  		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.");
>  		default:
>  			; /* ok. (usually accepts an argument) */
>  		}

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 4/5] receive-pack: use bug() and BUG_if_bug()
  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
  2 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-05-25 21:15 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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.
>
> Let's also rename the warn_if_*() function that code is in to
> BUG_if_*(), its name became outdated in b6a4788586d.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/receive-pack.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index ad20b41e3c8..d1b3e5c419e 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);

OK.

>  	}
> -	if (!checked_connectivity)
> -		BUG("connectivity check skipped???");
> +	BUG_if_bug();

This is why I brought up the "shouldn't the concluding step allow
its own message?" earlier.  I suspect that the one in 5/5 shares the
same.

>  }
>  
>  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,

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  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
  0 siblings, 1 reply; 49+ messages in thread
From: Junio C Hamano @ 2022-05-26  4:17 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty

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?

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  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
  0 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-26  7:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Bagas Sanjaya, Abhradeep Chakraborty


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.




^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  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-31 16:52           ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-05-26 16:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty

Æ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.



^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  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
  1 sibling, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-26 18:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Bagas Sanjaya, Abhradeep Chakraborty


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.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  2022-05-26 18:29           ` Ævar Arnfjörð Bjarmason
@ 2022-05-26 18:55             ` Junio C Hamano
  0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-05-26 18:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty

Æ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.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug()
  2022-05-21 17:14 [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug() Ævar Arnfjörð Bjarmason
                   ` (4 preceding siblings ...)
  2022-05-21 17:14 ` [PATCH 5/5] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-05-27 17:02 ` Josh Steadmon
  2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
  6 siblings, 0 replies; 49+ messages in thread
From: Josh Steadmon @ 2022-05-27 17:02 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty

On 2022.05.21 19:14, Ævar Arnfjörð Bjarmason wrote:
> This series adds a bug() (lower-case) function to go along with
> BUG(). As seen in 2-5/5 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.
> 
> I have more fixes for parse-options.c queued up on top of this
> locally, including a fix for one (tiny) recent-ish regression, but
> found that it was much easier to do so with this new API, as we'll now
> be able to freely use normal sprintf() formats in these cases, instead
> of xstrfmt() (where we'd also memory leak).
> 
> Ævar Arnfjörð Bjarmason (5):
>   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          | 17 +++++-
>  Documentation/technical/api-trace2.txt        |  4 +-
>  builtin/receive-pack.c                        | 16 +++---
>  cache-tree.c                                  |  7 ++-
>  git-compat-util.h                             | 12 +++++
>  parse-options.c                               | 50 +++++++++---------
>  t/helper/test-trace2.c                        | 21 +++++++-
>  t/t0210-trace2-normal.sh                      | 52 +++++++++++++++++++
>  trace2.c                                      |  6 +++
>  usage.c                                       | 30 +++++++++--
>  10 files changed, 165 insertions(+), 50 deletions(-)
> 
> -- 
> 2.36.1.960.g7a4e2fc85c9
> 

Thank you for the series! I am generally in favor and think this is a
useful change. There are a few minor comments that I'll leave on
individual patches.


Thanks,
Josh

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  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-27 17:03   ` Josh Steadmon
  2022-05-27 19:05     ` Junio C Hamano
  1 sibling, 1 reply; 49+ messages in thread
From: Josh Steadmon @ 2022-05-27 17:03 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty

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?

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 2/5] parse-options.c: use new bug() API for optbug()
  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
  0 siblings, 0 replies; 49+ messages in thread
From: Josh Steadmon @ 2022-05-27 17:03 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty

On 2022.05.21 19:14, Ævar Arnfjörð Bjarmason wrote:
> When we run into bugs in parse-options.c usage it's good to be able to
> note all the issues we ran into before dying. This use-case is why we
> have the optbug() function introduced in 1e5ce570ca3 (parse-options:
> clearer reporting of API misuse, 2010-12-02)
> 
> Let's change this code to use the new bug() API introduced in the
> preceding commit, which cuts down on the verbosity of
> parse_options_check().
> 
> There are existing uses of BUG() in adjacent code that should have
> been using optbug() that aren't being changed here. That'll be done in
> a subsequent commit. This only changes the optbug() callers.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  parse-options.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 6e57744fd22..7fff588a45f 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,7 +470,7 @@ 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)
> @@ -494,10 +493,9 @@ static void parse_options_check(const struct option *opts)
>  		}
>  		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();

It might be worth noting in the commit message that this changes the
exit value in this case. On the other hand, it seems unlikely that
people are depending on this to detect programmer error that shouldn't
show up in the first place. I guess I don't feel too strongly about
this, it's probably fine to not mention it.


>  }
>  
>  static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
> -- 
> 2.36.1.960.g7a4e2fc85c9
> 

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 4/5] receive-pack: use bug() and BUG_if_bug()
  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
  2 siblings, 0 replies; 49+ messages in thread
From: Josh Steadmon @ 2022-05-27 17:04 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty

On 2022.05.21 19:14, Ævar Arnfjörð Bjarmason wrote:
> 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.
> 
> Let's also rename the warn_if_*() function that code is in to
> BUG_if_*(), its name became outdated in b6a4788586d.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/receive-pack.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index ad20b41e3c8..d1b3e5c419e 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])

Unless I'm missing something, the logic has flipped here:
- if(should_process_cmd(cmd) && ...
+ if(!should_process_cmd(cmd) && ...

Seems like a mistake, but if this is intentional could you please
describe in the commit message what it was intended to fix?


> +			bug("connectivity check has not been run on ref %s",
> +			    cmd->ref_name);
>  	}
> -	if (!checked_connectivity)
> -		BUG("connectivity check skipped???");
> +	BUG_if_bug();
>  }
>  
>  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,
> -- 
> 2.36.1.960.g7a4e2fc85c9
> 

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  2022-05-27 17:03   ` Josh Steadmon
@ 2022-05-27 19:05     ` Junio C Hamano
  0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-05-27 19:05 UTC (permalink / raw)
  To: Josh Steadmon
  Cc: Ævar Arnfjörð Bjarmason, git, Bagas Sanjaya,
	Abhradeep Chakraborty

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?


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 5/5] cache-tree.c: use bug() and BUG_if_bug()
  2022-05-21 17:14 ` [PATCH 5/5] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-05-27 21:29   ` Glen Choo
  0 siblings, 0 replies; 49+ messages in thread
From: Glen Choo @ 2022-05-27 21:29 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Ævar Arnfjörð Bjarmason

Capturing the discussion from Review Club :)

Ævar Arnfjörð Bjarmason         <avarab@gmail.com> writes:

> 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().

[...]

>  cache-tree.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 6752f69d515..9e96097500d 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -692,14 +692,13 @@ 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");
>  		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("unmerged index entries when writing inmemory index");
> +		BUG_if_bug();
>  	}
>  
>  	return lookup_tree(repo, &index_state->cache_tree->oid);

Once we hit this `if` block, we are already confident that we want to
BUG() out no matter what, so I think this could be equivalently
rewritten as:

  -		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\n", ce_stage(ce), (int)ce_namelen(ce), ce->name);
   		}
   		BUG("unmerged index entries when writing inmemory index");
   	}
   
   	return lookup_tree(repo, &index_state->cache_tree->oid);

And just musing here, like Junio mentioned upthread [1], BUG_if_bug() no
longer protects us the way BUG() does. This technically isn't an issue
here since we're confident we'll call bug() at least once, but the
reader has to do a bit more work to convince themselves that we will
never hit the return statement later on.

So in this case, we want to be aware of the previous bug() calls, but
don't really want the 'conditional dying' behavior of BUG_if_bug().
Maybe BUG() could learn about previous bug() messages, and we insist
that bug() be followed up by BUG_if_bug() or BUG().

[1] https://lore.kernel.org/git/xmqqk0a95nni.fsf@gitster.g

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 4/5] receive-pack: use bug() and BUG_if_bug()
  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
  2 siblings, 0 replies; 49+ messages in thread
From: Andrei Rybak @ 2022-05-27 22:53 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty

On 21/05/2022 19:14, Ævar Arnfjörð Bjarmason wrote:
> 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.

A verb is missing after "to". "to use the new bug() function", perhaps?

> 
> Let's also rename the warn_if_*() function that code is in to
> BUG_if_*(), its name became outdated in b6a4788586d.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()
  2022-05-26 16:55         ` Junio C Hamano
  2022-05-26 18:29           ` Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:52           ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Bagas Sanjaya, Abhradeep Chakraborty


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;



^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug()
  2022-05-21 17:14 [PATCH 0/5] usage API: add and use a bug() + BUG_if_bug() Ævar Arnfjörð Bjarmason
                   ` (5 preceding siblings ...)
  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 ` Æ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
                     ` (7 more replies)
  6 siblings, 8 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
  2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:58   ` Æ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
                     ` (6 subsequent siblings)
  7 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Change the exit() wrapper added in ee4512ed481 (trace2: create new
combined trace facility, 2019-02-22) so that we'll split up the trace2
logging concerns from wanting to wrap the "exit()" function itself for
other purposes.

This makes more sense structurally, as we won't seem to conflate
non-trace2 behavior with the trace2 code. I'd previously added an
explanation for this in 368b5843158 (common-main.c: call exit(), don't
return, 2021-12-07), that comment is being adjusted here.

Now the only thing we'll do if we're not using trace2 is to truncate
the "code" argument to the lowest 8 bits.

We only need to do that truncation on non-POSIX systems, but in
ee4512ed481 that "if defined(__MINGW32__)" code added in
47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
2009-07-05) was made to run everywhere. It might be good for clarify
to narrow that down by an "ifdef" again, but I'm not certain that in
the interim we haven't had some other non-POSIX systems rely the
behavior. On a POSIX system taking the lowest 8 bits is implicit, see
exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.

1. https://man7.org/linux/man-pages/man3/exit.3.html
2. https://man7.org/linux/man-pages/man2/wait.2.html

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 common-main.c     | 19 +++++++++++++++++--
 git-compat-util.h | 12 ++++++++++--
 trace2.c          |  8 ++------
 trace2.h          |  8 +-------
 4 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/common-main.c b/common-main.c
index 29fb7452f8a..bb0100f6024 100644
--- a/common-main.c
+++ b/common-main.c
@@ -56,9 +56,24 @@ 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);
 }
+
+int common_exit(const char *file, int line, int code)
+{
+	/*
+	 * 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".
+	 */
+	code &= 0xff;
+
+	trace2_cmd_exit_fl(file, line, code);
+
+	return code;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 96293b6c43a..a446a363de2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1447,12 +1447,20 @@ 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);
+
 /*
  * 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)))
+#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/trace2.c b/trace2.c
index e01cf77f1a8..0c0a11e07d5 100644
--- a/trace2.c
+++ b/trace2.c
@@ -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,
diff --git a/trace2.h b/trace2.h
index 1b109f57d0a..88d906ea830 100644
--- a/trace2.h
+++ b/trace2.h
@@ -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)))
 
-- 
2.36.1.1100.g16130010d07


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG()
  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-05-31 16:58   ` Æ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
                     ` (5 subsequent siblings)
  7 siblings, 2 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Add a bug() function to use in cases where we'd like to indicate a
runtime BUG(), but would like to defer 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() can follow up such calls with BUG_if_bug(),
which will BUG() out (i.e. abort()) if there were any preceding calls
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
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          | 19 ++++++-
 Documentation/technical/api-trace2.txt        |  4 +-
 common-main.c                                 | 11 ++++
 git-compat-util.h                             | 12 +++++
 t/helper/test-trace2.c                        | 22 +++++++-
 t/t0210-trace2-normal.sh                      | 52 +++++++++++++++++++
 usage.c                                       | 30 +++++++++--
 7 files changed, 140 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt
index 8be4f4d0d6a..c4131dc3a2c 100644
--- a/Documentation/technical/api-error-handling.txt
+++ b/Documentation/technical/api-error-handling.txt
@@ -1,12 +1,29 @@
 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.
++
+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 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/common-main.c b/common-main.c
index bb0100f6024..0e35243724d 100644
--- a/common-main.c
+++ b/common-main.c
@@ -63,6 +63,16 @@ 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)
 {
 	/*
@@ -73,6 +83,7 @@ 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;
diff --git a/git-compat-util.h b/git-compat-util.h
index a446a363de2..a80079a06fe 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1320,9 +1320,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__, __VA_ARGS__); \
+	} \
+} 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..97346554674 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,22 @@ 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;
+}
+
 /*
  * Usage:
  *     test-tool trace2 <ut_name_1> <ut_usage_1>
@@ -222,7 +238,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..1b63945cf91 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
+	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
+'
+
 sane_unset GIT_TRACE2_BRIEF
 
 # Now test without environment variables and get all Trace2 settings
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)
 {
-- 
2.36.1.1100.g16130010d07


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v2 3/6] parse-options.c: use new bug() API for optbug()
  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-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 16:58   ` Æ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
                     ` (4 subsequent siblings)
  7 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

When we run into bugs in parse-options.c usage it's good to be able to
note all the issues we ran into before dying. This use-case is why we
have the optbug() function introduced in 1e5ce570ca3 (parse-options:
clearer reporting of API misuse, 2010-12-02)

Let's change this code to use the new bug() API introduced in the
preceding commit, which cuts down on the verbosity of
parse_options_check().

There are existing uses of BUG() in adjacent code that should have
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 | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 6e57744fd22..78b46ae9698 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,7 +470,7 @@ 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)
@@ -494,10 +493,9 @@ static void parse_options_check(const struct option *opts)
 		}
 		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,
-- 
2.36.1.1100.g16130010d07


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
  2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  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   ` Ævar Arnfjörð Bjarmason
  2022-05-31 17:38     ` Josh Steadmon
  2022-05-31 16:58   ` [PATCH v2 5/6] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
                     ` (3 subsequent siblings)
  7 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Change the assertions added in bf3ff338a25 (parse-options: stop
abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
instead of BUG(). At this point we're looping over individual options,
so if we encounter any issues we'd like to report the offending option.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 78b46ae9698..243016ae30f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -473,21 +473,30 @@ 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)
-				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) {
+				optbug(opts, "OPTION_CALLBACK can't have two callbacks");
+				break;
+			}
 			break;
 		case OPTION_LOWLEVEL_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) {
+				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+				break;
+			}
 			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) */
 		}
-- 
2.36.1.1100.g16130010d07


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v2 5/6] receive-pack: use bug() and BUG_if_bug()
  2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  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 16:58   ` Ævar Arnfjörð Bjarmason
  2022-05-31 16:58   ` [PATCH v2 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

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 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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/receive-pack.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index daa153d0446..4a62327dee9 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,
-- 
2.36.1.1100.g16130010d07


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v2 6/6] cache-tree.c: use bug() and BUG_if_bug()
  2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  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   ` Æ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   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
  7 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Change "BUG" output originally added in a97e4075a16 (Keep
rename/rename conflicts of intermediate merges while doing recursive
merge, 2007-03-31), and later made to say it was a "BUG" in
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 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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 6752f69d515..b91995af602 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -692,14 +692,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 inmemory index");
 	}
 
 	return lookup_tree(repo, &index_state->cache_tree->oid);
-- 
2.36.1.1100.g16130010d07


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
  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
  0 siblings, 1 reply; 49+ messages in thread
From: Josh Steadmon @ 2022-05-31 17:38 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Glen Choo, Andrei Rybak, Emily Shaffer

On 2022.05.31 18:58, Ævar Arnfjörð Bjarmason wrote:
> Change the assertions added in bf3ff338a25 (parse-options: stop
> abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
> instead of BUG(). At this point we're looping over individual options,
> so if we encounter any issues we'd like to report the offending option.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  parse-options.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 78b46ae9698..243016ae30f 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -473,21 +473,30 @@ 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)
> -				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) {
> +				optbug(opts, "OPTION_CALLBACK can't have two callbacks");
> +				break;
> +			}
>  			break;
>  		case OPTION_LOWLEVEL_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) {
> +				optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
> +				break;
> +			}
>  			break;

A minor point, but I'm not sure I understand why we're adding breaks for
the two cases above. In the OPTION_CALLBACK case, the if conditions are
mutually exclusive and are followed by an unconditional break, so adding
additional breaks seems unnecessary. For the OPTION_LOWLEVEL_CALLBACK
case, the conditions are not mutually exclusive, but isn't this exactly
the issue that optbug() is intended to address? I.e., wouldn't the
caller want to see both optbug()s if both are relevant?

>  		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) */
>  		}
> -- 
> 2.36.1.1100.g16130010d07
> 

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug()
  2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
                     ` (5 preceding siblings ...)
  2022-05-31 16:58   ` [PATCH v2 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-05-31 17:39   ` Josh Steadmon
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
  7 siblings, 0 replies; 49+ messages in thread
From: Josh Steadmon @ 2022-05-31 17:39 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Glen Choo, Andrei Rybak, Emily Shaffer

On 2022.05.31 18:58, Ævar Arnfjörð Bjarmason wrote:
> 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(-)

Thanks for the updates! I have a minor question about 4/6, but
regardless of if/how that's addressed, I think this version looks good.

Reviewed-by: Josh Steadmon <steadmon@google.com>

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG()
  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
  1 sibling, 0 replies; 49+ messages in thread
From: Glen Choo @ 2022-05-31 18:18 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> +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()");
> +}

Hm thinking out loud here, what if we set bug_called_must_BUG = 0 inside
of BUG()? Then we'd be guaranteed that BUG() will never infinitely
recurse.

Another benefit is that we can invoke bug() and follow up with BUG() to
unconditionally exit without a bogus warning "had bug() call(s) without
explicit BUG_if_bug()". This pattern could be useful for 6/6 [1], where
we no longer exit after printing the "BUG:" messages.

Just an idea :)

[1] https://lore.kernel.org/git/patch-v2-6.6-cbbe0276966-20220531T164806Z-avarab@gmail.com

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
  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
  0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:37 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
	Glen Choo, Andrei Rybak, Emily Shaffer

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/common-main.c b/common-main.c
> index 29fb7452f8a..bb0100f6024 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -56,9 +56,24 @@ 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.
>  	 */

I was expecting to see this comment to speak in a more generic terms
(i.e. "after the main processing is done, we clean up after
ourselves, either by exiting cmd_main and coming here or explicitly
calling exit()"), now we have introduced common_exit().

>  	exit(result);
>  }
> +
> +int common_exit(const char *file, int line, int code)
> +{
> +	/*
> +	 * 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".
> +	 */
> +	code &= 0xff;

And I expected that the trace2 specific comment, if it is still
needed, would migrate here before this call.

Not huge enough to cause a reroll, but if there are other reasons to
see an updated version, it would be good to fix it while at it.

> +	trace2_cmd_exit_fl(file, line, code);
> +
> +	return code;
> +}

> +/**
> + * 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);

Nicely explained.

Thanks.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG()
  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
  1 sibling, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:50 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
	Glen Choo, Andrei Rybak, Emily Shaffer

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Unlike the code this replaces we'll log to trace2 with this new bug()
> function (as with other usage.c functions, including BUG()),

I do not terribly mind repeated fprintf(stderr, ...) or error() in a
loop, but this aspect of the change is probably among the two things
that make the series shine (the other obviously being bug_if() which
allows us to lose the "did we see any bug?" counter).

> 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(), 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.

OK.

> 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.

I think the last paragraph is an after-three-dash material.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v2 6/6] cache-tree.c: use bug() and BUG_if_bug()
  2022-05-31 16:58   ` [PATCH v2 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-06-01 18:52     ` Junio C Hamano
  0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:52 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
	Glen Choo, Andrei Rybak, Emily Shaffer

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change "BUG" output originally added in a97e4075a16 (Keep
> rename/rename conflicts of intermediate merges while doing recursive
> merge, 2007-03-31), and later made to say it was a "BUG" in
> 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 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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 6752f69d515..b91995af602 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -692,14 +692,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 inmemory index");
>  	}

I do not quite understand the last change here.  Shouldn't this be
either a BUG() that flushes previous bug() or BUG_if_bug() followed
by either a BUG() or an exit()?

>  
>  	return lookup_tree(repo, &index_state->cache_tree->oid);

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
  2022-05-31 17:38     ` Josh Steadmon
@ 2022-06-01 18:55       ` Junio C Hamano
  0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:55 UTC (permalink / raw)
  To: Josh Steadmon
  Cc: Ævar Arnfjörð Bjarmason, git, Bagas Sanjaya,
	Abhradeep Chakraborty, Glen Choo, Andrei Rybak, Emily Shaffer

Josh Steadmon <steadmon@google.com> writes:

>> +			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;
>
> A minor point, but I'm not sure I understand why we're adding breaks for
> the two cases above. In the OPTION_CALLBACK case, the if conditions are
> mutually exclusive and are followed by an unconditional break, so adding
> additional breaks seems unnecessary.

Yeah, good thinking.  

> For the OPTION_LOWLEVEL_CALLBACK
> case, the conditions are not mutually exclusive, but isn't this exactly
> the issue that optbug() is intended to address? I.e., wouldn't the
> caller want to see both optbug()s if both are relevant?

Exactly.

Thanks for a careful reading.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug()
  2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
                     ` (6 preceding siblings ...)
  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
  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
                       ` (6 more replies)
  7 siblings, 7 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
@ 2022-06-02 12:25     ` Ævar Arnfjörð Bjarmason
  2022-06-03  1:19       ` Junio C Hamano
  2022-06-07 20:09       ` 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
                       ` (5 subsequent siblings)
  6 siblings, 2 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Change the exit() wrapper added in ee4512ed481 (trace2: create new
combined trace facility, 2019-02-22) so that we'll split up the trace2
logging concerns from wanting to wrap the "exit()" function itself for
other purposes.

This makes more sense structurally, as we won't seem to conflate
non-trace2 behavior with the trace2 code. I'd previously added an
explanation for this in 368b5843158 (common-main.c: call exit(), don't
return, 2021-12-07), that comment is being adjusted here.

Now the only thing we'll do if we're not using trace2 is to truncate
the "code" argument to the lowest 8 bits.

We only need to do that truncation on non-POSIX systems, but in
ee4512ed481 that "if defined(__MINGW32__)" code added in
47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
2009-07-05) was made to run everywhere. It might be good for clarify
to narrow that down by an "ifdef" again, but I'm not certain that in
the interim we haven't had some other non-POSIX systems rely the
behavior. On a POSIX system taking the lowest 8 bits is implicit, see
exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.

1. https://man7.org/linux/man-pages/man3/exit.3.html
2. https://man7.org/linux/man-pages/man2/wait.2.html

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 common-main.c     | 20 ++++++++++++++++----
 git-compat-util.h |  4 ++--
 trace2.c          |  8 ++------
 trace2.h          |  8 +-------
 4 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/common-main.c b/common-main.c
index 29fb7452f8a..b6124dd2c2c 100644
--- a/common-main.c
+++ b/common-main.c
@@ -55,10 +55,22 @@ int main(int argc, const char **argv)
 
 	result = cmd_main(argc, argv);
 
+	/* 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;
 }
diff --git a/git-compat-util.h b/git-compat-util.h
index 96293b6c43a..1227ff80ca7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1451,8 +1451,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/trace2.c b/trace2.c
index e01cf77f1a8..0c0a11e07d5 100644
--- a/trace2.c
+++ b/trace2.c
@@ -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,
diff --git a/trace2.h b/trace2.h
index 1b109f57d0a..88d906ea830 100644
--- a/trace2.h
+++ b/trace2.h
@@ -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)))
 
-- 
2.36.1.1103.gb3ecdfb3e6a


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v3 2/6] usage.c: add a non-fatal bug() function to go with BUG()
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
  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-02 12:25     ` Æ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
                       ` (4 subsequent siblings)
  6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Add a bug() function to use in cases where we'd like to indicate a
runtime BUG(), but would like to defer 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() can follow up such calls with BUG_if_bug(),
which will BUG() out (i.e. abort()) if there were any preceding calls
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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .../technical/api-error-handling.txt          | 24 +++++-
 Documentation/technical/api-trace2.txt        |  4 +-
 common-main.c                                 |  8 ++
 git-compat-util.h                             | 10 +++
 t/helper/test-trace2.c                        | 29 ++++++-
 t/t0210-trace2-normal.sh                      | 76 +++++++++++++++++++
 usage.c                                       | 33 ++++++--
 7 files changed, 174 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt
index 8be4f4d0d6a..70bf1d3e522 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 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/common-main.c b/common-main.c
index b6124dd2c2c..c531372f3ff 100644
--- a/common-main.c
+++ b/common-main.c
@@ -59,6 +59,13 @@ int main(int argc, const char **argv)
 	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)
 {
@@ -70,6 +77,7 @@ 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;
diff --git a/git-compat-util.h b/git-compat-util.h
index 1227ff80ca7..ce007102f76 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1320,9 +1320,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)
 
 #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..180c7f53f31 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 37c359bd5a2..80e76a4695e 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
diff --git a/usage.c b/usage.c
index b738dd178b3..79900d0287f 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();
@@ -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)
 {
-- 
2.36.1.1103.gb3ecdfb3e6a


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v3 3/6] parse-options.c: use new bug() API for optbug()
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
  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-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     ` Æ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
                       ` (3 subsequent siblings)
  6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

When we run into bugs in parse-options.c usage it's good to be able to
note all the issues we ran into before dying. This use-case is why we
have the optbug() function introduced in 1e5ce570ca3 (parse-options:
clearer reporting of API misuse, 2010-12-02)

Let's change this code to use the new bug() API introduced in the
preceding commit, which cuts down on the verbosity of
parse_options_check().

There are existing uses of BUG() in adjacent code that should have
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 | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 6e57744fd22..78b46ae9698 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,7 +470,7 @@ 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)
@@ -494,10 +493,9 @@ static void parse_options_check(const struct option *opts)
 		}
 		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,
-- 
2.36.1.1103.gb3ecdfb3e6a


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v3 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (2 preceding siblings ...)
  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     ` Æ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
                       ` (2 subsequent siblings)
  6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Change the assertions added in bf3ff338a25 (parse-options: stop
abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
instead of BUG(). At this point we're looping over individual options,
so if we encounter any issues we'd like to report the offending option.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 parse-options.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 78b46ae9698..edf55d3ef5d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -474,20 +474,21 @@ static void parse_options_check(const struct option *opts)
 			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) */
 		}
-- 
2.36.1.1103.gb3ecdfb3e6a


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v3 5/6] receive-pack: use bug() and BUG_if_bug()
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (3 preceding siblings ...)
  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     ` Ævar Arnfjörð Bjarmason
  2022-06-02 12:25     ` [PATCH v3 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
  2022-06-02 16:56     ` [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug() Glen Choo
  6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

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 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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/receive-pack.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index daa153d0446..4a62327dee9 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,
-- 
2.36.1.1103.gb3ecdfb3e6a


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* [PATCH v3 6/6] cache-tree.c: use bug() and BUG_if_bug()
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (4 preceding siblings ...)
  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     ` Æ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
  6 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Change "BUG" output originally added in a97e4075a16 (Keep
rename/rename conflicts of intermediate merges while doing recursive
merge, 2007-03-31), and later made to say it was a "BUG" in
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 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.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 cache-tree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 6752f69d515..63953bf7772 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -692,12 +692,12 @@ 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");
 	}
-- 
2.36.1.1103.gb3ecdfb3e6a


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug()
  2022-06-02 12:25   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (5 preceding siblings ...)
  2022-06-02 12:25     ` [PATCH v3 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-06-02 16:56     ` Glen Choo
  6 siblings, 0 replies; 49+ messages in thread
From: Glen Choo @ 2022-06-02 16:56 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Josh Steadmon, Andrei Rybak, Emily Shaffer,
	Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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/

I didn't look too closely at patch 1/6 (since I'm not too familiar with
the trace2 stuff), but everything else looks great :)

(not sure if this is enough to give a Reviewed-By)

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v3 6/6] cache-tree.c: use bug() and BUG_if_bug()
  2022-06-02 12:25     ` [PATCH v3 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-06-02 19:54       ` Junio C Hamano
  0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-02 19:54 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
	Glen Choo, Andrei Rybak, Emily Shaffer

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change "BUG" output originally added in a97e4075a16 (Keep
> rename/rename conflicts of intermediate merges while doing recursive
> merge, 2007-03-31), and later made to say it was a "BUG" in
> 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 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.
>
> 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.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  cache-tree.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 6752f69d515..63953bf7772 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -692,12 +692,12 @@ 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");

"git grep inmemory" shows no hits on the non-word.  We say "in-core"
often, and it is even in the name of the function this new messages
is added ;-)

Will tweak as this is a new message.

Thanks.


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
  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
  1 sibling, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-03  1:19 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
	Glen Choo, Andrei Rybak, Emily Shaffer

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Subject: Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c

.o, not .c?

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
  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
  1 sibling, 1 reply; 49+ messages in thread
From: Josh Steadmon @ 2022-06-07 20:09 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Glen Choo, Andrei Rybak, Emily Shaffer

On 2022.06.02 14:25, Ævar Arnfjörð Bjarmason wrote:
> Change the exit() wrapper added in ee4512ed481 (trace2: create new
> combined trace facility, 2019-02-22) so that we'll split up the trace2
> logging concerns from wanting to wrap the "exit()" function itself for
> other purposes.
> 
> This makes more sense structurally, as we won't seem to conflate
> non-trace2 behavior with the trace2 code. I'd previously added an
> explanation for this in 368b5843158 (common-main.c: call exit(), don't
> return, 2021-12-07), that comment is being adjusted here.
> 
> Now the only thing we'll do if we're not using trace2 is to truncate
> the "code" argument to the lowest 8 bits.
> 
> We only need to do that truncation on non-POSIX systems, but in
> ee4512ed481 that "if defined(__MINGW32__)" code added in
> 47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
> 2009-07-05) was made to run everywhere. It might be good for clarify
> to narrow that down by an "ifdef" again, but I'm not certain that in
> the interim we haven't had some other non-POSIX systems rely the
> behavior. On a POSIX system taking the lowest 8 bits is implicit, see
> exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
> 
> 1. https://man7.org/linux/man-pages/man3/exit.3.html
> 2. https://man7.org/linux/man-pages/man2/wait.2.html
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  common-main.c     | 20 ++++++++++++++++----
>  git-compat-util.h |  4 ++--
>  trace2.c          |  8 ++------
>  trace2.h          |  8 +-------
>  4 files changed, 21 insertions(+), 19 deletions(-)

Just realized that this unexpectedly breaks the fuzzer builds:

$ git checkout ab/bug-if-bug
$ make CC=clang CXX=clang++ \
   CFLAGS="-fsanitize=fuzzer-no-link,address" \
   LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
   fuzz-all

[...]
    LINK fuzz-pack-headers
    LINK fuzz-pack-idx
/usr/bin/ld: archive.o: in function `parse_archive_args':
archive.c:(.text.parse_archive_args[parse_archive_args]+0x2cb8): undefined reference to `common_exit'
[...]


The issue is that we don't link the fuzzers against common-main.o
because the fuzzing engine provides its own main(). Would it be too much
of a pain to move this out to a new common-exit.c file?

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
  2022-06-07 20:09       ` Josh Steadmon
@ 2022-06-07 21:12         ` Ævar Arnfjörð Bjarmason
  2022-06-07 22:05           ` Josh Steadmon
  0 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-07 21:12 UTC (permalink / raw)
  To: Josh Steadmon
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Glen Choo, Andrei Rybak, Emily Shaffer


On Tue, Jun 07 2022, Josh Steadmon wrote:

> On 2022.06.02 14:25, Ævar Arnfjörð Bjarmason wrote:
>> Change the exit() wrapper added in ee4512ed481 (trace2: create new
>> combined trace facility, 2019-02-22) so that we'll split up the trace2
>> logging concerns from wanting to wrap the "exit()" function itself for
>> other purposes.
>> 
>> This makes more sense structurally, as we won't seem to conflate
>> non-trace2 behavior with the trace2 code. I'd previously added an
>> explanation for this in 368b5843158 (common-main.c: call exit(), don't
>> return, 2021-12-07), that comment is being adjusted here.
>> 
>> Now the only thing we'll do if we're not using trace2 is to truncate
>> the "code" argument to the lowest 8 bits.
>> 
>> We only need to do that truncation on non-POSIX systems, but in
>> ee4512ed481 that "if defined(__MINGW32__)" code added in
>> 47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
>> 2009-07-05) was made to run everywhere. It might be good for clarify
>> to narrow that down by an "ifdef" again, but I'm not certain that in
>> the interim we haven't had some other non-POSIX systems rely the
>> behavior. On a POSIX system taking the lowest 8 bits is implicit, see
>> exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
>> 
>> 1. https://man7.org/linux/man-pages/man3/exit.3.html
>> 2. https://man7.org/linux/man-pages/man2/wait.2.html
>> 
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  common-main.c     | 20 ++++++++++++++++----
>>  git-compat-util.h |  4 ++--
>>  trace2.c          |  8 ++------
>>  trace2.h          |  8 +-------
>>  4 files changed, 21 insertions(+), 19 deletions(-)
>
> Just realized that this unexpectedly breaks the fuzzer builds:
>
> $ git checkout ab/bug-if-bug
> $ make CC=clang CXX=clang++ \
>    CFLAGS="-fsanitize=fuzzer-no-link,address" \
>    LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
>    fuzz-all
>
> [...]
>     LINK fuzz-pack-headers
>     LINK fuzz-pack-idx
> /usr/bin/ld: archive.o: in function `parse_archive_args':
> archive.c:(.text.parse_archive_args[parse_archive_args]+0x2cb8): undefined reference to `common_exit'
> [...]

Hrm, that's a pain, sorry about that.

> The issue is that we don't link the fuzzers against common-main.o
> because the fuzzing engine provides its own main(). Would it be too much
> of a pain to move this out to a new common-exit.c file?

I could do that, I actually did that in a WIP copy, but figured it was
too much boilerplate to pass the list.

But why it is that we can't just link to common-main.o? Yeah the linker
error, but we already depend on modern clang here, so can't we just use
-Wl,--allow-multiple-definition?

This works for me with my local clang & GNU ld: 

    make CC=clang CXX=clang++ \
	CFLAGS="-fsanitize=fuzzer-no-link,address" \
        FUZZ_CXXFLAGS="-fsanitize=fuzzer-no-link,address -Wl,--allow-multiple-definition" \
	LIB_FUZZING_ENGINE="common-main.o -fsanitize=fuzzer" fuzz-all

It seems to me that the $(FUZZ_PROGRAMS) rule is mostly trying to work
around that by size, i.e. re-declaring most of $(OBJECTS).

But maybe it's a no-go for some reason, maybe due to OSX ld?


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
  2022-06-07 21:12         ` Ævar Arnfjörð Bjarmason
@ 2022-06-07 22:05           ` Josh Steadmon
  0 siblings, 0 replies; 49+ messages in thread
From: Josh Steadmon @ 2022-06-07 22:05 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
	Glen Choo, Andrei Rybak, Emily Shaffer

On 2022.06.07 23:12, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jun 07 2022, Josh Steadmon wrote:
> 
> > On 2022.06.02 14:25, Ævar Arnfjörð Bjarmason wrote:
> >> Change the exit() wrapper added in ee4512ed481 (trace2: create new
> >> combined trace facility, 2019-02-22) so that we'll split up the trace2
> >> logging concerns from wanting to wrap the "exit()" function itself for
> >> other purposes.
> >> 
> >> This makes more sense structurally, as we won't seem to conflate
> >> non-trace2 behavior with the trace2 code. I'd previously added an
> >> explanation for this in 368b5843158 (common-main.c: call exit(), don't
> >> return, 2021-12-07), that comment is being adjusted here.
> >> 
> >> Now the only thing we'll do if we're not using trace2 is to truncate
> >> the "code" argument to the lowest 8 bits.
> >> 
> >> We only need to do that truncation on non-POSIX systems, but in
> >> ee4512ed481 that "if defined(__MINGW32__)" code added in
> >> 47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
> >> 2009-07-05) was made to run everywhere. It might be good for clarify
> >> to narrow that down by an "ifdef" again, but I'm not certain that in
> >> the interim we haven't had some other non-POSIX systems rely the
> >> behavior. On a POSIX system taking the lowest 8 bits is implicit, see
> >> exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
> >> 
> >> 1. https://man7.org/linux/man-pages/man3/exit.3.html
> >> 2. https://man7.org/linux/man-pages/man2/wait.2.html
> >> 
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  common-main.c     | 20 ++++++++++++++++----
> >>  git-compat-util.h |  4 ++--
> >>  trace2.c          |  8 ++------
> >>  trace2.h          |  8 +-------
> >>  4 files changed, 21 insertions(+), 19 deletions(-)
> >
> > Just realized that this unexpectedly breaks the fuzzer builds:
> >
> > $ git checkout ab/bug-if-bug
> > $ make CC=clang CXX=clang++ \
> >    CFLAGS="-fsanitize=fuzzer-no-link,address" \
> >    LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
> >    fuzz-all
> >
> > [...]
> >     LINK fuzz-pack-headers
> >     LINK fuzz-pack-idx
> > /usr/bin/ld: archive.o: in function `parse_archive_args':
> > archive.c:(.text.parse_archive_args[parse_archive_args]+0x2cb8): undefined reference to `common_exit'
> > [...]
> 
> Hrm, that's a pain, sorry about that.
> 
> > The issue is that we don't link the fuzzers against common-main.o
> > because the fuzzing engine provides its own main(). Would it be too much
> > of a pain to move this out to a new common-exit.c file?
> 
> I could do that, I actually did that in a WIP copy, but figured it was
> too much boilerplate to pass the list.
> 
> But why it is that we can't just link to common-main.o? Yeah the linker
> error, but we already depend on modern clang here, so can't we just use
> -Wl,--allow-multiple-definition?
> 
> This works for me with my local clang & GNU ld: 
> 
>     make CC=clang CXX=clang++ \
> 	CFLAGS="-fsanitize=fuzzer-no-link,address" \
>         FUZZ_CXXFLAGS="-fsanitize=fuzzer-no-link,address -Wl,--allow-multiple-definition" \
> 	LIB_FUZZING_ENGINE="common-main.o -fsanitize=fuzzer" fuzz-all
> 
> It seems to me that the $(FUZZ_PROGRAMS) rule is mostly trying to work
> around that by size, i.e. re-declaring most of $(OBJECTS).
> 
> But maybe it's a no-go for some reason, maybe due to OSX ld?

Ah yeah, that would probably work. The main concern is that we want
these to run via OSS-Fuzz[1], but I believe I can send a change to our
build script there to add that flag. Thanks for the suggestion!

[1]: https://google.github.io/oss-fuzz/

^ permalink raw reply	[flat|nested] 49+ messages in thread

end of thread, other threads:[~2022-06-08  2:47 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.