[RFC PATCH 00/21] C99: show meaningful <file>:<line> in trace2 via macros

[RFC PATCH 00/21] C99: show meaningful <file>:<line> in trace2 via macros

[Ævar Arnfjörð Bjarmason]

Since everyone's getting in on the C99 fun.

Well, $subject and a bit more. This RFC series has bits and pieces
from thing I've submitted before. I'd proposed to make variadic macros
a hard dependency before in [1] because I wanted to get to the goal in
$subject, perhaps the whole thing will be more convincing.

This also includes the die_message() in a recent series of mine[2]
that I abandoned.

At the end of this series we expose a config variable to have
usage/die/warning emit line numbers. I.e. going from:

    $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
    fatal: bad boolean config value 'y' for 'core.x'

To:

    $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
    fatal: config.c:1241: bad boolean config value 'y' for 'core.x'

I find that to make tracing down errors in the test suite, and 21/21
has a GIT_TEST_* mode to turn it on there (which fails a lot now, but
I'm hoping I'll eventually get passing).

But most importantly we've now got meaningful file/line numbers in
trace2 error events. I.e. from all of them being some line in usage.c:
    
    $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
    {
      "event": "error",
      "sid": "20211115T221343.534151Z-Hc2f5b994-P003f3980",
      "thread": "main",
      "time": "2021-11-15T22:13:43.537981Z",
      "file": "usage.c",
      "line": 65,
      "msg": "bad boolean config value 'y' for 'core.x'",
      "fmt": "bad boolean config value '%s' for '%s'"
    }

To:
    
    $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
    {
      "event": "error",
      "sid": "20211115T221357.083824Z-Hc2f5b994-P003f4a82",
      "thread": "main",
      "time": "2021-11-15T22:13:57.087596Z",
      "file": "config.c",
      "line": 1241,
      "msg": "bad boolean config value 'y' for 'core.x'",
      "fmt": "bad boolean config value '%s' for '%s'"
    }

I've got some speculation in 19/21 that this may make the "fmt" part
redundant, i.e. did we only add that because we couldn't group these
by file/line, but as noted there there's still some use-cases for
"fmt" even with this series. In any case, this series doesn't touch
that "fmt" key at all.

This is "RFC" mainly because there's a CI failure in 0061.2 with this,
I still can't figure out what that's about (or if it's some fluke
unrelated to this topic), but that has to be investigated.

But I wanted to see if people found the general idea interesting
too. I picked the CC list mainly from paging through "--grep=trace2",
and people who'd modified the tricker bits of usage.c code being
modified here.

1. https://lore.kernel.org/git/cover-0.2-00000000000-20210412T105422Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com/
3. https://github.com/avar/git/runs/4216916706?check_suite_focus=true

Ævar Arnfjörð Bjarmason (21):
  git-compat-util.h: clarify GCC v.s. C99-specific in comment
  C99 support: hard-depend on C99 variadic macros
  usage.c: add a die_message() routine
  usage.c API users: use die_message() where appropriate
  usage.c + gc: add and use a die_message_errno()
  config API: don't use vreportf(), make it static in usage.c
  common-main.c: call exit(), don't return
  usage.c: add a non-fatal bug() function to go with BUG()
  parse-options.[ch] API: use bug() to improve error output
  receive-pack: use bug() and BUG_if_bug()
  cache-tree.c: use bug() and BUG_if_bug()
  pack-objects: use BUG(...) not die("BUG: ...")
  strbuf.h: use BUG(...) not die("BUG: ...")
  usage API: create a new usage.h, move API docs there
  usage.[ch] API users: use report_fn, not hardcoded prototype
  usage.[ch] API: rename "warn" vars functions to "warning"
  usage.c: move usage routines around
  usage.c: move rename variables in usage routines around
  usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()
  usage API: make the "{usage,fatal,error,warning,BUG}: " translatable
  usage API: add "core.usageAddSource" config to add <file>:<line>

 Documentation/CodingGuidelines                |   3 +
 Documentation/config/core.txt                 |   7 +
 .../technical/api-error-handling.txt          |  81 ------
 Documentation/technical/api-trace2.txt        |   4 +-
 apply.c                                       |   8 +-
 apply.h                                       |   6 +-
 banned.h                                      |   5 -
 builtin/fast-import.c                         |  22 +-
 builtin/gc.c                                  |  21 +-
 builtin/notes.c                               |  15 +-
 builtin/pack-objects.c                        |   2 +-
 builtin/receive-pack.c                        |  16 +-
 cache-tree.c                                  |   7 +-
 common-main.c                                 |   9 +-
 config.c                                      |  22 +-
 config.h                                      |  10 +-
 daemon.c                                      |   3 +-
 git-compat-util.h                             |  59 +---
 http-backend.c                                |   6 +-
 imap-send.c                                   |   3 +-
 parse-options.c                               |  56 ++--
 repo-settings.c                               |  11 +
 repository.h                                  |   2 +
 run-command.c                                 |  32 +--
 strbuf.h                                      |   2 +-
 t/helper/test-trace2.c                        |  27 +-
 t/t0210-trace2-normal.sh                      |  52 ++++
 trace.c                                       |  80 +-----
 trace.h                                       | 133 ++++-----
 trace2.c                                      |  45 +--
 trace2.h                                      |  28 --
 usage.c                                       | 270 +++++++++++-------
 usage.h                                       | 180 ++++++++++++
 33 files changed, 636 insertions(+), 591 deletions(-)
 delete mode 100644 Documentation/technical/api-error-handling.txt
 create mode 100644 usage.h

-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 01/21] git-compat-util.h: clarify GCC v.s. C99-specific in comment

[Ævar Arnfjörð Bjarmason]

Change a comment added in e208f9cc757 (make error()'s constant return
value more visible, 2012-12-15). It's not correct that this is GCC-ism
anymore, it's code that uses standard C99 features.

The comment being changed here pre-dates the HAVE_VARIADIC_MACROS
define, which we got in e05bed960d3 (trace: add 'file:line' to all
trace output, 2014-07-12).

The original implementation of an error() macro) in e208f9cc757 used a
GCC-ism with the paste operator (see the commit message for mention of
it), but that was dropped later by 9798f7e5f9 (Use __VA_ARGS__ for all
of error's arguments, 2013-02-08), giving us the C99-portable version
we have now.

While we could remove the __GNUC__ define here, it might cause issues
for other compilers or static analysis systems, so let's not. See
87fe5df365 (inline constant return from error() function, 2014-05-06)
for one such issue.

See also e05bed960d3 (trace: add 'file:line' to all trace output,
2014-07-12) for another comment about GNUC's handling of __VA_ARGS__.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-compat-util.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index d70ce142861..e86df9769ba 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -489,9 +489,7 @@ void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 /*
  * Let callers be aware of the constant return value; this can help
  * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
- * because some compilers may not support variadic macros. Since we're only
- * trying to help gcc, anyway, it's OK; other compilers will fall back to
- * using the function as usual.
+ * because other compilers may be confused by this.
  */
 #if defined(__GNUC__)
 static inline int const_error(void)
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 02/21] C99 support: hard-depend on C99 variadic macros

[Ævar Arnfjörð Bjarmason]

Remove the "else" branches of the HAVE_VARIADIC_MACROS macro, which
has been unconditionally set since 765dc168882 (git-compat-util:
always enable variadic macros, 2021-01-28).

That commit went out with v2.31.0, so we've got a few releases already
where we've been explicitly breaking things for compilers that don't
support this C99 feature.

In addition to that we've been emitting extremely verbose warnings
since at least ee4512ed481 (trace2: create new combined trace
facility, 2019-02-22) if HAVE_VARIADIC_MACROS wasn't true. There is no
such thing as a "region_enter_printf" or "region_leave_printf" format,
so at least under GCC and Clang everything that includes
trace.h (almost every file) emits a couple of warnings about that.

There's a large benefit to being able to rely on variadic macros, the
code surrounding usage.c is hard to maintain if we need to write two
implementations of everything, and by relying on "__FILE__" and
"__LINE__" along with "__VA_ARGS__" we can in the future make error(),
die() etc. log where they were called from, without the verbosity of a
dual-implementation.

Let's also update our CodingGuidelines to note that we depend on
this. The added bullet-point starts with lower-case for consistency
with other bullet-points in that section.

The diff in "trace.h" is relatively hard to read, since we need to
retain the existing API docs, which were comments on the code used if
HAVE_VARIADIC_MACROS was not defined.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/CodingGuidelines |   3 +
 banned.h                       |   5 --
 git-compat-util.h              |  12 ---
 trace.c                        |  80 +-------------------
 trace.h                        | 133 +++++++++++++--------------------
 trace2.c                       |  39 ----------
 trace2.h                       |  25 -------
 usage.c                        |  15 +---
 8 files changed, 58 insertions(+), 254 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 711cb9171e0..2c217c55502 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -210,6 +210,9 @@ For C programs:
    . since mid 2017 with 512f41cf, we have been using designated
      initializers for array (e.g. "int array[10] = { [5] = 2 }").
 
+   . since late 2021 with 765dc168882, we have been using variadic
+     macros, mostly for printf-like trace and debug macros.
+
    These used to be forbidden, but we have not heard any breakage
    report, and they are assumed to be safe.
 
diff --git a/banned.h b/banned.h
index 7ab4f2e4921..6ccf46bc197 100644
--- a/banned.h
+++ b/banned.h
@@ -21,13 +21,8 @@
 
 #undef sprintf
 #undef vsprintf
-#ifdef HAVE_VARIADIC_MACROS
 #define sprintf(...) BANNED(sprintf)
 #define vsprintf(...) BANNED(vsprintf)
-#else
-#define sprintf(buf,fmt,arg) BANNED(sprintf)
-#define vsprintf(buf,fmt,arg) BANNED(vsprintf)
-#endif
 
 #undef gmtime
 #define gmtime(t) BANNED(gmtime)
diff --git a/git-compat-util.h b/git-compat-util.h
index e86df9769ba..aec64261a96 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1193,24 +1193,12 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
 #endif
 #endif
 
-/*
- * This is always defined as a first step towards making the use of variadic
- * macros unconditional. If it causes compilation problems on your platform,
- * please report it to the Git mailing list at git@vger.kernel.org.
- */
-#define HAVE_VARIADIC_MACROS 1
-
 /* usage.c: only to be used for testing BUG() implementation (see test-tool) */
 extern int BUG_exit_code;
 
-#ifdef HAVE_VARIADIC_MACROS
 __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__)
-#else
-__attribute__((format (printf, 1, 2))) NORETURN
-void BUG(const char *fmt, ...);
-#endif
 
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
diff --git a/trace.c b/trace.c
index f726686fd92..794a087c21e 100644
--- a/trace.c
+++ b/trace.c
@@ -108,16 +108,11 @@ static int prepare_trace_line(const char *file, int line,
 	gettimeofday(&tv, NULL);
 	secs = tv.tv_sec;
 	localtime_r(&secs, &tm);
-	strbuf_addf(buf, "%02d:%02d:%02d.%06ld ", tm.tm_hour, tm.tm_min,
-		    tm.tm_sec, (long) tv.tv_usec);
-
-#ifdef HAVE_VARIADIC_MACROS
-	/* print file:line */
-	strbuf_addf(buf, "%s:%d ", file, line);
+	strbuf_addf(buf, "%02d:%02d:%02d.%06ld %s:%d", tm.tm_hour, tm.tm_min,
+		    tm.tm_sec, (long) tv.tv_usec, file, line);
 	/* align trace output (column 40 catches most files names in git) */
 	while (buf->len < 40)
 		strbuf_addch(buf, ' ');
-#endif
 
 	return 1;
 }
@@ -229,74 +224,6 @@ static void trace_performance_vprintf_fl(const char *file, int line,
 	strbuf_release(&buf);
 }
 
-#ifndef HAVE_VARIADIC_MACROS
-
-void trace_printf(const char *format, ...)
-{
-	va_list ap;
-	va_start(ap, format);
-	trace_vprintf_fl(NULL, 0, &trace_default_key, format, ap);
-	va_end(ap);
-}
-
-void trace_printf_key(struct trace_key *key, const char *format, ...)
-{
-	va_list ap;
-	va_start(ap, format);
-	trace_vprintf_fl(NULL, 0, key, format, ap);
-	va_end(ap);
-}
-
-void trace_argv_printf(const char **argv, const char *format, ...)
-{
-	va_list ap;
-	va_start(ap, format);
-	trace_argv_vprintf_fl(NULL, 0, argv, format, ap);
-	va_end(ap);
-}
-
-void trace_strbuf(struct trace_key *key, const struct strbuf *data)
-{
-	trace_strbuf_fl(NULL, 0, key, data);
-}
-
-void trace_performance(uint64_t nanos, const char *format, ...)
-{
-	va_list ap;
-	va_start(ap, format);
-	trace_performance_vprintf_fl(NULL, 0, nanos, format, ap);
-	va_end(ap);
-}
-
-void trace_performance_since(uint64_t start, const char *format, ...)
-{
-	va_list ap;
-	va_start(ap, format);
-	trace_performance_vprintf_fl(NULL, 0, getnanotime() - start,
-				     format, ap);
-	va_end(ap);
-}
-
-void trace_performance_leave(const char *format, ...)
-{
-	va_list ap;
-	uint64_t since;
-
-	if (perf_indent)
-		perf_indent--;
-
-	if (!format) /* Allow callers to leave without tracing anything */
-		return;
-
-	since = perf_start_times[perf_indent];
-	va_start(ap, format);
-	trace_performance_vprintf_fl(NULL, 0, getnanotime() - since,
-				     format, ap);
-	va_end(ap);
-}
-
-#else
-
 void trace_printf_key_fl(const char *file, int line, struct trace_key *key,
 			 const char *format, ...)
 {
@@ -342,9 +269,6 @@ void trace_performance_leave_fl(const char *file, int line,
 	va_end(ap);
 }
 
-#endif /* HAVE_VARIADIC_MACROS */
-
-
 static const char *quote_crnl(const char *path)
 {
 	static struct strbuf new_path = STRBUF_INIT;
diff --git a/trace.h b/trace.h
index e25984051aa..7ad2f03d07c 100644
--- a/trace.h
+++ b/trace.h
@@ -126,85 +126,10 @@ void trace_command_performance(const char **argv);
 void trace_verbatim(struct trace_key *key, const void *buf, unsigned len);
 uint64_t trace_performance_enter(void);
 
-#ifndef HAVE_VARIADIC_MACROS
-
-/**
- * Prints a formatted message, similar to printf.
- */
-__attribute__((format (printf, 1, 2)))
-void trace_printf(const char *format, ...);
-
-__attribute__((format (printf, 2, 3)))
-void trace_printf_key(struct trace_key *key, const char *format, ...);
-
-/**
- * Prints a formatted message, followed by a quoted list of arguments.
- */
-__attribute__((format (printf, 2, 3)))
-void trace_argv_printf(const char **argv, const char *format, ...);
-
-/**
- * Prints the strbuf, without additional formatting (i.e. doesn't
- * choke on `%` or even `\0`).
- */
-void trace_strbuf(struct trace_key *key, const struct strbuf *data);
-
 /**
- * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
+ * Macros to add the file:line of the calling code, instead of that of
+ * the trace function itself.
  *
- * Example:
- * ------------
- * uint64_t t = 0;
- * for (;;) {
- * 	// ignore
- * t -= getnanotime();
- * // code section to measure
- * t += getnanotime();
- * // ignore
- * }
- * trace_performance(t, "frotz");
- * ------------
- */
-__attribute__((format (printf, 2, 3)))
-void trace_performance(uint64_t nanos, const char *format, ...);
-
-/**
- * Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled.
- *
- * Example:
- * ------------
- * uint64_t start = getnanotime();
- * // code section to measure
- * trace_performance_since(start, "foobar");
- * ------------
- */
-__attribute__((format (printf, 2, 3)))
-void trace_performance_since(uint64_t start, const char *format, ...);
-
-__attribute__((format (printf, 1, 2)))
-void trace_performance_leave(const char *format, ...);
-
-#else
-
-/*
- * Macros to add file:line - see above for C-style declarations of how these
- * should be used.
- */
-
-/*
- * TRACE_CONTEXT may be set to __FUNCTION__ if the compiler supports it. The
- * default is __FILE__, as it is consistent with assert(), and static function
- * names are not necessarily unique.
- *
- * __FILE__ ":" __FUNCTION__ doesn't work with GNUC, as __FILE__ is supplied
- * by the preprocessor as a string literal, and __FUNCTION__ is filled in by
- * the compiler as a string constant.
- */
-#ifndef TRACE_CONTEXT
-# define TRACE_CONTEXT __FILE__
-#endif
-
-/*
  * Note: with C99 variadic macros, __VA_ARGS__ must include the last fixed
  * parameter ('format' in this case). Otherwise, a call without variable
  * arguments will have a surplus ','. E.g.:
@@ -218,7 +143,23 @@ void trace_performance_leave(const char *format, ...);
  *
  * which is invalid (note the ',)'). With GNUC, '##__VA_ARGS__' drops the
  * comma, but this is non-standard.
+ *
+ * TRACE_CONTEXT may be set to __FUNCTION__ if the compiler supports it. The
+ * default is __FILE__, as it is consistent with assert(), and static function
+ * names are not necessarily unique.
+ *
+ * __FILE__ ":" __FUNCTION__ doesn't work with GNUC, as __FILE__ is supplied
+ * by the preprocessor as a string literal, and __FUNCTION__ is filled in by
+ * the compiler as a string constant.
+ */
+#ifndef TRACE_CONTEXT
+# define TRACE_CONTEXT __FILE__
+#endif
+
+/**
+ * Prints a formatted message, similar to printf.
  */
+#define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)
 
 #define trace_printf_key(key, ...)					    \
 	do {								    \
@@ -227,8 +168,9 @@ void trace_performance_leave(const char *format, ...);
 					    __VA_ARGS__);		    \
 	} while (0)
 
-#define trace_printf(...) trace_printf_key(&trace_default_key, __VA_ARGS__)
-
+/**
+ * Prints a formatted message, followed by a quoted list of arguments.
+ */
 #define trace_argv_printf(argv, ...)					    \
 	do {								    \
 		if (trace_pass_fl(&trace_default_key))			    \
@@ -236,12 +178,32 @@ void trace_performance_leave(const char *format, ...);
 					    argv, __VA_ARGS__);		    \
 	} while (0)
 
+/**
+ * Prints the strbuf, without additional formatting (i.e. doesn't
+ * choke on `%` or even `\0`).
+ */
 #define trace_strbuf(key, data)						    \
 	do {								    \
 		if (trace_pass_fl(key))					    \
 			trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
 	} while (0)
 
+/**
+ * Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled.
+ *
+ * Example:
+ * ------------
+ * uint64_t t = 0;
+ * for (;;) {
+ * 	// ignore
+ * t -= getnanotime();
+ * // code section to measure
+ * t += getnanotime();
+ * // ignore
+ * }
+ * trace_performance(t, "frotz");
+ * ------------
+ */
 #define trace_performance(nanos, ...)					    \
 	do {								    \
 		if (trace_pass_fl(&trace_perf_key))			    \
@@ -249,6 +211,16 @@ void trace_performance_leave(const char *format, ...);
 					     __VA_ARGS__);		    \
 	} while (0)
 
+/**
+ * Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled.
+ *
+ * Example:
+ * ------------
+ * uint64_t start = getnanotime();
+ * // code section to measure
+ * trace_performance_since(start, "foobar");
+ * ------------
+ */
 #define trace_performance_since(start, ...)				    \
 	do {								    \
 		if (trace_pass_fl(&trace_perf_key))			    \
@@ -265,6 +237,7 @@ void trace_performance_leave(const char *format, ...);
 						   __VA_ARGS__);	    \
 	} while (0)
 
+
 /* backend functions, use non-*fl macros instead */
 __attribute__((format (printf, 4, 5)))
 void trace_printf_key_fl(const char *file, int line, struct trace_key *key,
@@ -285,6 +258,4 @@ static inline int trace_pass_fl(struct trace_key *key)
 	return key->fd || !key->initialized;
 }
 
-#endif /* HAVE_VARIADIC_MACROS */
-
 #endif /* TRACE_H */
diff --git a/trace2.c b/trace2.c
index b2d471526fd..179caa72cfe 100644
--- a/trace2.c
+++ b/trace2.c
@@ -641,20 +641,6 @@ void trace2_region_enter_printf_fl(const char *file, int line,
 	va_end(ap);
 }
 
-#ifndef HAVE_VARIADIC_MACROS
-void trace2_region_enter_printf(const char *category, const char *label,
-				const struct repository *repo, const char *fmt,
-				...)
-{
-	va_list ap;
-
-	va_start(ap, fmt);
-	trace2_region_enter_printf_va_fl(NULL, 0, category, label, repo, fmt,
-					 ap);
-	va_end(ap);
-}
-#endif
-
 void trace2_region_leave_printf_va_fl(const char *file, int line,
 				      const char *category, const char *label,
 				      const struct repository *repo,
@@ -717,20 +703,6 @@ void trace2_region_leave_printf_fl(const char *file, int line,
 	va_end(ap);
 }
 
-#ifndef HAVE_VARIADIC_MACROS
-void trace2_region_leave_printf(const char *category, const char *label,
-				const struct repository *repo, const char *fmt,
-				...)
-{
-	va_list ap;
-
-	va_start(ap, fmt);
-	trace2_region_leave_printf_va_fl(NULL, 0, category, label, repo, fmt,
-					 ap);
-	va_end(ap);
-}
-#endif
-
 void trace2_data_string_fl(const char *file, int line, const char *category,
 			   const struct repository *repo, const char *key,
 			   const char *value)
@@ -826,17 +798,6 @@ void trace2_printf_fl(const char *file, int line, const char *fmt, ...)
 	va_end(ap);
 }
 
-#ifndef HAVE_VARIADIC_MACROS
-void trace2_printf(const char *fmt, ...)
-{
-	va_list ap;
-
-	va_start(ap, fmt);
-	trace2_printf_va_fl(NULL, 0, fmt, ap);
-	va_end(ap);
-}
-#endif
-
 const char *trace2_session_id(void)
 {
 	return tr2_sid_get();
diff --git a/trace2.h b/trace2.h
index 0cc7b5f5312..1b109f57d0a 100644
--- a/trace2.h
+++ b/trace2.h
@@ -397,18 +397,9 @@ void trace2_region_enter_printf_fl(const char *file, int line,
 				   const struct repository *repo,
 				   const char *fmt, ...);
 
-#ifdef HAVE_VARIADIC_MACROS
 #define trace2_region_enter_printf(category, label, repo, ...)                 \
 	trace2_region_enter_printf_fl(__FILE__, __LINE__, (category), (label), \
 				      (repo), __VA_ARGS__)
-#else
-/* clang-format off */
-__attribute__((format (region_enter_printf, 4, 5)))
-void trace2_region_enter_printf(const char *category, const char *label,
-				const struct repository *repo, const char *fmt,
-				...);
-/* clang-format on */
-#endif
 
 /**
  * Emit a 'region_leave' event for <category>.<label> with optional
@@ -442,18 +433,9 @@ void trace2_region_leave_printf_fl(const char *file, int line,
 				   const struct repository *repo,
 				   const char *fmt, ...);
 
-#ifdef HAVE_VARIADIC_MACROS
 #define trace2_region_leave_printf(category, label, repo, ...)                 \
 	trace2_region_leave_printf_fl(__FILE__, __LINE__, (category), (label), \
 				      (repo), __VA_ARGS__)
-#else
-/* clang-format off */
-__attribute__((format (region_leave_printf, 4, 5)))
-void trace2_region_leave_printf(const char *category, const char *label,
-				const struct repository *repo, const char *fmt,
-				...);
-/* clang-format on */
-#endif
 
 /**
  * Emit a key-value pair 'data' event of the form <category>.<key> = <value>.
@@ -506,14 +488,7 @@ void trace2_printf_va_fl(const char *file, int line, const char *fmt,
 
 void trace2_printf_fl(const char *file, int line, const char *fmt, ...);
 
-#ifdef HAVE_VARIADIC_MACROS
 #define trace2_printf(...) trace2_printf_fl(__FILE__, __LINE__, __VA_ARGS__)
-#else
-/* clang-format off */
-__attribute__((format (printf, 1, 2)))
-void trace2_printf(const char *fmt, ...);
-/* clang-format on */
-#endif
 
 /*
  * Optional platform-specific code to dump information about the
diff --git a/usage.c b/usage.c
index c7d233b0de9..1128cf000d5 100644
--- a/usage.c
+++ b/usage.c
@@ -265,10 +265,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
 	va_copy(params_copy, params);
 
 	/* truncation via snprintf is OK here */
-	if (file)
-		snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
-	else
-		snprintf(prefix, sizeof(prefix), "BUG: ");
+	snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
 
 	vreportf(prefix, fmt, params);
 
@@ -283,7 +280,6 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
 	abort();
 }
 
-#ifdef HAVE_VARIADIC_MACROS
 NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
@@ -291,15 +287,6 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 	BUG_vfl(file, line, fmt, ap);
 	va_end(ap);
 }
-#else
-NORETURN void BUG(const char *fmt, ...)
-{
-	va_list ap;
-	va_start(ap, fmt);
-	BUG_vfl(NULL, 0, fmt, ap);
-	va_end(ap);
-}
-#endif
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS
 void unleak_memory(const void *ptr, size_t len)
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 03/21] usage.c: add a die_message() routine

[Ævar Arnfjörð Bjarmason]

We have code in various places that would like to call die(), but
wants to defer the exit(128) it would invoke, e.g. to print an
additional message, or adjust the exit code. Add a die_message()
helper routine to bridge this gap in the API.

Functionally this behaves just like the error() routine, except it'll
print a "fatal: " prefix, and it will exit with 128 instead of -1,
this is so that caller can pas the return value to exit(128), instead
of having to hardcode "128".

A subsequent commit will migrate various callers that benefit from
this function over to it, for now we're just adding the routine and
making die_builtin() in usage.c itself use it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-compat-util.h |  1 +
 usage.c           | 22 ++++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index aec64261a96..2c780825f9e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -471,6 +471,7 @@ NORETURN void usage(const char *err);
 NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
diff --git a/usage.c b/usage.c
index 1128cf000d5..f6a539cade9 100644
--- a/usage.c
+++ b/usage.c
@@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 	exit(129);
 }
 
+static void die_message_builtin(const char *err, va_list params)
+{
+	trace2_cmd_error_va(err, params);
+	vreportf("fatal: ", err, params);
+}
+
 /*
  * We call trace2_cmd_error_va() in the below functions first and
  * expect it to va_copy 'params' before using it (because an 'ap' can
@@ -62,10 +68,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
  */
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-	trace2_cmd_error_va(err, params);
-
-	vreportf("fatal: ", err, params);
-
+	die_message_builtin(err, params);
 	exit(128);
 }
 
@@ -211,6 +214,17 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
+#undef die_message
+int die_message(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	die_message_builtin(err, params);
+	va_end(params);
+	return 128;
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 04/21] usage.c API users: use die_message() where appropriate

[Ævar Arnfjörð Bjarmason]

Change code that either called error() and proceeded to exit with 128,
or emitted its own "fatal: " messages to use the die_message()
function added in a preceding commit.

In order to do that we need to add a get_die_message_routine()
function, which works like the other get_*_routine() functions in
usage.c. There is no set_die_message_rotine(), as it hasn't been
needed yet. We can add it if we ever need it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c | 12 +++++++-----
 builtin/notes.c       |  9 +++++----
 git-compat-util.h     |  1 +
 http-backend.c        |  3 ++-
 parse-options.c       |  2 +-
 run-command.c         | 16 +++++-----------
 usage.c               | 12 ++++++++++--
 7 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 20406f67754..2b2e28bad79 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -401,16 +401,18 @@ static void dump_marks(void);
 
 static NORETURN void die_nicely(const char *err, va_list params)
 {
+	va_list cp;
 	static int zombie;
-	char message[2 * PATH_MAX];
+	report_fn die_message_fn = get_die_message_routine();
 
-	vsnprintf(message, sizeof(message), err, params);
-	fputs("fatal: ", stderr);
-	fputs(message, stderr);
-	fputc('\n', stderr);
+	va_copy(cp, params);
+	die_message_fn(err, params);
 
 	if (!zombie) {
+		char message[2 * PATH_MAX];
+
 		zombie = 1;
+		vsnprintf(message, sizeof(message), err, cp);
 		write_crash_report(message);
 		end_packfile();
 		unkeep_all_packs();
diff --git a/builtin/notes.c b/builtin/notes.c
index 71c59583a17..2812d1eac40 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
 static void write_note_data(struct note_data *d, struct object_id *oid)
 {
 	if (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {
-		error(_("unable to write note object"));
+		int status = die_message(_("unable to write note object"));
+
 		if (d->edit_path)
-			error(_("the note contents have been left in %s"),
-				d->edit_path);
-		exit(128);
+			die_message(_("the note contents have been left in %s"),
+				    d->edit_path);
+		exit(status);
 	}
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 2c780825f9e..dd0170e3dd9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -504,6 +504,7 @@ static inline int const_error(void)
 typedef void (*report_fn)(const char *, va_list params);
 
 void set_die_routine(NORETURN_PTR report_fn routine);
+report_fn get_die_message_routine(void);
 void set_error_routine(report_fn routine);
 report_fn get_error_routine(void);
 void set_warn_routine(report_fn routine);
diff --git a/http-backend.c b/http-backend.c
index 3d6e2ff17f8..982cb62c7cb 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -659,8 +659,9 @@ static NORETURN void die_webcgi(const char *err, va_list params)
 {
 	if (dead <= 1) {
 		struct strbuf hdr = STRBUF_INIT;
+		report_fn die_message_fn = get_die_message_routine();
 
-		vreportf("fatal: ", err, params);
+		die_message_fn(err, params);
 
 		http_status(&hdr, 500, "Internal Server Error");
 		hdr_nocache(&hdr);
diff --git a/parse-options.c b/parse-options.c
index fc5b43ff0b2..8bc0a21f1d7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1075,6 +1075,6 @@ void NORETURN usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
 {
-	fprintf(stderr, "fatal: %s\n\n", msg);
+	die_message("%s\n", msg); /* The extra \n is intentional */
 	usage_with_options(usagestr, options);
 }
diff --git a/run-command.c b/run-command.c
index f40df01c772..a790fe9799d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])
 	child_close(fd[1]);
 }
 
-/*
- * parent will make it look like the child spewed a fatal error and died
- * this is needed to prevent changes to t0061.
- */
-static void fake_fatal(const char *err, va_list params)
-{
-	vreportf("fatal: ", err, params);
-}
-
 static void child_error_fn(const char *err, va_list params)
 {
 	const char msg[] = "error() should not be called in child\n";
@@ -372,9 +363,10 @@ static void NORETURN child_die_fn(const char *err, va_list params)
 static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
 {
 	static void (*old_errfn)(const char *err, va_list params);
+	report_fn die_message_routine = get_die_message_routine();
 
 	old_errfn = get_error_routine();
-	set_error_routine(fake_fatal);
+	set_error_routine(die_message_routine);
 	errno = cerr->syserr;
 
 	switch (cerr->err) {
@@ -1082,7 +1074,9 @@ static void *run_thread(void *data)
 
 static NORETURN void die_async(const char *err, va_list params)
 {
-	vreportf("fatal: ", err, params);
+	report_fn die_message_fn = get_die_message_routine();
+
+	die_message_fn(err, params);
 
 	if (in_async()) {
 		struct async *async = pthread_getspecific(async_key);
diff --git a/usage.c b/usage.c
index f6a539cade9..8ee5c6493fb 100644
--- a/usage.c
+++ b/usage.c
@@ -68,7 +68,9 @@ static void die_message_builtin(const char *err, va_list params)
  */
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-	die_message_builtin(err, params);
+	report_fn die_message_fn = get_die_message_routine();
+
+	die_message_fn(err, params);
 	exit(128);
 }
 
@@ -112,6 +114,7 @@ static int die_is_recursing_builtin(void)
  * (ugh), so keep things static. */
 static NORETURN_PTR report_fn usage_routine = usage_builtin;
 static NORETURN_PTR report_fn die_routine = die_builtin;
+static report_fn die_message_routine = die_message_builtin;
 static report_fn error_routine = error_builtin;
 static report_fn warn_routine = warn_builtin;
 static int (*die_is_recursing)(void) = die_is_recursing_builtin;
@@ -121,6 +124,11 @@ void set_die_routine(NORETURN_PTR report_fn routine)
 	die_routine = routine;
 }
 
+report_fn get_die_message_routine(void)
+{
+	return die_message_routine;
+}
+
 void set_error_routine(report_fn routine)
 {
 	error_routine = routine;
@@ -220,7 +228,7 @@ int die_message(const char *err, ...)
 	va_list params;
 
 	va_start(params, err);
-	die_message_builtin(err, params);
+	die_message_routine(err, params);
 	va_end(params);
 	return 128;
 }
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 05/21] usage.c + gc: add and use a die_message_errno()

[Ævar Arnfjörð Bjarmason]

Change code the "error: " output when we exit with 128 due to gc.log
errors to use a "fatal: " prefix instead. This adds a sibling function
to the die_errno() added in a preceding commit.

Since it returns 128 instead of -1 we'll need to adjust
report_last_gc_error(). Let's adjust it while we're at it to not
conflate the "should skip" and "exit with this non-zero code"
conditions, as the caller is no longer hardcoding "128", but relying
on die_errno() to return a nen-zero exit() status.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c      | 21 ++++++++++++---------
 git-compat-util.h |  1 +
 usage.c           | 12 ++++++++++++
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bcef6a4c8da..a70a935dab3 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
  * gc should not proceed due to an error in the last run. Prints a
  * message and returns -1 if an error occurred while reading gc.log
  */
-static int report_last_gc_error(void)
+static int report_last_gc_error(int *skip)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int ret = 0;
 	ssize_t len;
 	struct stat st;
 	char *gc_log_path = git_pathdup("gc.log");
+	*skip = 0;
 
 	if (stat(gc_log_path, &st)) {
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot stat '%s'"), gc_log_path);
 		goto done;
 	}
 
@@ -493,7 +494,7 @@ static int report_last_gc_error(void)
 
 	len = strbuf_read_file(&sb, gc_log_path, 0);
 	if (len < 0)
-		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot read '%s'"), gc_log_path);
 	else if (len > 0) {
 		/*
 		 * A previous gc failed.  Report the error, and don't
@@ -507,7 +508,7 @@ static int report_last_gc_error(void)
 			       "until the file is removed.\n\n"
 			       "%s"),
 			    gc_log_path, sb.buf);
-		ret = 1;
+		*skip = 1;
 	}
 	strbuf_release(&sb);
 done:
@@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
 		}
 		if (detach_auto) {
-			int ret = report_last_gc_error();
-			if (ret < 0)
-				/* an I/O error occurred, already reported */
-				exit(128);
-			if (ret == 1)
+			int skip;
+			int ret = report_last_gc_error(&skip);
+
+			if (skip)
 				/* Last gc --auto failed. Skip this one. */
 				return 0;
+			if (ret)
+				/* an error occurred, already reported */
+				exit(ret);
 
 			if (lock_repo_for_gc(force, &pid))
 				return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index dd0170e3dd9..d56f416af8a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -472,6 +472,7 @@ NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))
 NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
diff --git a/usage.c b/usage.c
index 8ee5c6493fb..a39d7aa330b 100644
--- a/usage.c
+++ b/usage.c
@@ -233,6 +233,18 @@ int die_message(const char *err, ...)
 	return 128;
 }
 
+#undef die_message_errno
+int die_message_errno(const char *fmt, ...)
+{
+	char buf[1024];
+	va_list params;
+
+	va_start(params, fmt);
+	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
+	va_end(params);
+	return -1;
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 06/21] config API: don't use vreportf(), make it static in usage.c

[Ævar Arnfjörð Bjarmason]

In preceding commits the rest of the vreportf() users outside of
usage.c have been migrated to die_message(), leaving only the
git_die_config() function added in 5a80e97c827 (config: add
`git_die_config()` to the config-set API, 2014-08-07).

Let's have its callers call error() themselves if they want to emit a
message, which is exactly what git_die_config() was doing for them
before emitting its own die() message.

This means that we can make the vreportf() in usage.c "static", and
only expose functions such as usage(), die(), warning() etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c |  7 ++++---
 builtin/notes.c       |  6 ++++--
 config.c              | 22 +++++++++-------------
 config.h              | 10 ++++++----
 git-compat-util.h     |  1 -
 imap-send.c           |  3 ++-
 usage.c               |  2 +-
 7 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 2b2e28bad79..4e2432bb491 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3456,9 +3456,10 @@ static void git_pack_config(void)
 	}
 	if (!git_config_get_int("pack.indexversion", &indexversion_value)) {
 		pack_idx_opts.version = indexversion_value;
-		if (pack_idx_opts.version > 2)
-			git_die_config("pack.indexversion",
-					"bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
+		if (pack_idx_opts.version > 2) {
+			error("bad pack.indexversion=%"PRIu32, pack_idx_opts.version);
+			git_die_config("pack.indexversion");
+		}
 	}
 	if (!git_config_get_ulong("pack.packsizelimit", &packsizelimit_value))
 		max_packsize = packsizelimit_value;
diff --git a/builtin/notes.c b/builtin/notes.c
index 2812d1eac40..60c5dab4122 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -763,8 +763,10 @@ static int git_config_get_notes_strategy(const char *key,
 
 	if (git_config_get_string(key, &value))
 		return 1;
-	if (parse_notes_merge_strategy(value, strategy))
-		git_die_config(key, _("unknown notes merge strategy %s"), value);
+	if (parse_notes_merge_strategy(value, strategy)) {
+		error(_("unknown notes merge strategy %s"), value);
+		git_die_config(key);
+	}
 
 	free(value);
 	return 0;
diff --git a/config.c b/config.c
index c5873f3a706..30f7971e0cc 100644
--- a/config.c
+++ b/config.c
@@ -2323,7 +2323,7 @@ int repo_config_get_string(struct repository *repo,
 	git_config_check_init(repo);
 	ret = git_configset_get_string(repo->config, key, dest);
 	if (ret < 0)
-		git_die_config(key, NULL);
+		git_die_config(key);
 	return ret;
 }
 
@@ -2334,7 +2334,7 @@ int repo_config_get_string_tmp(struct repository *repo,
 	git_config_check_init(repo);
 	ret = git_configset_get_string_tmp(repo->config, key, dest);
 	if (ret < 0)
-		git_die_config(key, NULL);
+		git_die_config(key);
 	return ret;
 }
 
@@ -2380,7 +2380,7 @@ int repo_config_get_pathname(struct repository *repo,
 	git_config_check_init(repo);
 	ret = git_configset_get_pathname(repo->config, key, dest);
 	if (ret < 0)
-		git_die_config(key, NULL);
+		git_die_config(key);
 	return ret;
 }
 
@@ -2452,8 +2452,10 @@ int git_config_get_expiry(const char *key, const char **output)
 		return ret;
 	if (strcmp(*output, "now")) {
 		timestamp_t now = approxidate("now");
-		if (approxidate(*output) >= now)
-			git_die_config(key, _("Invalid %s: '%s'"), key, *output);
+		if (approxidate(*output) >= now) {
+			error(_("Invalid %s: '%s'"), key, *output);
+			git_die_config(key);
+		}
 	}
 	return ret;
 }
@@ -2550,18 +2552,12 @@ void git_die_config_linenr(const char *key, const char *filename, int linenr)
 		    key, filename, linenr);
 }
 
-NORETURN __attribute__((format(printf, 2, 3)))
-void git_die_config(const char *key, const char *err, ...)
+NORETURN
+void git_die_config(const char *key)
 {
 	const struct string_list *values;
 	struct key_value_info *kv_info;
 
-	if (err) {
-		va_list params;
-		va_start(params, err);
-		vreportf("error: ", err, params);
-		va_end(params);
-	}
 	values = git_config_get_value_multi(key);
 	kv_info = values->items[values->nr - 1].util;
 	git_die_config_linenr(key, kv_info->filename, kv_info->linenr);
diff --git a/config.h b/config.h
index f119de01309..fae585d2005 100644
--- a/config.h
+++ b/config.h
@@ -626,11 +626,13 @@ struct key_value_info {
 };
 
 /**
- * First prints the error message specified by the caller in `err` and then
- * dies printing the line number and the file name of the highest priority
- * value for the configuration variable `key`.
+ * Dies printing the line number and the file name of the highest
+ * priority value for the configuration variable `key`.
+ *
+ * Consider calling error() first with a more specific formatted
+ * message of your own.
  */
-NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));
+NORETURN void git_die_config(const char *key);
 
 /**
  * Helper function which formats the die error message according to the
diff --git a/git-compat-util.h b/git-compat-util.h
index d56f416af8a..2f44aa86a8b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -466,7 +466,6 @@ static inline int git_has_dir_sep(const char *path)
 struct strbuf;
 
 /* General helper functions */
-void vreportf(const char *prefix, const char *err, va_list params);
 NORETURN void usage(const char *err);
 NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
diff --git a/imap-send.c b/imap-send.c
index e6090a0346a..0fdfe5159eb 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1350,7 +1350,8 @@ static int git_imap_config(const char *var, const char *val, void *cb)
 		server.port = git_config_int(var, val);
 	else if (!strcmp("imap.host", var)) {
 		if (!val) {
-			git_die_config("imap.host", "Missing value for 'imap.host'");
+			error("Missing value for 'imap.host'");
+			git_die_config("imap.host");
 		} else {
 			if (starts_with(val, "imap:"))
 				val += 5;
diff --git a/usage.c b/usage.c
index a39d7aa330b..43231c8eac0 100644
--- a/usage.c
+++ b/usage.c
@@ -6,7 +6,7 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
-void vreportf(const char *prefix, const char *err, va_list params)
+static void vreportf(const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 07/21] common-main.c: call exit(), don't return

[Ævar Arnfjörð Bjarmason]

Refactor the main() function so that we always take the same path
towards trace2_cmd_exit() whether exit() is invoked, or we end up in
the "return" in the pre-image. This contains no functional change, and
is only intended for the benefit of readers of the code, who'll now be
pointed to our exit() wrapper.

Since ee4512ed481 (trace2: create new combined trace facility,
2019-02-22) we've defined "exit" with a macro to call
trace2_cmd_exit() for us in "git-compat-util.h". So in cases where an
exit() is invoked (such as in several places in "git.c") we don't
reach the trace2_cmd_exit() in the pre-image. This makes it so that
we'll always take that same exit() path.

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

diff --git a/common-main.c b/common-main.c
index 71e21dd20a3..eafc70718a5 100644
--- a/common-main.c
+++ b/common-main.c
@@ -51,7 +51,10 @@ int main(int argc, const char **argv)
 
 	result = cmd_main(argc, argv);
 
-	trace2_cmd_exit(result);
-
-	return result;
+	/*
+	 * 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.
+	 */
+	exit(result);
 }
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 08/21] usage.c: add a non-fatal bug() function to go with BUG()

[Æ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. This will be used in place of optbug() in parse-options.c

Any caller to bug() must follow up such calls with BUG_if_bug(), and
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 I'll be converting existing
in-tree users to this, so hopefully this won't be controversial.

I'm not bothering to support the "else" branch of
"HAVE_VARIADIC_MACROS". Since 765dc168882 (git-compat-util: always
enable variadic macros, 2021-01-28) we've had a hard dependency on
them, and manually undefining the macro will nowadays result in a hard
compilation error. We should follow up with [2] instead and remove the
"else" codepath.

1. https://lore.kernel.org/git/YGRXomWwRYPdXZi3@coredump.intra.peff.net/
2. https://lore.kernel.org/git/cover-0.2-00000000000-20210412T105422Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .../technical/api-error-handling.txt          | 13 ++++-
 Documentation/technical/api-trace2.txt        |  4 +-
 git-compat-util.h                             | 12 +++++
 t/helper/test-trace2.c                        | 27 ++++++++--
 t/t0210-trace2-normal.sh                      | 52 +++++++++++++++++++
 trace2.c                                      |  6 +++
 usage.c                                       | 32 ++++++++++--
 7 files changed, 133 insertions(+), 13 deletions(-)

diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt
index 8be4f4d0d6a..818489bc3d4 100644
--- a/Documentation/technical/api-error-handling.txt
+++ b/Documentation/technical/api-error-handling.txt
@@ -1,7 +1,7 @@
 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,
@@ -18,6 +18,17 @@ various kinds.
   to the user and returns -1 for convenience in signaling the error
   to the caller.
 
+- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but
+  returns -1 like error. The user should then call `BUG_if_bug()` to die.
++
+This is for the convenience of APIs who'd like to potentially report
+more than one bug before calling `BUG_if_bug()`, which will invoke
+`BUG()` if there were any preceding calls to `bug()`.
++
+We call `BUG_if_bug()` ourselves in on `exit()` (via a wrapper, not
+`atexit()`), which guarantees that we'll catch cases where we forgot
+to invoke `BUG_if_bug()` following a call or calls to `bug()`.
+
 - `warning` is for reporting situations that probably should not
   occur but which the user (and Git) can continue to work around
   without running into too many problems.  Like `error`, it
diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
index bb13ca3db8b..68e221b95ab 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 2f44aa86a8b..9b02a3e05ba 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1198,9 +1198,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 must have a BUG() invocation 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)))
+int 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)
 
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index f93633f895a..84bc26fc20c 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -198,14 +198,29 @@ static int ut_006data(int argc, const char **argv)
 	return 0;
 }
 
-static int ut_007bug(int argc, const char **argv)
+/*
+ * Ensure that BUG() and bug() print to trace2.
+ */
+static int ut_007BUG(int argc, const char **argv)
 {
-	/*
-	 * Exercise BUG() to ensure that the message is printed to trace2.
-	 */
 	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 179caa72cfe..970e541a0d1 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 43231c8eac0..b411dfb5641 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,22 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 	va_end(ap);
 }
 
+int bug_called_must_BUG;
+int 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);
+
+	return -1;
+}
+
 #ifdef SUPPRESS_ANNOTATED_LEAKS
 void unleak_memory(const void *ptr, size_t len)
 {
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 09/21] parse-options.[ch] API: use bug() to improve error output

[Æ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, which is why we have the
optbug() function.

Let's instead use the bug() helper function that's newly added to
usage.c to do the same thing, which cuts down on the verbosity of
parse_options_check().

In addition change the use of BUG() in that function to bug(), we'll
be dying soon enough, but always want exhaustive error reporting from
the function.

Let's also use bug() instead of BUG() in preprocess_options() and
parse_options_start_1() (which is called shortly after
preprocess_options()). Since the BUG_if_bug() is called at the end of
parse_options_start_1() we won't miss it, and even if we did the
invocation in common-main.c would trigger.

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

diff --git a/parse-options.c b/parse-options.c
index 8bc0a21f1d7..54cbd382cb5 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)
@@ -440,28 +440,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:
@@ -470,22 +469,22 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
-				err |= optbug(opts, "should not accept an argument");
+				optbug(opts, "should not accept an argument");
 			break;
 		case OPTION_CALLBACK:
 			if (!opts->callback && !opts->ll_callback)
-				BUG("OPTION_CALLBACK needs one callback");
+				bug("OPTION_CALLBACK needs one callback");
 			if (opts->callback && opts->ll_callback)
-				BUG("OPTION_CALLBACK can't have two callbacks");
+				bug("OPTION_CALLBACK can't have two callbacks");
 			break;
 		case OPTION_LOWLEVEL_CALLBACK:
 			if (!opts->ll_callback)
-				BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
+				bug("OPTION_LOWLEVEL_CALLBACK needs a callback");
 			if (opts->callback)
-				BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+				bug("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
 			break;
 		case OPTION_ALIAS:
-			BUG("OPT_ALIAS() should not remain at this point. "
+			bug("OPT_ALIAS() should not remain at this point. "
 			    "Are you using parse_options_step() directly?\n"
 			    "That case is not supported yet.");
 		default:
@@ -493,10 +492,8 @@ 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);
 }
 
 static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
@@ -518,11 +515,12 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&
 	    !(flags & PARSE_OPT_ONE_SHOT))
-		BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+		bug("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
 	if ((flags & PARSE_OPT_ONE_SHOT) &&
 	    (flags & PARSE_OPT_KEEP_ARGV0))
-		BUG("Can't keep argv0 if you don't have it");
+		bug("Can't keep argv0 if you don't have it");
 	parse_options_check(options);
+	BUG_if_bug();
 }
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
@@ -673,7 +671,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 		source = newopt[i].value;
 
 		if (!long_name)
-			BUG("An alias must have long option name");
+			bug("An alias must have long option name");
 		strbuf_addf(&help, _("alias of --%s"), source);
 
 		for (j = 0; j < nr; j++) {
@@ -694,7 +692,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 		}
 
 		if (j == nr)
-			BUG("could not find source option '%s' of alias '%s'",
+			bug("could not find source option '%s' of alias '%s'",
 			    source, newopt[i].long_name);
 		ctx->alias_groups[alias * 3 + 0] = newopt[i].long_name;
 		ctx->alias_groups[alias * 3 + 1] = options[j].long_name;
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 10/21] receive-pack: use bug() and BUG_if_bug()

[Æ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 49b846d9605..42fabafa726 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1824,21 +1824,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,
@@ -2010,7 +2006,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.34.0.rc2.809.g11e21d44b24

[RFC PATCH 11/21] cache-tree.c: use bug() and BUG_if_bug()

[Æ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 79d168192d7..944e9709ae0 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.34.0.rc2.809.g11e21d44b24

[RFC PATCH 12/21] pack-objects: use BUG(...) not die("BUG: ...")

[Ævar Arnfjörð Bjarmason]

Change this code added in da93d12b004 (pack-objects: be incredibly
anal about stdio semantics, 2006-04-02) to use BUG() instead.

See 1a07e59c3e2 (Update messages in preparation for i18n, 2018-07-21)
for when the "BUG: " prefix was added, and [1] for background on the
Solaris behavior that prompted the exhaustive error checking in this
fgets() loop.

1. https://lore.kernel.org/git/824.1144007555@lotus.CS.Berkeley.EDU/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1a3dd445f83..0b1f82cd3ad 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3397,7 +3397,7 @@ static void read_object_list_from_stdin(void)
 			if (feof(stdin))
 				break;
 			if (!ferror(stdin))
-				die("BUG: fgets returned NULL, not EOF, not error!");
+				BUG("fgets returned NULL, not EOF, not error!");
 			if (errno != EINTR)
 				die_errno("fgets");
 			clearerr(stdin);
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 13/21] strbuf.h: use BUG(...) not die("BUG: ...")

[Ævar Arnfjörð Bjarmason]

In 7141efab248 (strbuf: clarify assertion in strbuf_setlen(),
2011-04-27) this 'die("BUG: "' invocation was added with the rationale
that strbuf.c had existing users doing the same, but those users were
later changed to use BUG() in 033abf97fcb (Replace all die("BUG: ...")
calls by BUG() ones, 2018-05-02). Let's do the same here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 96512f85b31..76965a17d44 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -160,7 +160,7 @@ void strbuf_grow(struct strbuf *sb, size_t amount);
 static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 {
 	if (len > (sb->alloc ? sb->alloc - 1 : 0))
-		die("BUG: strbuf_setlen() beyond buffer");
+		BUG("strbuf_setlen() beyond buffer");
 	sb->len = len;
 	if (sb->buf != strbuf_slopbuf)
 		sb->buf[len] = '\0';
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 14/21] usage API: create a new usage.h, move API docs there

[Ævar Arnfjörð Bjarmason]

Follow-up 26c816a67de (Merge branch 'hw/doc-in-header', 2019-12-16)
and move the API docs for the usage.c API to a new usage.h, which is
included from "git-compat-util.h".

All of the code and documentation is unchanged here, with the
following exceptions:

 * Added a short paragraph to the start of the comment in usage.h
   describing the API in general terms.

 * Prefixed comments with " * ", reformatted "+"-flowing lines away
   from that ASCIIDOC syntax.

 * Re-arranged the function definitions that were previously in
   "git-compat-util.h" to be grouped by task, and added short comments
   above each one. None of the code was changed.

 * There was an unrelated forward-declaration of "struct strbuf" that
   happened to be just above these usage functions. Move it down to just
   above "unlink_or_msg()" where it's needed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .../technical/api-error-handling.txt          |  92 ----------
 git-compat-util.h                             |  59 +------
 usage.h                                       | 161 ++++++++++++++++++
 3 files changed, 163 insertions(+), 149 deletions(-)
 delete mode 100644 Documentation/technical/api-error-handling.txt
 create mode 100644 usage.h

diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt
deleted file mode 100644
index 818489bc3d4..00000000000
--- a/Documentation/technical/api-error-handling.txt
+++ /dev/null
@@ -1,92 +0,0 @@
-Error reporting in git
-======================
-
-`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.
-
-- `die` is for fatal application errors.  It prints a message to
-  the user and exits with status 128.
-
-- `usage` is for errors in command line usage.  After printing its
-  message, it exits with status 129.  (See also `usage_with_options`
-  in the link:api-parse-options.html[parse-options API].)
-
-- `error` is for non-fatal library errors.  It prints a message
-  to the user and returns -1 for convenience in signaling the error
-  to the caller.
-
-- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but
-  returns -1 like error. The user should then call `BUG_if_bug()` to die.
-+
-This is for the convenience of APIs who'd like to potentially report
-more than one bug before calling `BUG_if_bug()`, which will invoke
-`BUG()` if there were any preceding calls to `bug()`.
-+
-We call `BUG_if_bug()` ourselves in on `exit()` (via a wrapper, not
-`atexit()`), which guarantees that we'll catch cases where we forgot
-to invoke `BUG_if_bug()` following a call or calls to `bug()`.
-
-- `warning` is for reporting situations that probably should not
-  occur but which the user (and Git) can continue to work around
-  without running into too many problems.  Like `error`, it
-  returns -1 after reporting the situation to the caller.
-
-These reports will be logged via the trace2 facility. See the "error"
-event in link:api-trace2.txt[trace2 API].
-
-Customizable error handlers
----------------------------
-
-The default behavior of `die` and `error` is to write a message to
-stderr and then exit or return as appropriate.  This behavior can be
-overridden using `set_die_routine` and `set_error_routine`.  For
-example, "git daemon" uses set_die_routine to write the reason `die`
-was called to syslog before exiting.
-
-Library errors
---------------
-
-Functions return a negative integer on error.  Details beyond that
-vary from function to function:
-
-- Some functions return -1 for all errors.  Others return a more
-  specific value depending on how the caller might want to react
-  to the error.
-
-- Some functions report the error to stderr with `error`,
-  while others leave that for the caller to do.
-
-- errno is not meaningful on return from most functions (except
-  for thin wrappers for system calls).
-
-Check the function's API documentation to be sure.
-
-Caller-handled errors
----------------------
-
-An increasing number of functions take a parameter 'struct strbuf *err'.
-On error, such functions append a message about what went wrong to the
-'err' strbuf.  The message is meant to be complete enough to be passed
-to `die` or `error` as-is.  For example:
-
-	if (ref_transaction_commit(transaction, &err))
-		die("%s", err.buf);
-
-The 'err' parameter will be untouched if no error occurred, so multiple
-function calls can be chained:
-
-	t = ref_transaction_begin(&err);
-	if (!t ||
-	    ref_transaction_update(t, "HEAD", ..., &err) ||
-	    ret_transaction_commit(t, &err))
-		die("%s", err.buf);
-
-The 'err' parameter must be a pointer to a valid strbuf.  To silence
-a message, pass a strbuf that is explicitly ignored:
-
-	if (thing_that_can_fail_in_an_ignorable_way(..., &err))
-		/* This failure is okay. */
-		strbuf_reset(&err);
diff --git a/git-compat-util.h b/git-compat-util.h
index 9b02a3e05ba..7a0b88a54c6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -462,20 +462,7 @@ static inline int git_has_dir_sep(const char *path)
 #include "compat/bswap.h"
 
 #include "wildmatch.h"
-
-struct strbuf;
-
-/* General helper functions */
-NORETURN void usage(const char *err);
-NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
-NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
-NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
-void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+#include "usage.h"
 
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
@@ -487,30 +474,6 @@ void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 #include <openssl/x509v3.h>
 #endif /* NO_OPENSSL */
 
-/*
- * Let callers be aware of the constant return value; this can help
- * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
- * because other compilers may be confused by this.
- */
-#if defined(__GNUC__)
-static inline int const_error(void)
-{
-	return -1;
-}
-#define error(...) (error(__VA_ARGS__), const_error())
-#define error_errno(...) (error_errno(__VA_ARGS__), const_error())
-#endif
-
-typedef void (*report_fn)(const char *, va_list params);
-
-void set_die_routine(NORETURN_PTR report_fn routine);
-report_fn get_die_message_routine(void);
-void set_error_routine(report_fn routine);
-report_fn get_error_routine(void);
-void set_warn_routine(report_fn routine);
-report_fn get_warn_routine(void);
-void set_die_is_recursing_routine(int (*routine)(void));
-
 int starts_with(const char *str, const char *prefix);
 int istarts_with(const char *str, const char *prefix);
 
@@ -1195,25 +1158,6 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
 #endif
 #endif
 
-/* 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 must have a BUG() invocation 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)))
-int 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)
-
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
  * Returns 0 on success, which includes trying to unlink an object that does
@@ -1226,6 +1170,7 @@ int unlink_or_warn(const char *path);
   * appends a message to err suitable for
   * 'error("%s", err->buf)' on error.
   */
+struct strbuf;
 int unlink_or_msg(const char *file, struct strbuf *err);
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
diff --git a/usage.h b/usage.h
new file mode 100644
index 00000000000..a2567a609fc
--- /dev/null
+++ b/usage.h
@@ -0,0 +1,161 @@
+#ifndef USAGE_H
+#define USAGE_H
+
+/**
+ * The usage.h is an API for error reporting in git, errors are
+ * reported both to the user, to Trace2 (see "trace2.h"), and possibly
+ * to custom callbacks via "report_fn" callbacks.
+ *
+ * `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.
+ *
+ * - `die` is for fatal application errors.  It prints a message to
+ *   the user and exits with status 128.
+ *
+ * - `usage` is for errors in command line usage.  After printing its
+ *   message, it exits with status 129.  (See also `usage_with_options`
+ *   in the link:api-parse-options.html[parse-options API].)
+ *
+ * - `error` is for non-fatal library errors.  It prints a message
+ *   to the user and returns -1 for convenience in signaling the error
+ *   to the caller.
+ *
+ * - `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but
+ *   returns -1 like error. The user should then call `BUG_if_bug()` to die.
+ *
+ *   This is for the convenience of APIs who'd like to potentially report
+ *   more than one bug before calling `BUG_if_bug()`, which will invoke
+ *   `BUG()` if there were any preceding calls to `bug()`.
+ *
+ *   We call `BUG_if_bug()` ourselves in on `exit()` (via a wrapper, not
+ *   `atexit()`), which guarantees that we'll catch cases where we forgot
+ *   to invoke `BUG_if_bug()` following a call or calls to `bug()`.
+ *
+ * - `warning` is for reporting situations that probably should not
+ *   occur but which the user (and Git) can continue to work around
+ *   without running into too many problems.  Like `error`, it
+ *   returns -1 after reporting the situation to the caller.
+ *
+ * These reports will be logged via the trace2 facility. See the "error"
+ * event in link:api-trace2.txt[trace2 API].
+ *
+ * Customizable error handlers
+ * ---------------------------
+ *
+ * The default behavior of `die` and `error` is to write a message to
+ * stderr and then exit or return as appropriate.  This behavior can be
+ * overridden using `set_die_routine` and `set_error_routine`.  For
+ * example, "git daemon" uses set_die_routine to write the reason `die`
+ * was called to syslog before exiting.
+ *
+ * Library errors
+ * --------------
+ *
+ * Functions return a negative integer on error.  Details beyond that
+ * vary from function to function:
+ *
+ * - Some functions return -1 for all errors.  Others return a more
+ *   specific value depending on how the caller might want to react
+ *   to the error.
+ *
+ * - Some functions report the error to stderr with `error`,
+ *   while others leave that for the caller to do.
+ *
+ * - errno is not meaningful on return from most functions (except
+ *   for thin wrappers for system calls).
+ *
+ * Check the function's API documentation to be sure.
+ *
+ * Caller-handled errors
+ * ---------------------
+ *
+ * An increasing number of functions take a parameter 'struct strbuf *err'.
+ * On error, such functions append a message about what went wrong to the
+ * 'err' strbuf.  The message is meant to be complete enough to be passed
+ * to `die` or `error` as-is.  For example:
+ *
+ * 	if (ref_transaction_commit(transaction, &err))
+ * 		die("%s", err.buf);
+ *
+ * The 'err' parameter will be untouched if no error occurred, so multiple
+ * function calls can be chained:
+ *
+ * 	t = ref_transaction_begin(&err);
+ * 	if (!t ||
+ * 	    ref_transaction_update(t, "HEAD", ..., &err) ||
+ * 	    ret_transaction_commit(t, &err))
+ * 		die("%s", err.buf);
+ *
+ * The 'err' parameter must be a pointer to a valid strbuf.  To silence
+ * a message, pass a strbuf that is explicitly ignored:
+ *
+ * 	if (thing_that_can_fail_in_an_ignorable_way(..., &err))
+ * 		// This failure is okay.
+ * 		strbuf_reset(&err);
+ */
+
+/**
+ * External but private variables, don't use these except for
+ * implementation details of this API itself.
+ */
+/* Only to be used for testing BUG() implementation (see test-tool) */
+extern int BUG_exit_code;
+/* If bug() is called we must have a BUG() invocation afterwards */
+extern int bug_called_must_BUG;
+
+/* General helper functions */
+NORETURN void usage(const char *err);
+NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
+NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
+NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
+void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+
+/* General helper functions invoked via macro wrappers */
+__attribute__((format (printf, 3, 4))) NORETURN
+void BUG_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4)))
+int bug_fl(const char *file, int line, const char *fmt, ...);
+
+/* General helper macros */
+#define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
+#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)
+
+/* Setting custom handling routines */
+typedef void (*report_fn)(const char *, va_list params);
+void set_die_routine(NORETURN_PTR report_fn routine);
+report_fn get_die_message_routine(void);
+void set_error_routine(report_fn routine);
+report_fn get_error_routine(void);
+void set_warn_routine(report_fn routine);
+report_fn get_warn_routine(void);
+void set_die_is_recursing_routine(int (*routine)(void));
+
+/*
+ * Let callers be aware of the constant return value; this can help
+ * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
+ * because other compilers may be confused by this.
+ */
+#if defined(__GNUC__)
+static inline int const_error(void)
+{
+	return -1;
+}
+#define error(...) (error(__VA_ARGS__), const_error())
+#define error_errno(...) (error_errno(__VA_ARGS__), const_error())
+#endif
+
+#endif
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 15/21] usage.[ch] API users: use report_fn, not hardcoded prototype

[Ævar Arnfjörð Bjarmason]

Change a couple of users of "report_fn" that hardcoded a definition of
it to use the definition of report_fn instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.h       | 4 ++--
 run-command.c | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/apply.h b/apply.h
index da3d95fa509..8dca3703d3b 100644
--- a/apply.h
+++ b/apply.h
@@ -106,8 +106,8 @@ struct apply_state {
 	 * set_error_routine() or set_warn_routine() to install muting
 	 * routines when in verbosity_silent mode.
 	 */
-	void (*saved_error_routine)(const char *err, va_list params);
-	void (*saved_warn_routine)(const char *warn, va_list params);
+	report_fn saved_error_routine;
+	report_fn saved_warn_routine;
 
 	/* These control whitespace errors */
 	enum apply_ws_error_action ws_error_action;
diff --git a/run-command.c b/run-command.c
index a790fe9799d..4792d170be7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -362,10 +362,9 @@ static void NORETURN child_die_fn(const char *err, va_list params)
 /* this runs in the parent process */
 static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
 {
-	static void (*old_errfn)(const char *err, va_list params);
+	report_fn old_errfn = get_error_routine();
 	report_fn die_message_routine = get_die_message_routine();
 
-	old_errfn = get_error_routine();
 	set_error_routine(die_message_routine);
 	errno = cerr->syserr;
 
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 16/21] usage.[ch] API: rename "warn" vars functions to "warning"

[Ævar Arnfjörð Bjarmason]

There is a warning() routine, not warn(), but parts of the function
interface confusingly used "warn". Let's rename these for consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c       |  6 +++---
 apply.h       |  4 ++--
 run-command.c |  8 ++++----
 usage.c       | 16 ++++++++--------
 usage.h       |  4 ++--
 5 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/apply.c b/apply.c
index 43a0aebf4ee..5c9872f8c10 100644
--- a/apply.c
+++ b/apply.c
@@ -160,9 +160,9 @@ int check_apply_state(struct apply_state *state, int force_apply)
 
 	if (state->apply_verbosity <= verbosity_silent) {
 		state->saved_error_routine = get_error_routine();
-		state->saved_warn_routine = get_warn_routine();
+		state->saved_warning_routine = get_warning_routine();
 		set_error_routine(mute_routine);
-		set_warn_routine(mute_routine);
+		set_warning_routine(mute_routine);
 	}
 
 	return 0;
@@ -4999,7 +4999,7 @@ int apply_all_patches(struct apply_state *state,
 
 	if (state->apply_verbosity <= verbosity_silent) {
 		set_error_routine(state->saved_error_routine);
-		set_warn_routine(state->saved_warn_routine);
+		set_warning_routine(state->saved_warning_routine);
 	}
 
 	if (res > -1)
diff --git a/apply.h b/apply.h
index 8dca3703d3b..496cd6f0083 100644
--- a/apply.h
+++ b/apply.h
@@ -103,11 +103,11 @@ struct apply_state {
 
 	/*
 	 * This is to save reporting routines before using
-	 * set_error_routine() or set_warn_routine() to install muting
+	 * set_error_routine() or set_warning_routine() to install muting
 	 * routines when in verbosity_silent mode.
 	 */
 	report_fn saved_error_routine;
-	report_fn saved_warn_routine;
+	report_fn saved_warning_routine;
 
 	/* These control whitespace errors */
 	enum apply_ws_error_action ws_error_action;
diff --git a/run-command.c b/run-command.c
index 4792d170be7..48b5fe19a80 100644
--- a/run-command.c
+++ b/run-command.c
@@ -346,9 +346,9 @@ static void child_error_fn(const char *err, va_list params)
 	xwrite(2, msg, sizeof(msg) - 1);
 }
 
-static void child_warn_fn(const char *err, va_list params)
+static void child_warning_fn(const char *err, va_list params)
 {
-	const char msg[] = "warn() should not be called in child\n";
+	const char msg[] = "warning() should not be called in child\n";
 	xwrite(2, msg, sizeof(msg) - 1);
 }
 
@@ -778,12 +778,12 @@ int start_command(struct child_process *cmd)
 	if (!cmd->pid) {
 		int sig;
 		/*
-		 * Ensure the default die/error/warn routines do not get
+		 * Ensure the default die/error/warning routines do not get
 		 * called, they can take stdio locks and malloc.
 		 */
 		set_die_routine(child_die_fn);
 		set_error_routine(child_error_fn);
-		set_warn_routine(child_warn_fn);
+		set_warning_routine(child_warning_fn);
 
 		close(notify_pipe[0]);
 		set_cloexec(notify_pipe[1]);
diff --git a/usage.c b/usage.c
index b411dfb5641..b41f8618f34 100644
--- a/usage.c
+++ b/usage.c
@@ -81,7 +81,7 @@ static void error_builtin(const char *err, va_list params)
 	vreportf("error: ", err, params);
 }
 
-static void warn_builtin(const char *warn, va_list params)
+static void warning_builtin(const char *warn, va_list params)
 {
 	trace2_cmd_error_va(warn, params);
 
@@ -116,7 +116,7 @@ static NORETURN_PTR report_fn usage_routine = usage_builtin;
 static NORETURN_PTR report_fn die_routine = die_builtin;
 static report_fn die_message_routine = die_message_builtin;
 static report_fn error_routine = error_builtin;
-static report_fn warn_routine = warn_builtin;
+static report_fn warning_routine = warning_builtin;
 static int (*die_is_recursing)(void) = die_is_recursing_builtin;
 
 void set_die_routine(NORETURN_PTR report_fn routine)
@@ -139,14 +139,14 @@ report_fn get_error_routine(void)
 	return error_routine;
 }
 
-void set_warn_routine(report_fn routine)
+void set_warning_routine(report_fn routine)
 {
-	warn_routine = routine;
+	warning_routine = routine;
 }
 
-report_fn get_warn_routine(void)
+report_fn get_warning_routine(void)
 {
-	return warn_routine;
+	return warning_routine;
 }
 
 void set_die_is_recursing_routine(int (*routine)(void))
@@ -274,7 +274,7 @@ void warning_errno(const char *warn, ...)
 	va_list params;
 
 	va_start(params, warn);
-	warn_routine(fmt_with_err(buf, sizeof(buf), warn), params);
+	warning_routine(fmt_with_err(buf, sizeof(buf), warn), params);
 	va_end(params);
 }
 
@@ -283,7 +283,7 @@ void warning(const char *warn, ...)
 	va_list params;
 
 	va_start(params, warn);
-	warn_routine(warn, params);
+	warning_routine(warn, params);
 	va_end(params);
 }
 
diff --git a/usage.h b/usage.h
index a2567a609fc..df02fe9bcaf 100644
--- a/usage.h
+++ b/usage.h
@@ -140,8 +140,8 @@ void set_die_routine(NORETURN_PTR report_fn routine);
 report_fn get_die_message_routine(void);
 void set_error_routine(report_fn routine);
 report_fn get_error_routine(void);
-void set_warn_routine(report_fn routine);
-report_fn get_warn_routine(void);
+void set_warning_routine(report_fn routine);
+report_fn get_warning_routine(void);
 void set_die_is_recursing_routine(int (*routine)(void));
 
 /*
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 17/21] usage.c: move usage routines around

[Ævar Arnfjörð Bjarmason]

A move-only change to re-order the usage routines in the order of
usage,die,die_message,error,warning, and to have the "errno" variant
after the non-errno variant, in addition to defining them all after
the fmt_with_err() helper which some of them need.

This change make a subsequent non-refactoring commit's diff smaller.

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

diff --git a/usage.c b/usage.c
index b41f8618f34..28005106f10 100644
--- a/usage.c
+++ b/usage.c
@@ -154,34 +154,6 @@ void set_die_is_recursing_routine(int (*routine)(void))
 	die_is_recursing = routine;
 }
 
-void NORETURN usagef(const char *err, ...)
-{
-	va_list params;
-
-	va_start(params, err);
-	usage_routine(err, params);
-	va_end(params);
-}
-
-void NORETURN usage(const char *err)
-{
-	usagef("%s", err);
-}
-
-void NORETURN die(const char *err, ...)
-{
-	va_list params;
-
-	if (die_is_recursing()) {
-		fputs("fatal: recursion detected in die handler\n", stderr);
-		exit(128);
-	}
-
-	va_start(params, err);
-	die_routine(err, params);
-	va_end(params);
-}
-
 static const char *fmt_with_err(char *buf, int n, const char *fmt)
 {
 	char str_error[256], *err;
@@ -206,6 +178,34 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt)
 	return buf;
 }
 
+void NORETURN usage(const char *err)
+{
+	usagef("%s", err);
+}
+
+void NORETURN usagef(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	usage_routine(err, params);
+	va_end(params);
+}
+
+void NORETURN die(const char *err, ...)
+{
+	va_list params;
+
+	if (die_is_recursing()) {
+		fputs("fatal: recursion detected in die handler\n", stderr);
+		exit(128);
+	}
+
+	va_start(params, err);
+	die_routine(err, params);
+	va_end(params);
+}
+
 void NORETURN die_errno(const char *fmt, ...)
 {
 	char buf[1024];
@@ -245,45 +245,45 @@ int die_message_errno(const char *fmt, ...)
 	return -1;
 }
 
-#undef error_errno
-int error_errno(const char *fmt, ...)
+#undef error
+int error(const char *err, ...)
 {
-	char buf[1024];
 	va_list params;
 
-	va_start(params, fmt);
-	error_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
+	va_start(params, err);
+	error_routine(err, params);
 	va_end(params);
 	return -1;
 }
 
-#undef error
-int error(const char *err, ...)
+#undef error_errno
+int error_errno(const char *fmt, ...)
 {
+	char buf[1024];
 	va_list params;
 
-	va_start(params, err);
-	error_routine(err, params);
+	va_start(params, fmt);
+	error_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
 	va_end(params);
 	return -1;
 }
 
-void warning_errno(const char *warn, ...)
+void warning(const char *warn, ...)
 {
-	char buf[1024];
 	va_list params;
 
 	va_start(params, warn);
-	warning_routine(fmt_with_err(buf, sizeof(buf), warn), params);
+	warning_routine(warn, params);
 	va_end(params);
 }
 
-void warning(const char *warn, ...)
+void warning_errno(const char *warn, ...)
 {
+	char buf[1024];
 	va_list params;
 
 	va_start(params, warn);
-	warning_routine(warn, params);
+	warning_routine(fmt_with_err(buf, sizeof(buf), warn), params);
 	va_end(params);
 }
 
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 18/21] usage.c: move rename variables in usage routines around

[Ævar Arnfjörð Bjarmason]

A renaming-only change to rename variables in the usage routines to be
consistent. Before we'd use "params", now we use "ap", and the mixture
of "fmt", "err", "warn" etc. is replaced with just "fmt".

This change make a subsequent non-refactoring commit's diff smaller
and easier to understand.

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

diff --git a/usage.c b/usage.c
index 28005106f10..4b93744137d 100644
--- a/usage.c
+++ b/usage.c
@@ -178,38 +178,38 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt)
 	return buf;
 }
 
-void NORETURN usage(const char *err)
+void NORETURN usage(const char *fmt)
 {
-	usagef("%s", err);
+	usagef("%s", fmt);
 }
 
-void NORETURN usagef(const char *err, ...)
+void NORETURN usagef(const char *fmt, ...)
 {
-	va_list params;
+	va_list ap;
 
-	va_start(params, err);
-	usage_routine(err, params);
-	va_end(params);
+	va_start(ap, fmt);
+	usage_routine(fmt, ap);
+	va_end(ap);
 }
 
-void NORETURN die(const char *err, ...)
+void NORETURN die(const char *fmt, ...)
 {
-	va_list params;
+	va_list ap;
 
 	if (die_is_recursing()) {
 		fputs("fatal: recursion detected in die handler\n", stderr);
 		exit(128);
 	}
 
-	va_start(params, err);
-	die_routine(err, params);
-	va_end(params);
+	va_start(ap, fmt);
+	die_routine(fmt, ap);
+	va_end(ap);
 }
 
 void NORETURN die_errno(const char *fmt, ...)
 {
 	char buf[1024];
-	va_list params;
+	va_list ap;
 
 	if (die_is_recursing()) {
 		fputs("fatal: recursion detected in die_errno handler\n",
@@ -217,19 +217,19 @@ void NORETURN die_errno(const char *fmt, ...)
 		exit(128);
 	}
 
-	va_start(params, fmt);
-	die_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
-	va_end(params);
+	va_start(ap, fmt);
+	die_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	va_end(ap);
 }
 
 #undef die_message
-int die_message(const char *err, ...)
+int die_message(const char *fmt, ...)
 {
-	va_list params;
+	va_list ap;
 
-	va_start(params, err);
-	die_message_routine(err, params);
-	va_end(params);
+	va_start(ap, fmt);
+	die_message_routine(fmt, ap);
+	va_end(ap);
 	return 128;
 }
 
@@ -237,22 +237,22 @@ int die_message(const char *err, ...)
 int die_message_errno(const char *fmt, ...)
 {
 	char buf[1024];
-	va_list params;
+	va_list ap;
 
-	va_start(params, fmt);
-	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
-	va_end(params);
+	va_start(ap, fmt);
+	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	va_end(ap);
 	return -1;
 }
 
 #undef error
-int error(const char *err, ...)
+int error(const char *fmt, ...)
 {
-	va_list params;
+	va_list ap;
 
-	va_start(params, err);
-	error_routine(err, params);
-	va_end(params);
+	va_start(ap, fmt);
+	error_routine(fmt, ap);
+	va_end(ap);
 	return -1;
 }
 
@@ -260,31 +260,31 @@ int error(const char *err, ...)
 int error_errno(const char *fmt, ...)
 {
 	char buf[1024];
-	va_list params;
+	va_list ap;
 
-	va_start(params, fmt);
-	error_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
-	va_end(params);
+	va_start(ap, fmt);
+	error_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	va_end(ap);
 	return -1;
 }
 
-void warning(const char *warn, ...)
+void warning(const char *fmt, ...)
 {
-	va_list params;
+	va_list ap;
 
-	va_start(params, warn);
-	warning_routine(warn, params);
-	va_end(params);
+	va_start(ap, fmt);
+	warning_routine(fmt, ap);
+	va_end(ap);
 }
 
-void warning_errno(const char *warn, ...)
+void warning_errno(const char *fmt, ...)
 {
 	char buf[1024];
-	va_list params;
+	va_list ap;
 
-	va_start(params, warn);
-	warning_routine(fmt_with_err(buf, sizeof(buf), warn), params);
-	va_end(params);
+	va_start(ap, fmt);
+	warning_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	va_end(ap);
 }
 
 /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Ævar Arnfjörð Bjarmason]

Change the "usage" family of functions to be defined in terms of C99
variadic macros, as we've optionally done with the BUG() macro and
BUG_fl() function since d8193743e08 (usage.c: add BUG() function,
2017-05-12), and unconditionally since 765dc168882 (git-compat-util:
always enable variadic macros, 2021-01-28).

This would have been possible before having a hard dependency on C99,
but as the dual implementations of C99 and pre-C99 macros and
functions adjusted in preceding commits show, doing so would have been
rather painful.

By having these be macros we'll now log meaningful "file" and "line"
entries in trace2 events. Before this we'd log "usage.c" in all of
them, and the line would be the relevant locations in that file.

To do this we need to not only introduce X_fl() functions for
{die,error,warning,die}{,_errno}(), but also change all the callers of
the set_*() and get_() functions in usage.h to take "file" and "line"
arguments.

Neither the built-in {die,error,warning,die}{,_errno}() nor anyone
else does anything useful with these "file" and "line" arguments for
now, but it means we can define all our macros and functions
consistently.

It also opens the door for a follow-up change where these functions
could optionally emit the file name and line number, e.g. for
DEVELOPER=1 builds, or depending on configuration.

It might be a good change to remove the "fmt" key from the "error"
events as a follow-up change. As these few examples from running the
test suite show it's sometimes redundant (same as the "msg"), rather
useless (just a "%s"), or something we could now mostly aggregate by
file/line instead of the normalized printf format:

      1 file":"builtin/gc.c","line":1391,"msg":"'bogus' is not a valid task","fmt":"'%s' is not a valid task"}
      1 file":"builtin/for-each-ref.c","line":89,"msg":"format: %(then) atom used more than once","fmt":"%s"}
      1 file":"builtin/fast-import.c","line":411,"msg":"Garbage after mark: N :202 :302x","fmt":"Garbage after mark: %s"}

"Mostly" here assumes that it would be OK if the aggregation changed
between git versions, which may be what all users of trace2 want. The
change that introduced the "fmt" code was ee4512ed481 (trace2: create
new combined trace facility, 2019-02-22), and the documentation change
was e544221d97a (trace2: Documentation/technical/api-trace2.txt,
2019-02-22).

Both are rather vague on what problem "fmt" solved exactly, aside from
the obvious one of it being impossible to do meaningful aggregations
due to the "file" and "line" being the same everywhere, which isn't
the case now.

In any case, let's leave "fmt" be for now, the above summary was given
in case it's interesting to remove it in the future, e.g. to save
space in trace2 payloads.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c               |  2 +-
 builtin/fast-import.c |  5 +--
 daemon.c              |  3 +-
 http-backend.c        |  5 +--
 run-command.c         | 15 ++++----
 trace2.h              |  3 --
 usage.c               | 79 ++++++++++++++++++++-----------------------
 usage.h               | 49 +++++++++++++++++++--------
 8 files changed, 87 insertions(+), 74 deletions(-)

diff --git a/apply.c b/apply.c
index 5c9872f8c10..1a505a2e097 100644
--- a/apply.c
+++ b/apply.c
@@ -123,7 +123,7 @@ void clear_apply_state(struct apply_state *state)
 	/* &state->fn_table is cleared at the end of apply_patch() */
 }
 
-static void mute_routine(const char *msg, va_list params)
+static void mute_routine(const char *file, int line, const char *msg, va_list params)
 {
 	/* do nothing */
 }
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 4e2432bb491..687c7194842 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -399,14 +399,15 @@ static void end_packfile(void);
 static void unkeep_all_packs(void);
 static void dump_marks(void);
 
-static NORETURN void die_nicely(const char *err, va_list params)
+static NORETURN void die_nicely(const char *file, int line, const char *err,
+				va_list params)
 {
 	va_list cp;
 	static int zombie;
 	report_fn die_message_fn = get_die_message_routine();
 
 	va_copy(cp, params);
-	die_message_fn(err, params);
+	die_message_fn(__FILE__, __LINE__, err, params);
 
 	if (!zombie) {
 		char message[2 * PATH_MAX];
diff --git a/daemon.c b/daemon.c
index b1fcbe0d6fa..2dc46e37852 100644
--- a/daemon.c
+++ b/daemon.c
@@ -131,7 +131,8 @@ static void loginfo(const char *err, ...)
 	va_end(params);
 }
 
-static void NORETURN daemon_die(const char *err, va_list params)
+static void NORETURN daemon_die(const char *file, int line, const char *err,
+				va_list params)
 {
 	logreport(LOG_ERR, err, params);
 	exit(1);
diff --git a/http-backend.c b/http-backend.c
index 982cb62c7cb..185fd84887d 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -655,13 +655,14 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
 }
 
 static int dead;
-static NORETURN void die_webcgi(const char *err, va_list params)
+static NORETURN void die_webcgi(const char *file, int line, const char *err,
+				va_list params)
 {
 	if (dead <= 1) {
 		struct strbuf hdr = STRBUF_INIT;
 		report_fn die_message_fn = get_die_message_routine();
 
-		die_message_fn(err, params);
+		die_message_fn(__FILE__, __LINE__, err, params);
 
 		http_status(&hdr, 500, "Internal Server Error");
 		hdr_nocache(&hdr);
diff --git a/run-command.c b/run-command.c
index 48b5fe19a80..c7d410f20a8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -340,19 +340,19 @@ static void child_close_pair(int fd[2])
 	child_close(fd[1]);
 }
 
-static void child_error_fn(const char *err, va_list params)
+static void child_error_fn(const char *file, int line, const char *err, va_list params)
 {
 	const char msg[] = "error() should not be called in child\n";
 	xwrite(2, msg, sizeof(msg) - 1);
 }
 
-static void child_warning_fn(const char *err, va_list params)
+static void child_warn_fn(const char *file, int line, const char *err, va_list params)
 {
-	const char msg[] = "warning() should not be called in child\n";
+	const char msg[] = "warn() should not be called in child\n";
 	xwrite(2, msg, sizeof(msg) - 1);
 }
 
-static void NORETURN child_die_fn(const char *err, va_list params)
+static void NORETURN child_die_fn(const char *file, int line, const char *err, va_list params)
 {
 	const char msg[] = "die() should not be called in child\n";
 	xwrite(2, msg, sizeof(msg) - 1);
@@ -783,7 +783,7 @@ int start_command(struct child_process *cmd)
 		 */
 		set_die_routine(child_die_fn);
 		set_error_routine(child_error_fn);
-		set_warning_routine(child_warning_fn);
+		set_warning_routine(child_warn_fn);
 
 		close(notify_pipe[0]);
 		set_cloexec(notify_pipe[1]);
@@ -1071,11 +1071,12 @@ static void *run_thread(void *data)
 	return (void *)ret;
 }
 
-static NORETURN void die_async(const char *err, va_list params)
+static NORETURN void die_async(const char *file, int line, const char *err,
+			       va_list params)
 {
 	report_fn die_message_fn = get_die_message_routine();
 
-	die_message_fn(err, params);
+	die_message_fn(__FILE__, __LINE__, err, params);
 
 	if (in_async()) {
 		struct async *async = pthread_getspecific(async_key);
diff --git a/trace2.h b/trace2.h
index 1b109f57d0a..713271d2c24 100644
--- a/trace2.h
+++ b/trace2.h
@@ -120,9 +120,6 @@ int trace2_cmd_exit_fl(const char *file, int line, int code);
 void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
 			    va_list ap);
 
-#define trace2_cmd_error_va(fmt, ap) \
-	trace2_cmd_error_va_fl(__FILE__, __LINE__, (fmt), (ap))
-
 /*
  * Emit a 'pathname' event with the canonical pathname of the current process
  * This gives post-processors a simple field to identify the command without
diff --git a/usage.c b/usage.c
index 4b93744137d..e6f609fe49a 100644
--- a/usage.c
+++ b/usage.c
@@ -31,7 +31,7 @@ static void vreportf(const char *prefix, const char *err, va_list params)
 	write_in_full(2, msg, p - msg);
 }
 
-static NORETURN void usage_builtin(const char *err, va_list params)
+static NORETURN void usage_builtin(const char *file, int line, const char *err, va_list params)
 {
 	vreportf("usage: ", err, params);
 
@@ -55,35 +55,35 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 	exit(129);
 }
 
-static void die_message_builtin(const char *err, va_list params)
+static void die_message_builtin(const char *file, int line, const char *err, va_list params)
 {
-	trace2_cmd_error_va(err, params);
+	trace2_cmd_error_va_fl(file, line, err, params);
 	vreportf("fatal: ", err, params);
 }
 
 /*
- * We call trace2_cmd_error_va() in the below functions first and
+ * We call trace2_cmd_error_va_fl(file, line, ...) in the below functions first and
  * expect it to va_copy 'params' before using it (because an 'ap' can
  * only be walked once).
  */
-static NORETURN void die_builtin(const char *err, va_list params)
+static NORETURN void die_builtin(const char *file, int line, const char *err, va_list params)
 {
 	report_fn die_message_fn = get_die_message_routine();
 
-	die_message_fn(err, params);
+	die_message_fn(file, line, err, params);
 	exit(128);
 }
 
-static void error_builtin(const char *err, va_list params)
+static void error_builtin(const char *file, int line, const char *err, va_list params)
 {
-	trace2_cmd_error_va(err, params);
+	trace2_cmd_error_va_fl(file, line, err, params);
 
 	vreportf("error: ", err, params);
 }
 
-static void warning_builtin(const char *warn, va_list params)
+static void warning_builtin(const char *file, int line, const char *warn, va_list params)
 {
-	trace2_cmd_error_va(warn, params);
+	trace2_cmd_error_va_fl(file, line, warn, params);
 
 	vreportf("warning: ", warn, params);
 }
@@ -178,21 +178,18 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt)
 	return buf;
 }
 
-void NORETURN usage(const char *fmt)
-{
-	usagef("%s", fmt);
-}
-
-void NORETURN usagef(const char *fmt, ...)
+NORETURN
+void usage_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
 
 	va_start(ap, fmt);
-	usage_routine(fmt, ap);
+	usage_routine(file, line, fmt, ap);
 	va_end(ap);
 }
 
-void NORETURN die(const char *fmt, ...)
+NORETURN
+void die_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
 
@@ -202,14 +199,15 @@ void NORETURN die(const char *fmt, ...)
 	}
 
 	va_start(ap, fmt);
-	die_routine(fmt, ap);
+	die_routine(file, line, fmt, ap);
 	va_end(ap);
 }
 
-void NORETURN die_errno(const char *fmt, ...)
+NORETURN
+void die_errno_fl(const char *file, int line, const char *fmt, ...)
 {
-	char buf[1024];
 	va_list ap;
+	char buf[1024];
 
 	if (die_is_recursing()) {
 		fputs("fatal: recursion detected in die_errno handler\n",
@@ -218,72 +216,67 @@ void NORETURN die_errno(const char *fmt, ...)
 	}
 
 	va_start(ap, fmt);
-	die_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	die_routine(file, line, fmt_with_err(buf, sizeof(buf), fmt), ap);
 	va_end(ap);
 }
 
-#undef die_message
-int die_message(const char *fmt, ...)
+int die_message_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
 
 	va_start(ap, fmt);
-	die_message_routine(fmt, ap);
+	die_message_routine(file, line, fmt, ap);
 	va_end(ap);
-	return 128;
+	return -1;
 }
 
-#undef die_message_errno
-int die_message_errno(const char *fmt, ...)
+int die_message_errno_fl(const char *file, int line, const char *fmt, ...)
 {
-	char buf[1024];
 	va_list ap;
 
 	va_start(ap, fmt);
-	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	die_message_routine(file, line, fmt, ap);
 	va_end(ap);
 	return -1;
 }
 
-#undef error
-int error(const char *fmt, ...)
+int error_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
 
 	va_start(ap, fmt);
-	error_routine(fmt, ap);
+	error_routine(file, line, fmt, ap);
 	va_end(ap);
 	return -1;
 }
 
-#undef error_errno
-int error_errno(const char *fmt, ...)
+int error_errno_fl(const char *file, int line, const char *fmt, ...)
 {
-	char buf[1024];
 	va_list ap;
+	char buf[1024];
 
 	va_start(ap, fmt);
-	error_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	error_routine(file, line, fmt_with_err(buf, sizeof(buf), fmt), ap);
 	va_end(ap);
 	return -1;
 }
 
-void warning(const char *fmt, ...)
+void warning_fl(const char *file, int line, const char *fmt, ...)
 {
 	va_list ap;
 
 	va_start(ap, fmt);
-	warning_routine(fmt, ap);
+	warning_routine(file, line, fmt, ap);
 	va_end(ap);
 }
 
-void warning_errno(const char *fmt, ...)
+void warning_errno_fl(const char *file, int line, const char *fmt, ...)
 {
 	char buf[1024];
 	va_list ap;
 
 	va_start(ap, fmt);
-	warning_routine(fmt_with_err(buf, sizeof(buf), fmt), ap);
+	warning_routine(file, line, fmt_with_err(buf, sizeof(buf), fmt), ap);
 	va_end(ap);
 }
 
@@ -313,7 +306,7 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
 		abort();
 	in_bug = 1;
 
-	trace2_cmd_error_va(fmt, params_copy);
+	trace2_cmd_error_va_fl(file, line, fmt, params_copy);
 
 	if (BUG_exit_code)
 		exit(BUG_exit_code);
@@ -339,7 +332,7 @@ int bug_fl(const char *file, int line, const char *fmt, ...)
 	va_start(ap, fmt);
 	BUG_vfl_common(file, line, fmt, ap);
 	va_end(ap);
-	trace2_cmd_error_va(fmt, cp);
+	trace2_cmd_error_va_fl(file, line, fmt, cp);
 
 	return -1;
 }
diff --git a/usage.h b/usage.h
index df02fe9bcaf..e4a65ad755a 100644
--- a/usage.h
+++ b/usage.h
@@ -106,25 +106,41 @@ extern int BUG_exit_code;
 /* If bug() is called we must have a BUG() invocation afterwards */
 extern int bug_called_must_BUG;
 
-/* General helper functions */
-NORETURN void usage(const char *err);
-NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
-NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
-NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
-int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
-void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-
 /* General helper functions invoked via macro wrappers */
 __attribute__((format (printf, 3, 4))) NORETURN
+void usage_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4))) NORETURN
+void die_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4))) NORETURN
+void die_errno_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4)))
+int die_message_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4)))
+int die_message_errno_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4)))
+int error_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4)))
+int error_errno_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4)))
+void warning_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4)))
+void warning_errno_fl(const char *file, int line, const char *fmt, ...);
+__attribute__((format (printf, 3, 4))) NORETURN
 void BUG_fl(const char *file, int line, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
 int bug_fl(const char *file, int line, const char *fmt, ...);
 
 /* General helper macros */
+#define usage(...) usage_fl(__FILE__, __LINE__, "%s", __VA_ARGS__)
+#define usagef(...) usage_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define die(...) die_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define die_errno(...) die_errno_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define die_message(...) die_message_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define die_message_errno(...) die_message_errno_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define error(...) error_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define error_errno(...) error_errno_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define warning(...) warning_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define warning_errno(...) warning_errno_fl(__FILE__, __LINE__, __VA_ARGS__)
 #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
 #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
 #define BUG_if_bug() do { \
@@ -135,7 +151,8 @@ int bug_fl(const char *file, int line, const char *fmt, ...);
 } while (0)
 
 /* Setting custom handling routines */
-typedef void (*report_fn)(const char *, va_list params);
+typedef void (*report_fn)(const char *file, int line, const char *fmt,
+			  va_list params);
 void set_die_routine(NORETURN_PTR report_fn routine);
 report_fn get_die_message_routine(void);
 void set_error_routine(report_fn routine);
@@ -154,8 +171,10 @@ static inline int const_error(void)
 {
 	return -1;
 }
-#define error(...) (error(__VA_ARGS__), const_error())
-#define error_errno(...) (error_errno(__VA_ARGS__), const_error())
+#undef error
+#undef error_errno
+#define error(...) (error_fl(__FILE__, __LINE__, __VA_ARGS__), const_error())
+#define error_errno(...) (error_errno_fl(__FILE__, __LINE__, __VA_ARGS__), const_error())
 #endif
 
 #endif
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 20/21] usage API: make the "{usage,fatal,error,warning,BUG}: " translatable

[Ævar Arnfjörð Bjarmason]

In preceding commits the vreportf() function was made static, so we
know it's only being called with a limited set of fixed prefixes. Pass
an enum indicating the kind of usage message we're emitting instead,
which means that we can fold the BUG_vfl_common() functionality
directly into it.

Since we've now got one place were we're emitting these usage messages
we can make them translatable.

We need to be careful with this function to not malloc() anything, as
a failure in say a use of strbuf_vaddf() would call xmalloc(), which
would in turn call die(), but here we're using static strings, either
from libintl or not.

I was on the fence about making the "BUG: " message translatable, but
let's do it for consistency. Someone who doesn't speak a word of
English may not know what "BUG" means, but if it's translated they
might have an easier time knowing that they have to report a bug
upstream. Since we'll always emit the line number it's unlikely that
we're going to be confused by such a report.

As we've moved the BUG_vfl_common() code into vsnprintf() we can do
away with one of the two checks for buffer sizes added in
116d1fa6c69 (vreportf(): avoid relying on stdio buffering, 2019-10-30)
and ac4896f007a (fmt_with_err: add a comment that truncation is OK,
2018-05-18).

I.e. we're being overly paranoid if we define the fixed-size "prefix"
and "msg" buffers, are OK with the former being truncated, and then
effectively check if our 256-byte buffer is larger than our 4096-byte
buffer. I wondered about adding a:

    assert(sizeof(prefix) < sizeof(msg)); /* overly paranoid much? */

But I think that would be overdoing it. Anyone modifying this function
will keep these two buffer sizes in mind, so let's just remove one of
the checks instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 usage.c | 68 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/usage.c b/usage.c
index e6f609fe49a..62313862977 100644
--- a/usage.c
+++ b/usage.c
@@ -6,16 +6,49 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
-static void vreportf(const char *prefix, const char *err, va_list params)
+enum usage_kind {
+	USAGE_USAGE,
+	USAGE_DIE,
+	USAGE_ERROR,
+	USAGE_WARNING,
+	USAGE_BUG,
+};
+
+static void vreportf(enum usage_kind kind,
+		     const char *file, int line,
+		     const char *err, va_list params)
 {
+	const char *prefix_i18n;
+	char prefix[256];
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
-	size_t prefix_len = strlen(prefix);
-
-	if (sizeof(msg) <= prefix_len) {
-		fprintf(stderr, "BUG!!! too long a prefix '%s'\n", prefix);
-		abort();
+	size_t prefix_len;
+
+	switch (kind) {
+	case USAGE_USAGE:
+		prefix_i18n =_("usage: ");
+		break;
+	case USAGE_DIE:
+		prefix_i18n =_("fatal: ");
+		break;
+	case USAGE_ERROR:
+		prefix_i18n =_("error: ");
+		break;
+	case USAGE_WARNING:
+		prefix_i18n =_("warning: ");
+		break;
+	case USAGE_BUG:
+		prefix_i18n =_("BUG: ");
+		break;
 	}
+
+	/* truncation via snprintf is OK here */
+	if (kind == USAGE_BUG)
+		snprintf(prefix, sizeof(prefix), "%s%s:%d: ", prefix_i18n, file, line);
+	else
+		snprintf(prefix, sizeof(prefix), "%s", prefix_i18n);
+
+	prefix_len = strlen(prefix);
 	memcpy(msg, prefix, prefix_len);
 	p = msg + prefix_len;
 	if (vsnprintf(p, pend - p, err, params) < 0)
@@ -33,7 +66,7 @@ static void vreportf(const char *prefix, const char *err, va_list params)
 
 static NORETURN void usage_builtin(const char *file, int line, const char *err, va_list params)
 {
-	vreportf("usage: ", err, params);
+	vreportf(USAGE_USAGE, file, line, err, params);
 
 	/*
 	 * When we detect a usage error *before* the command dispatch in
@@ -58,7 +91,7 @@ static NORETURN void usage_builtin(const char *file, int line, const char *err,
 static void die_message_builtin(const char *file, int line, const char *err, va_list params)
 {
 	trace2_cmd_error_va_fl(file, line, err, params);
-	vreportf("fatal: ", err, params);
+	vreportf(USAGE_DIE, file, line, err, params);
 }
 
 /*
@@ -78,14 +111,14 @@ static void error_builtin(const char *file, int line, const char *err, va_list p
 {
 	trace2_cmd_error_va_fl(file, line, err, params);
 
-	vreportf("error: ", err, params);
+	vreportf(USAGE_ERROR, file, line, err, params);
 }
 
 static void warning_builtin(const char *file, int line, const char *warn, va_list params)
 {
 	trace2_cmd_error_va_fl(file, line, warn, params);
 
-	vreportf("warning: ", warn, params);
+	vreportf(USAGE_WARNING, file, line, warn, params);
 }
 
 static int die_is_recursing_builtin(void)
@@ -283,24 +316,13 @@ void warning_errno_fl(const char *file, int line, const char *fmt, ...)
 /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
 int BUG_exit_code;
 
-static void BUG_vfl_common(const char *file, int line, const char *fmt,
-			   va_list params)
-{
-	char prefix[256];
-
-	/* 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);
+	vreportf(USAGE_BUG, file, line, fmt, params);
 
 	if (in_bug)
 		abort();
@@ -330,7 +352,7 @@ int bug_fl(const char *file, int line, const char *fmt, ...)
 
 	va_copy(cp, ap);
 	va_start(ap, fmt);
-	BUG_vfl_common(file, line, fmt, ap);
+	vreportf(USAGE_BUG, file, line, fmt, ap);
 	va_end(ap);
 	trace2_cmd_error_va_fl(file, line, fmt, cp);
 
-- 
2.34.0.rc2.809.g11e21d44b24

[RFC PATCH 21/21] usage API: add "core.usageAddSource" config to add <file>:<line>

[Ævar Arnfjörð Bjarmason]

Optionally extend the support that BUG() has had for emitting line
numbers to the {usage,fatal,error,warning}{,_errno}() functions.

Before we'd unconditionally get error messages like:

    $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
    fatal: bad boolean config value 'y' for 'core.x'

Which can be changed with core.usageAddSource=true to include the file
and line numbers:

    $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
    fatal: config.c:1241: bad boolean config value 'y' for 'core.x'

As the added documentation notes this is primarily intended to be
useful to developers of git itself, but is being exposed as a user
setting to e.g. help file better bug reports.

This also adds a "GIT_TEST_USAGE_ADD_SOURCE" setting intended to run
the test suite in this mode.

Currently it has a lot of failures. Most of those are rather trivial,
and can be "fixed" by pointing GIT_TEST_CMP to a "diff -u" that does a
s/^(usage|fatal|error|warning): [^:]+:[0-9]+/$1/g on its input files,
and likewise for a "grep" wrapper that does the same.

Even if we can't run the tests in this mode yet I'd like to have this
for ad-hoc debugging, and to make it easier to work towards running
the tests in this mode. If we can turn this on permanently it'll be
much easier to read test output, as we won't need to worry about the
indirection of looking up where an error might have been emitted,
which can be especially painful when the message being emitted isn't
unique within git.git.

This new code needs to be guarded by the "dying" variable for the
reasons explained in 2d3c02f5db6 (die(): stop hiding errors due to
overzealous recursion guard, 2017-06-21), and for those same reasons
it's racy under multi-threading.

Here the worst case is that incrementing the variable will run away
from us, and we won't get our desired <file>:<line> number. That's OK
in this case, those cases should be isolated to the config code (if we
can't parse the config), memory allocation etc, but we'll get it right
in the common cases.

Using GIT_TEST_USAGE_ADD_SOURCE should be immune from any racyness, as
it only needs a getenv() and git_parse_maybe_bool(), which won't die.

Add a repo_cfg_bool_env() wrapper to repo-settings.c for
GIT_TEST_USAGE_ADD_SOURCE, in 3050b6dfc75 (repo-settings.c: simplify
the setup, 2021-09-21) I indicated that the GIT_TEST_MULTI_PACK_INDEX
env variable/config pair in that file has odd semantics, but users of
repo_cfg_bool_env() have straightforward and expected semantics. If
the environment variable is set (true or false) we'll use it,
otherwise we'll use the config, and finally fall back on the
default (of "false", in this case).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/core.txt |  7 +++++++
 repo-settings.c               | 11 +++++++++++
 repository.h                  |  2 ++
 usage.c                       | 24 ++++++++++++++++++++++--
 4 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index c04f62a54a1..a2d8c274c6a 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -101,6 +101,13 @@ core.untrackedCache::
 	`feature.manyFiles` is enabled which sets this setting to
 	`true` by default.
 
+core.usageAddSource::
+	Adds the source "<file>:<line>" numbers to usage messages
+	("usage:", "fatal:", "error:", "warning:"). This setting is
+	primarily intended for those debugging or developing git
+	itself, or to compose a bug report against git itself (see
+	linkgit:git-bugreport[1]).
+
 core.checkStat::
 	When missing or is set to `default`, many fields in the stat
 	structure are checked to detect if a file has been modified
diff --git a/repo-settings.c b/repo-settings.c
index b93e91a212e..33eb582d590 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -10,6 +10,15 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
 		*dest = def;
 }
 
+static void repo_cfg_bool_env(struct repository *r, const char *env,
+			      const char *key, int *dest, int def)
+{
+	*dest = git_env_bool(env, -1);
+	if (*dest != -1)
+		return;
+	repo_cfg_bool(r, key, dest, def);
+}
+
 void prepare_repo_settings(struct repository *r)
 {
 	int experimental;
@@ -45,6 +54,8 @@ void prepare_repo_settings(struct repository *r)
 	repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
 	repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
 	repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
+	repo_cfg_bool_env(r, "GIT_TEST_USAGE_ADD_SOURCE", "core.usageaddsource",
+			  &r->settings.usage_add_source, 0);
 	repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
 
 	/*
diff --git a/repository.h b/repository.h
index a057653981c..0e362f4adb2 100644
--- a/repository.h
+++ b/repository.h
@@ -41,6 +41,8 @@ struct repo_settings {
 	enum fetch_negotiation_setting fetch_negotiation_algorithm;
 
 	int core_multi_pack_index;
+
+	int usage_add_source;
 };
 
 struct repository {
diff --git a/usage.c b/usage.c
index 62313862977..409f857a8f7 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,7 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#include "config.h"
 
 enum usage_kind {
 	USAGE_USAGE,
@@ -14,6 +15,8 @@ enum usage_kind {
 	USAGE_BUG,
 };
 
+static int dying;
+
 static void vreportf(enum usage_kind kind,
 		     const char *file, int line,
 		     const char *err, va_list params)
@@ -23,6 +26,24 @@ static void vreportf(enum usage_kind kind,
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
 	size_t prefix_len;
+	static int addln = -1;
+
+	if (addln < 0 && dying <= 1) {
+		struct repository *r = the_repository;
+		prepare_repo_settings(r);
+		addln = r->settings.usage_add_source;
+	} else if (addln < 0 && dying) {
+		/*
+		 * We're already dying when trying to read our config,
+		 * presumably via prepare_repo_settings(). Do a
+		 * last-ditch effort of trying to get it from the
+		 * environment.
+		 */
+		const char *v = getenv("GIT_TEST_USAGE_ADD_SOURCE");
+		addln = v ? git_parse_maybe_bool(v) : 0;
+		if (addln < 1)
+			addln = 0;
+	}
 
 	switch (kind) {
 	case USAGE_USAGE:
@@ -43,7 +64,7 @@ static void vreportf(enum usage_kind kind,
 	}
 
 	/* truncation via snprintf is OK here */
-	if (kind == USAGE_BUG)
+	if (kind == USAGE_BUG || addln)
 		snprintf(prefix, sizeof(prefix), "%s%s:%d: ", prefix_i18n, file, line);
 	else
 		snprintf(prefix, sizeof(prefix), "%s", prefix_i18n);
@@ -123,7 +144,6 @@ static void warning_builtin(const char *file, int line, const char *warn, va_lis
 
 static int die_is_recursing_builtin(void)
 {
-	static int dying;
 	/*
 	 * Just an arbitrary number X where "a < x < b" where "a" is
 	 * "maximum number of pthreads we'll ever plausibly spawn" and
-- 
2.34.0.rc2.809.g11e21d44b24

Re: [RFC PATCH 00/21] C99: show meaningful <file>:<line> in trace2 via macros

[Taylor Blau]

On Mon, Nov 15, 2021 at 11:18:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Since everyone's getting in on the C99 fun.
>
> Well, $subject and a bit more. This RFC series has bits and pieces
> from thing I've submitted before. I'd proposed to make variadic macros
> a hard dependency before in [1] because I wanted to get to the goal in
> $subject, perhaps the whole thing will be more convincing.
>
> This also includes the die_message() in a recent series of mine[2]
> that I abandoned.
>
> At the end of this series we expose a config variable to have
> usage/die/warning emit line numbers. I.e. going from:
>
>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>     fatal: bad boolean config value 'y' for 'core.x'
>
> To:
>
>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>     fatal: config.c:1241: bad boolean config value 'y' for 'core.x'

Just picking on this output change in particular. I agree that this is
easier for folks hacking on Git to trace down errors. But I'm not sure
that I could say then same about users, who will likely treat this extra
output as noise.

Now we may find it helpful if they include it in a bug report, but I
feel reasonably comfortable saying that the value there is pretty
marginal. I don't find it all that problematic to grep for a specific
error string, and usually find myself in the right place.

> I find that to make tracing down errors in the test suite, and 21/21
> has a GIT_TEST_* mode to turn it on there (which fails a lot now, but
> I'm hoping I'll eventually get passing).
>
> But most importantly we've now got meaningful file/line numbers in
> trace2 error events. I.e. from all of them being some line in usage.c:
>
>     $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
>     {
>       "event": "error",
>       "sid": "20211115T221343.534151Z-Hc2f5b994-P003f3980",
>       "thread": "main",
>       "time": "2021-11-15T22:13:43.537981Z",
>       "file": "usage.c",
>       "line": 65,
>       "msg": "bad boolean config value 'y' for 'core.x'",
>       "fmt": "bad boolean config value '%s' for '%s'"
>     }
>
> To:
>
>     $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
>     {
>       "event": "error",
>       "sid": "20211115T221357.083824Z-Hc2f5b994-P003f4a82",
>       "thread": "main",
>       "time": "2021-11-15T22:13:57.087596Z",
>       "file": "config.c",
>       "line": 1241,
>       "msg": "bad boolean config value 'y' for 'core.x'",
>       "fmt": "bad boolean config value '%s' for '%s'"
>     }

Neat. This is a use-case that has all of the value without putting it in
front of users all of the time. I like it.

> This is "RFC" mainly because there's a CI failure in 0061.2 with this,
> I still can't figure out what that's about (or if it's some fluke
> unrelated to this topic), but that has to be investigated.

Hmm. Putting the CI failures aside for a second, wouldn't we want to
hold off on something like this until we have flown the C99 weather
balloon for a while? If we suddenly start introducing C99-isms into the
code while brian's patch is still young, then we can suddenly no longer
say, "oh, just drop this #if because there are no other C99-specific
uses here", and instead compilers that don't support the newer standard
are out of luck.

That may have been already communicated elsewhere in this message and/or
throughout your patch series, so if I missed it, I apologize. Just
felt that it was worth stating the obvious before we go too far down the
wrong path.

Thanks,
Taylor

Re: [RFC PATCH 00/21] C99: show meaningful <file>:<line> in trace2 via macros

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 16 2021, Taylor Blau wrote:

> On Mon, Nov 15, 2021 at 11:18:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Since everyone's getting in on the C99 fun.
>>
>> Well, $subject and a bit more. This RFC series has bits and pieces
>> from thing I've submitted before. I'd proposed to make variadic macros
>> a hard dependency before in [1] because I wanted to get to the goal in
>> $subject, perhaps the whole thing will be more convincing.
>>
>> This also includes the die_message() in a recent series of mine[2]
>> that I abandoned.
>>
>> At the end of this series we expose a config variable to have
>> usage/die/warning emit line numbers. I.e. going from:
>>
>>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>>     fatal: bad boolean config value 'y' for 'core.x'
>>
>> To:
>>
>>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>>     fatal: config.c:1241: bad boolean config value 'y' for 'core.x'
>
> Just picking on this output change in particular. I agree that this is
> easier for folks hacking on Git to trace down errors. But I'm not sure
> that I could say then same about users, who will likely treat this extra
> output as noise.
>
> Now we may find it helpful if they include it in a bug report, but I
> feel reasonably comfortable saying that the value there is pretty
> marginal. I don't find it all that problematic to grep for a specific
> error string, and usually find myself in the right place.

I wouldn't suggest exposing this to users, except perhaps as part of
some "how to submit a bugreport" instructions. It's thoroughly optional.

I thought it was easy enough to do with the preceding steps since all
the data is there, and would help my workflow a lot.

If you've got the file/line number like that you can make it clickable
in your terminal/compile mode, e.g. Emacs's M-x compile. Saves time over
having to grep manually select the string, grep for it etc.

Anyway, I can certainly live with peeling this patch off the end and
just stopping at the trace2 data for now, if you/others feel strongly
about it.

>> I find that to make tracing down errors in the test suite, and 21/21
>> has a GIT_TEST_* mode to turn it on there (which fails a lot now, but
>> I'm hoping I'll eventually get passing).
>>
>> But most importantly we've now got meaningful file/line numbers in
>> trace2 error events. I.e. from all of them being some line in usage.c:
>>
>>     $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
>>     {
>>       "event": "error",
>>       "sid": "20211115T221343.534151Z-Hc2f5b994-P003f3980",
>>       "thread": "main",
>>       "time": "2021-11-15T22:13:43.537981Z",
>>       "file": "usage.c",
>>       "line": 65,
>>       "msg": "bad boolean config value 'y' for 'core.x'",
>>       "fmt": "bad boolean config value '%s' for '%s'"
>>     }
>>
>> To:
>>
>>     $ GIT_TRACE2_EVENT=/dev/stdout ~/g/git/git -c core.usageAddSource=false -c core.x=y config --get --bool core.x 2>&1 2>/dev/null|grep error | jq -r .
>>     {
>>       "event": "error",
>>       "sid": "20211115T221357.083824Z-Hc2f5b994-P003f4a82",
>>       "thread": "main",
>>       "time": "2021-11-15T22:13:57.087596Z",
>>       "file": "config.c",
>>       "line": 1241,
>>       "msg": "bad boolean config value 'y' for 'core.x'",
>>       "fmt": "bad boolean config value '%s' for '%s'"
>>     }
>
> Neat. This is a use-case that has all of the value without putting it in
> front of users all of the time. I like it.
>
>> This is "RFC" mainly because there's a CI failure in 0061.2 with this,
>> I still can't figure out what that's about (or if it's some fluke
>> unrelated to this topic), but that has to be investigated.
>
> Hmm. Putting the CI failures aside for a second, wouldn't we want to
> hold off on something like this until we have flown the C99 weather
> balloon for a while? If we suddenly start introducing C99-isms into the
> code while brian's patch is still young, then we can suddenly no longer
> say, "oh, just drop this #if because there are no other C99-specific
> uses here", and instead compilers that don't support the newer standard
> are out of luck.
>
> That may have been already communicated elsewhere in this message and/or
> throughout your patch series, so if I missed it, I apologize. Just
> felt that it was worth stating the obvious before we go too far down the
> wrong path.

As noted in 02/21 we're hard depending on this particular C99 feature
already fon a few releases now, the only change on that front in this
series is to stop committing to maintaining the non-C99 codepaths.

We've already had hard dependencies on various bits of C99 for years now
without any trouble, and I wouldn't expect any problems on this front
either.

Brian's series and the current weatherbaloon from Junio are a bit
different in trying out new things we either haven't done before, or
have run into some trouble with in the past.

No need at all to apologize, it's a lot of patches, and raising this
sort of thing is what patch review is good for.

Thanks a lot for looking this over.

Re: [RFC PATCH 00/21] C99: show meaningful <file>:<line> in trace2 via macros

[Taylor Blau]

On Tue, Nov 16, 2021 at 07:58:01PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Nov 16 2021, Taylor Blau wrote:
> >> At the end of this series we expose a config variable to have
> >> usage/die/warning emit line numbers. I.e. going from:
> >>
> >>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
> >>     fatal: bad boolean config value 'y' for 'core.x'
> >>
> >> To:
> >>
> >>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
> >>     fatal: config.c:1241: bad boolean config value 'y' for 'core.x'
> >
> > Just picking on this output change in particular. I agree that this is
> > easier for folks hacking on Git to trace down errors. But I'm not sure
> > that I could say then same about users, who will likely treat this extra
> > output as noise.
> >
> > Now we may find it helpful if they include it in a bug report, but I
> > feel reasonably comfortable saying that the value there is pretty
> > marginal. I don't find it all that problematic to grep for a specific
> > error string, and usually find myself in the right place.
>
> I wouldn't suggest exposing this to users, except perhaps as part of
> some "how to submit a bugreport" instructions. It's thoroughly optional.
>
> I thought it was easy enough to do with the preceding steps since all
> the data is there, and would help my workflow a lot.
>
> If you've got the file/line number like that you can make it clickable
> in your terminal/compile mode, e.g. Emacs's M-x compile. Saves time over
> having to grep manually select the string, grep for it etc.
>
> Anyway, I can certainly live with peeling this patch off the end and
> just stopping at the trace2 data for now, if you/others feel strongly
> about it.

I don't feel strongly, and I was just noting that it seemed like users
would treat this extra information more often as noise than anything
else.

When you talk about making it optional, do you mean through
configuration / an environment variable, or by including / not including
the patch? In other words, the latter seems much more like us making a
decision on whether or not to include line numbers rather than
presenting a new option to users, though I may be misunderstanding.

> As noted in 02/21 we're hard depending on this particular C99 feature
> already fon a few releases now, the only change on that front in this
> series is to stop committing to maintaining the non-C99 codepaths.

> We've already had hard dependencies on various bits of C99 for years now
> without any trouble, and I wouldn't expect any problems on this front
> either.

Interesting; so this and others are likely part of MSVC's kind-of
support for C99 features? In other words, that MSVC supports some
features from C99 (and we are depending on a subset of those) but not
all features so that it could reasonably be called a spec-compliant
compiler for the C99 standard? If so, makes sense.

Thanks,
Taylor

Re: [RFC PATCH 00/21] C99: show meaningful <file>:<line> in trace2 via macros

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 16 2021, Taylor Blau wrote:

> On Tue, Nov 16, 2021 at 07:58:01PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Nov 16 2021, Taylor Blau wrote:
>> >> At the end of this series we expose a config variable to have
>> >> usage/die/warning emit line numbers. I.e. going from:
>> >>
>> >>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>> >>     fatal: bad boolean config value 'y' for 'core.x'
>> >>
>> >> To:
>> >>
>> >>     $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
>> >>     fatal: config.c:1241: bad boolean config value 'y' for 'core.x'
>> >
>> > Just picking on this output change in particular. I agree that this is
>> > easier for folks hacking on Git to trace down errors. But I'm not sure
>> > that I could say then same about users, who will likely treat this extra
>> > output as noise.
>> >
>> > Now we may find it helpful if they include it in a bug report, but I
>> > feel reasonably comfortable saying that the value there is pretty
>> > marginal. I don't find it all that problematic to grep for a specific
>> > error string, and usually find myself in the right place.
>>
>> I wouldn't suggest exposing this to users, except perhaps as part of
>> some "how to submit a bugreport" instructions. It's thoroughly optional.
>>
>> I thought it was easy enough to do with the preceding steps since all
>> the data is there, and would help my workflow a lot.
>>
>> If you've got the file/line number like that you can make it clickable
>> in your terminal/compile mode, e.g. Emacs's M-x compile. Saves time over
>> having to grep manually select the string, grep for it etc.
>>
>> Anyway, I can certainly live with peeling this patch off the end and
>> just stopping at the trace2 data for now, if you/others feel strongly
>> about it.
>
> I don't feel strongly, and I was just noting that it seemed like users
> would treat this extra information more often as noise than anything
> else.
>
> When you talk about making it optional, do you mean through
> configuration / an environment variable, or by including / not including
> the patch? In other words, the latter seems much more like us making a
> decision on whether or not to include line numbers rather than
> presenting a new option to users, though I may be misunderstanding.

Not surprising, since I see that I screwed up the summary in both the CL
and 21/21. I.e. both of these are =false (I copy/pasted the error
around, but didn't adjust the command that was invoked):

    $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
    fatal: bad boolean config value 'y' for 'core.x'
    $ git -c core.usageAddSource=false -c core.x=y config --get --bool core.x
    fatal: config.c:1241: bad boolean config value 'y' for 'core.x'

That second one should be core.usageAddSource=true, i.e. as seen in the
21/21 implentation and core.txt docs you only get these line numebers if
you opt-in to them via configuration or the new GIT_TEST_* environment
variable.

>> As noted in 02/21 we're hard depending on this particular C99 feature
>> already fon a few releases now, the only change on that front in this
>> series is to stop committing to maintaining the non-C99 codepaths.
>
>> We've already had hard dependencies on various bits of C99 for years now
>> without any trouble, and I wouldn't expect any problems on this front
>> either.
>
> Interesting; so this and others are likely part of MSVC's kind-of
> support for C99 features? In other words, that MSVC supports some
> features from C99 (and we are depending on a subset of those) but not
> all features so that it could reasonably be called a spec-compliant
> compiler for the C99 standard? If so, makes sense.

Yes, I think this is one of the things that's the same or similar enough
to C++ that MSVC has good support for it. See the "We try to support a
wide range of C compilers[...]" section in the CodingGuidelines for a
list of some other C99 features we've used for years already.

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Jeff Hostetler]

On 11/15/21 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
[...]
> 
> It might be a good change to remove the "fmt" key from the "error"
> events as a follow-up change. As these few examples from running the
> test suite show it's sometimes redundant (same as the "msg"), rather
> useless (just a "%s"), or something we could now mostly aggregate by
> file/line instead of the normalized printf format:
> 
>        1 file":"builtin/gc.c","line":1391,"msg":"'bogus' is not a valid task","fmt":"'%s' is not a valid task"}
>        1 file":"builtin/for-each-ref.c","line":89,"msg":"format: %(then) atom used more than once","fmt":"%s"}
>        1 file":"builtin/fast-import.c","line":411,"msg":"Garbage after mark: N :202 :302x","fmt":"Garbage after mark: %s"}
> 
> "Mostly" here assumes that it would be OK if the aggregation changed
> between git versions, which may be what all users of trace2 want. The
> change that introduced the "fmt" code was ee4512ed481 (trace2: create
> new combined trace facility, 2019-02-22), and the documentation change
> was e544221d97a (trace2: Documentation/technical/api-trace2.txt,
> 2019-02-22).
> 
> Both are rather vague on what problem "fmt" solved exactly, aside from
> the obvious one of it being impossible to do meaningful aggregations
> due to the "file" and "line" being the same everywhere, which isn't
> the case now.
> 
> In any case, let's leave "fmt" be for now, the above summary was given
> in case it's interesting to remove it in the future, e.g. to save
> space in trace2 payloads.

I added the "fmt" field so that we could do aggregations
of error messages across multiple users without regard
to what branch or filename or percentage or whatever was
formatted into the actual "msg" written to stderr.

The actual file:line wasn't useful (primarily because it
was probably something in usage.c), but even if we fix that
it might not be useful if we have users running 10 different
versions of Git (because some people don't upgrade immediately).

So I'd rather not kill it right now.

Jeff

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 27 2021, Jeff Hostetler wrote:

> On 11/15/21 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> It might be a good change to remove the "fmt" key from the "error"
>> events as a follow-up change. As these few examples from running the
>> test suite show it's sometimes redundant (same as the "msg"), rather
>> useless (just a "%s"), or something we could now mostly aggregate by
>> file/line instead of the normalized printf format:
>>        1 file":"builtin/gc.c","line":1391,"msg":"'bogus' is not a
>> valid task","fmt":"'%s' is not a valid task"}
>>        1 file":"builtin/for-each-ref.c","line":89,"msg":"format: %(then) atom used more than once","fmt":"%s"}
>>        1 file":"builtin/fast-import.c","line":411,"msg":"Garbage after mark: N :202 :302x","fmt":"Garbage after mark: %s"}
>> "Mostly" here assumes that it would be OK if the aggregation changed
>> between git versions, which may be what all users of trace2 want. The
>> change that introduced the "fmt" code was ee4512ed481 (trace2: create
>> new combined trace facility, 2019-02-22), and the documentation change
>> was e544221d97a (trace2: Documentation/technical/api-trace2.txt,
>> 2019-02-22).
>> Both are rather vague on what problem "fmt" solved exactly, aside
>> from
>> the obvious one of it being impossible to do meaningful aggregations
>> due to the "file" and "line" being the same everywhere, which isn't
>> the case now.
>> In any case, let's leave "fmt" be for now, the above summary was
>> given
>> in case it's interesting to remove it in the future, e.g. to save
>> space in trace2 payloads.
>
> I added the "fmt" field so that we could do aggregations
> of error messages across multiple users without regard
> to what branch or filename or percentage or whatever was
> formatted into the actual "msg" written to stderr.
>
> The actual file:line wasn't useful (primarily because it
> was probably something in usage.c), but even if we fix that
> it might not be useful if we have users running 10 different
> versions of Git (because some people don't upgrade immediately).
>
> So I'd rather not kill it right now.

Thanks. I'm not trying to kill it, but just poking at what it was for
exactly.

Depending on the answer to that perhaps we didn't need it anymore, but
the explanation you provide (mostly) makes sense.

The "mostly" being because I'm assuming that you only need to deal with
LC_ALL=C users?

I.e. the documented promise that you can group things by "fmt" doesn't
hold if you're processing even streams from users who are using a
translated git, because we'll get the translated format string, not the
original.

For that we'd need to change the API from/to to:

    - error(_("some format %s"), ...)
    + error(N_("some format %s"), ...)

So being able to say "just group on file/line" would be simpler.

And also "mostly" because the "fmt" case also won't handle these and
other duplicate formats (but maybe you haven't run into them in
practice):

    $ git grep -E '\b(usage|die|error|warning)(_errno)?\("%s\"' -- '*.[ch]' | wc -l
    90

So I was somewhat hoping for future work that you'd be OK with the new
file/line grouping.

Because keeping "fmt" would eventually need some massive coccinelle
search/replacement for "_(...)" -> "N_(...)" per the above, even then
consumers of the stream would get duplicate grouping for the likes of
"%s".

Do you think if as a follow-up we had "__func__"[1] along with
"file/line" that the "file/__func__" combination would be good enough?
The advantage of that would be that we could punt that "fmt"
change/complexity and document:

    If you'd like to group errors the "file/line" pair will be unique
    enough within a given git version to do so (sans a few codepaths that
    relay errors from elsewhere).

    If you'd like a semi-stable grouping across similar git versions the
    "file/func" pair should be Good Enough for most purposes. Some functions
    might emit multiple errors, but you'd probably want to group them as
    similar enough anyway.

But I realize that those things don't give you exactly the same things
that "fmt" does, but maybe they're good enough (or even better?), or
not.

1. https://gcc.gnu.org/onlinedocs/gcc/Function-Names.html

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Jeff Hostetler]

On 12/27/21 6:01 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Dec 27 2021, Jeff Hostetler wrote:
> 
>> On 11/15/21 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
>> [...]
>>> It might be a good change to remove the "fmt" key from the "error"
>>> events as a follow-up change. As these few examples from running the
>>> test suite show it's sometimes redundant (same as the "msg"), rather
>>> useless (just a "%s"), or something we could now mostly aggregate by
>>> file/line instead of the normalized printf format:
>>>         1 file":"builtin/gc.c","line":1391,"msg":"'bogus' is not a
>>> valid task","fmt":"'%s' is not a valid task"}
>>>         1 file":"builtin/for-each-ref.c","line":89,"msg":"format: %(then) atom used more than once","fmt":"%s"}
>>>         1 file":"builtin/fast-import.c","line":411,"msg":"Garbage after mark: N :202 :302x","fmt":"Garbage after mark: %s"}
>>> "Mostly" here assumes that it would be OK if the aggregation changed
>>> between git versions, which may be what all users of trace2 want. The
>>> change that introduced the "fmt" code was ee4512ed481 (trace2: create
>>> new combined trace facility, 2019-02-22), and the documentation change
>>> was e544221d97a (trace2: Documentation/technical/api-trace2.txt,
>>> 2019-02-22).
>>> Both are rather vague on what problem "fmt" solved exactly, aside
>>> from
>>> the obvious one of it being impossible to do meaningful aggregations
>>> due to the "file" and "line" being the same everywhere, which isn't
>>> the case now.
>>> In any case, let's leave "fmt" be for now, the above summary was
>>> given
>>> in case it's interesting to remove it in the future, e.g. to save
>>> space in trace2 payloads.
>>
>> I added the "fmt" field so that we could do aggregations
>> of error messages across multiple users without regard
>> to what branch or filename or percentage or whatever was
>> formatted into the actual "msg" written to stderr.
>>
>> The actual file:line wasn't useful (primarily because it
>> was probably something in usage.c), but even if we fix that
>> it might not be useful if we have users running 10 different
>> versions of Git (because some people don't upgrade immediately).
>>
>> So I'd rather not kill it right now.
> 
> Thanks. I'm not trying to kill it, but just poking at what it was for
> exactly.
> 
> Depending on the answer to that perhaps we didn't need it anymore, but
> the explanation you provide (mostly) makes sense.
> 
> The "mostly" being because I'm assuming that you only need to deal with
> LC_ALL=C users?
> 
> I.e. the documented promise that you can group things by "fmt" doesn't
> hold if you're processing even streams from users who are using a
> translated git, because we'll get the translated format string, not the
> original.

I just did a query on the data we've collected over the last
few weeks and there are only English error messages in the
database, so yes LC_ALL=C seems to be the norm.

> 
> For that we'd need to change the API from/to to:
> 
>      - error(_("some format %s"), ...)
>      + error(N_("some format %s"), ...)

So no, I don't think it is worth the complexity to change
this.  Besides, wouldn't you need to more machinery under
the hood -- to emit the untranslated string to trace2 and
the translated string to stderr?  (As in, move the translation
down a layer??)

My "fmt" field is not worth that effort.

And besides, my goal was only to get the "top 10 or 20 errors"
across a large set of users.  I guess I'm thinking of it as a
sample rather than an exhaustive list, so it is OK if we don't
capture the translated strings.


Something else to consider is the GDPR.  The "fmt" string is
generic (e.g. "path '%s' exists on disk, but not in the index")
but doesn't leak an PII or otherwise sensitive data.  Whereas
the corresponding "msg" field does include the pathname in this
example.  So if someone is post-processing the data and aggregating,
they may want to relay only the "fmt" field and not the "msg" field
to their database.

(Granted, there are lots of PII and GDPR problematic fields in
the data stream that a post-processor would need to be aware of,
but all of that is outside of the scope of the Trace2 logging.
I only mention it here because the "fmt" field may be useful for
reasons not previously discussed.)

> 
> So being able to say "just group on file/line" would be simpler.
> 
> And also "mostly" because the "fmt" case also won't handle these and
> other duplicate formats (but maybe you haven't run into them in
> practice):
> 
>      $ git grep -E '\b(usage|die|error|warning)(_errno)?\("%s\"' -- '*.[ch]' | wc -l
>      90
> 
> So I was somewhat hoping for future work that you'd be OK with the new
> file/line grouping.
> 
> Because keeping "fmt" would eventually need some massive coccinelle
> search/replacement for "_(...)" -> "N_(...)" per the above, even then
> consumers of the stream would get duplicate grouping for the likes of
> "%s".
> 
> Do you think if as a follow-up we had "__func__"[1] along with
> "file/line" that the "file/__func__" combination would be good enough?
> The advantage of that would be that we could punt that "fmt"
> change/complexity and document:
> 
>      If you'd like to group errors the "file/line" pair will be unique
>      enough within a given git version to do so (sans a few codepaths that
>      relay errors from elsewhere).
> 
>      If you'd like a semi-stable grouping across similar git versions the
>      "file/func" pair should be Good Enough for most purposes. Some functions
>      might emit multiple errors, but you'd probably want to group them as
>      similar enough anyway.
> 
> But I realize that those things don't give you exactly the same things
> that "fmt" does, but maybe they're good enough (or even better?), or
> not.
> 
> 1. https://gcc.gnu.org/onlinedocs/gcc/Function-Names.html
> 

I'll have to think about this some and get back to you.
Jeff

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Elijah Newren]

Just adding another datapoint...

At my $DAYJOB, we've been collecting trace2 information but have only
used it very lightly so far.  We've been meaning to use it more.
Anyway, I have a couple (possibly not fully informed) opinions based
on that...

On Tue, Dec 28, 2021 at 8:32 AM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
> On 12/27/21 6:01 PM, Ævar Arnfjörð Bjarmason wrote:
> >
> > On Mon, Dec 27 2021, Jeff Hostetler wrote:
> >
> >> On 11/15/21 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
> >> [...]
> >>> It might be a good change to remove the "fmt" key from the "error"
> >>> events as a follow-up change. As these few examples from running the
> >>> test suite show it's sometimes redundant (same as the "msg"), rather
> >>> useless (just a "%s"), or something we could now mostly aggregate by
> >>> file/line instead of the normalized printf format:
> >>>         1 file":"builtin/gc.c","line":1391,"msg":"'bogus' is not a
> >>> valid task","fmt":"'%s' is not a valid task"}
> >>>         1 file":"builtin/for-each-ref.c","line":89,"msg":"format: %(then) atom used more than once","fmt":"%s"}
> >>>         1 file":"builtin/fast-import.c","line":411,"msg":"Garbage after mark: N :202 :302x","fmt":"Garbage after mark: %s"}
> >>> "Mostly" here assumes that it would be OK if the aggregation changed
> >>> between git versions, which may be what all users of trace2 want. The
> >>> change that introduced the "fmt" code was ee4512ed481 (trace2: create
> >>> new combined trace facility, 2019-02-22), and the documentation change
> >>> was e544221d97a (trace2: Documentation/technical/api-trace2.txt,
> >>> 2019-02-22).
> >>> Both are rather vague on what problem "fmt" solved exactly, aside
> >>> from
> >>> the obvious one of it being impossible to do meaningful aggregations
> >>> due to the "file" and "line" being the same everywhere, which isn't
> >>> the case now.
> >>> In any case, let's leave "fmt" be for now, the above summary was
> >>> given
> >>> in case it's interesting to remove it in the future, e.g. to save
> >>> space in trace2 payloads.
> >>
> >> I added the "fmt" field so that we could do aggregations
> >> of error messages across multiple users without regard
> >> to what branch or filename or percentage or whatever was
> >> formatted into the actual "msg" written to stderr.
> >>
> >> The actual file:line wasn't useful (primarily because it
> >> was probably something in usage.c), but even if we fix that
> >> it might not be useful if we have users running 10 different
> >> versions of Git (because some people don't upgrade immediately).
> >>
> >> So I'd rather not kill it right now.
> >
> > Thanks. I'm not trying to kill it, but just poking at what it was for
> > exactly.
> >
> > Depending on the answer to that perhaps we didn't need it anymore, but
> > the explanation you provide (mostly) makes sense.
> >
> > The "mostly" being because I'm assuming that you only need to deal with
> > LC_ALL=C users?
> >
> > I.e. the documented promise that you can group things by "fmt" doesn't
> > hold if you're processing even streams from users who are using a
> > translated git, because we'll get the translated format string, not the
> > original.
>
> I just did a query on the data we've collected over the last
> few weeks and there are only English error messages in the
> database, so yes LC_ALL=C seems to be the norm.
>
> > For that we'd need to change the API from/to to:
> >
> >      - error(_("some format %s"), ...)
> >      + error(N_("some format %s"), ...)
>
> So no, I don't think it is worth the complexity to change
> this.  Besides, wouldn't you need to more machinery under
> the hood -- to emit the untranslated string to trace2 and
> the translated string to stderr?  (As in, move the translation
> down a layer??)
>
> My "fmt" field is not worth that effort.
>
> And besides, my goal was only to get the "top 10 or 20 errors"
> across a large set of users.  I guess I'm thinking of it as a
> sample rather than an exhaustive list, so it is OK if we don't
> capture the translated strings.

This makes sense to me.

> Something else to consider is the GDPR.  The "fmt" string is
> generic (e.g. "path '%s' exists on disk, but not in the index")
> but doesn't leak an PII or otherwise sensitive data.  Whereas
> the corresponding "msg" field does include the pathname in this
> example.  So if someone is post-processing the data and aggregating,
> they may want to relay only the "fmt" field and not the "msg" field
> to their database.
>
> (Granted, there are lots of PII and GDPR problematic fields in
> the data stream that a post-processor would need to be aware of,
> but all of that is outside of the scope of the Trace2 logging.
> I only mention it here because the "fmt" field may be useful for
> reasons not previously discussed.)
>
> >
> > So being able to say "just group on file/line" would be simpler.
> >
> > And also "mostly" because the "fmt" case also won't handle these and
> > other duplicate formats (but maybe you haven't run into them in
> > practice):
> >
> >      $ git grep -E '\b(usage|die|error|warning)(_errno)?\("%s\"' -- '*.[ch]' | wc -l
> >      90
> >
> > So I was somewhat hoping for future work that you'd be OK with the new
> > file/line grouping.
> >
> > Because keeping "fmt" would eventually need some massive coccinelle
> > search/replacement for "_(...)" -> "N_(...)" per the above, even then
> > consumers of the stream would get duplicate grouping for the likes of
> > "%s".
> >
> > Do you think if as a follow-up we had "__func__"[1] along with
> > "file/line" that the "file/__func__" combination would be good enough?
> > The advantage of that would be that we could punt that "fmt"
> > change/complexity and document:
> >
> >      If you'd like to group errors the "file/line" pair will be unique
> >      enough within a given git version to do so (sans a few codepaths that
> >      relay errors from elsewhere).

I don't actually like the file/line pair technique so much.  We have a
little influence but no control over deployed git versions.  We've
collected data about git versions in use, and we've got every single
git version going back several years in use (plus, some people took
the copy of Git I built with ort and other stuff included early but
they haven't all upgraded as I've updated it leaving us with an even
greater sprawl of file/line numbers).  That means file/line pairs
would split messages much more than the number of languages would.
(We do have developers in over a dozen different countries, though we
have a strong majority in English-speaking countries.)

> >      If you'd like a semi-stable grouping across similar git versions the
> >      "file/func" pair should be Good Enough for most purposes. Some functions
> >      might emit multiple errors, but you'd probably want to group them as
> >      similar enough anyway.

Why would we want to group different errors?  Isn't the point to
figure out which error is being triggered the most (or which errors)?
This sounds like it'd leave us with more investigation work to do.

> > But I realize that those things don't give you exactly the same things
> > that "fmt" does, but maybe they're good enough (or even better?), or
> > not.

I think "fmt" is strictly better, personally.

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 28 2021, Jeff Hostetler wrote:

> On 12/27/21 6:01 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Dec 27 2021, Jeff Hostetler wrote:
>> 
>>> On 11/15/21 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
>>> [...]
>>>> It might be a good change to remove the "fmt" key from the "error"
>>>> events as a follow-up change. As these few examples from running the
>>>> test suite show it's sometimes redundant (same as the "msg"), rather
>>>> useless (just a "%s"), or something we could now mostly aggregate by
>>>> file/line instead of the normalized printf format:
>>>>         1 file":"builtin/gc.c","line":1391,"msg":"'bogus' is not a
>>>> valid task","fmt":"'%s' is not a valid task"}
>>>>         1 file":"builtin/for-each-ref.c","line":89,"msg":"format: %(then) atom used more than once","fmt":"%s"}
>>>>         1 file":"builtin/fast-import.c","line":411,"msg":"Garbage after mark: N :202 :302x","fmt":"Garbage after mark: %s"}
>>>> "Mostly" here assumes that it would be OK if the aggregation changed
>>>> between git versions, which may be what all users of trace2 want. The
>>>> change that introduced the "fmt" code was ee4512ed481 (trace2: create
>>>> new combined trace facility, 2019-02-22), and the documentation change
>>>> was e544221d97a (trace2: Documentation/technical/api-trace2.txt,
>>>> 2019-02-22).
>>>> Both are rather vague on what problem "fmt" solved exactly, aside
>>>> from
>>>> the obvious one of it being impossible to do meaningful aggregations
>>>> due to the "file" and "line" being the same everywhere, which isn't
>>>> the case now.
>>>> In any case, let's leave "fmt" be for now, the above summary was
>>>> given
>>>> in case it's interesting to remove it in the future, e.g. to save
>>>> space in trace2 payloads.
>>>
>>> I added the "fmt" field so that we could do aggregations
>>> of error messages across multiple users without regard
>>> to what branch or filename or percentage or whatever was
>>> formatted into the actual "msg" written to stderr.
>>>
>>> The actual file:line wasn't useful (primarily because it
>>> was probably something in usage.c), but even if we fix that
>>> it might not be useful if we have users running 10 different
>>> versions of Git (because some people don't upgrade immediately).
>>>
>>> So I'd rather not kill it right now.
>> Thanks. I'm not trying to kill it, but just poking at what it was
>> for
>> exactly.
>> Depending on the answer to that perhaps we didn't need it anymore,
>> but
>> the explanation you provide (mostly) makes sense.
>> The "mostly" being because I'm assuming that you only need to deal
>> with
>> LC_ALL=C users?
>> I.e. the documented promise that you can group things by "fmt"
>> doesn't
>> hold if you're processing even streams from users who are using a
>> translated git, because we'll get the translated format string, not the
>> original.
>
> I just did a query on the data we've collected over the last
> few weeks and there are only English error messages in the
> database, so yes LC_ALL=C seems to be the norm.

Ah, that explains it.

>> For that we'd need to change the API from/to to:
>>      - error(_("some format %s"), ...)
>>      + error(N_("some format %s"), ...)
>
> So no, I don't think it is worth the complexity to change
> this.  Besides, wouldn't you need to more machinery under
> the hood -- to emit the untranslated string to trace2 and
> the translated string to stderr?  (As in, move the translation
> down a layer??)

Yes, hence N_(), it marks the string for translation, but doesn't
translate it. So the underlying function would call _() for what we emit
to stderr, but not for the "fmt" field.

> My "fmt" field is not worth that effort.

Probably not to you because you're deploying git into a monolingual
environment, but for anyone who is they'd need to maintain manual
groupings of messages by scraping our po/*.po files.

> And besides, my goal was only to get the "top 10 or 20 errors"
> across a large set of users.  I guess I'm thinking of it as a
> sample rather than an exhaustive list, so it is OK if we don't
> capture the translated strings.

I suppose with enough users it wouldn't matter either way, but you'd get
quite a bit of fragmentation. You'd also have errors that don't differ
between translations (e.g. those "%s" cases) amplified in count, as they
won't fragment due to i18n.

> Something else to consider is the GDPR.  The "fmt" string is
> generic (e.g. "path '%s' exists on disk, but not in the index")
> but doesn't leak an PII or otherwise sensitive data.  Whereas
> the corresponding "msg" field does include the pathname in this
> example.  So if someone is post-processing the data and aggregating,
> they may want to relay only the "fmt" field and not the "msg" field
> to their database.

Indeed.

> (Granted, there are lots of PII and GDPR problematic fields in
> the data stream that a post-processor would need to be aware of,
> but all of that is outside of the scope of the Trace2 logging.
> I only mention it here because the "fmt" field may be useful for
> reasons not previously discussed.)

I do think it would be a good thing to work on, i.e. to make the logging
less verbose, which as a side-effect would make it GDPR compliant.

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 28 2021, Elijah Newren wrote:

> On Tue, Dec 28, 2021 at 8:32 AM Jeff Hostetler <git@jeffhostetler.com> wrote:
>> >      If you'd like a semi-stable grouping across similar git versions the
>> >      "file/func" pair should be Good Enough for most purposes. Some functions
>> >      might emit multiple errors, but you'd probably want to group them as
>> >      similar enough anyway.
>
> Why would we want to group different errors?  Isn't the point to
> figure out which error is being triggered the most (or which errors)?
> This sounds like it'd leave us with more investigation work to do.

Ideally you wouldn't, i.e. the goal here is to get some approximation of
a unique ID for an error across versions.

But unless we're going to assign something like MySQL's error ID's
manually any automatic method we pick is only going to be an
approximation.

So the question is whether we can have something that's good enough. The
current "fmt" feature is fragmented by i18n. That's fixable (at the cost
of quite a lot of lines changed), but would something even more succinct
be good enough?

Which is why I suggested file/function, i.e. it'll have some
duplication, but for an error dashboard using trace2 data I'd think it's
probably good enough.

But maybe not. I just wanted to ask about it as a quick question...

>> > But I realize that those things don't give you exactly the same things
>> > that "fmt" does, but maybe they're good enough (or even better?), or
>> > not.
>
> I think "fmt" is strictly better, personally.

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Elijah Newren]

On Tue, Dec 28, 2021 at 3:53 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Tue, Dec 28 2021, Elijah Newren wrote:
>
> > On Tue, Dec 28, 2021 at 8:32 AM Jeff Hostetler <git@jeffhostetler.com> wrote:
> >> >      If you'd like a semi-stable grouping across similar git versions the
> >> >      "file/func" pair should be Good Enough for most purposes. Some functions
> >> >      might emit multiple errors, but you'd probably want to group them as
> >> >      similar enough anyway.
> >
> > Why would we want to group different errors?  Isn't the point to
> > figure out which error is being triggered the most (or which errors)?
> > This sounds like it'd leave us with more investigation work to do.
>
> Ideally you wouldn't, i.e. the goal here is to get some approximation of
> a unique ID for an error across versions.
>
> But unless we're going to assign something like MySQL's error ID's
> manually any automatic method we pick is only going to be an
> approximation.

I like this way that you frame it.  I agree.

> So the question is whether we can have something that's good enough. The
> current "fmt" feature is fragmented by i18n. That's fixable (at the cost
> of quite a lot of lines changed), but would something even more succinct
> be good enough?
>
> Which is why I suggested file/function, i.e. it'll have some
> duplication, but for an error dashboard using trace2 data I'd think it's
> probably good enough.
>
> But maybe not. I just wanted to ask about it as a quick question...

I think for determining the most frequently triggered errors,
fragmentation is a minor issue, so you are right to call it out.  In
particular, having the counts of issues separated by language might
mean that when we pick the top N errors, some of those in the top N
wouldn't really be in the top N if we had them correctly combined with
the other translations (and we also might get duplicates within our
chosen top N, since an english and a german translation of the same
error are both in the top N of the fragmented counts).  Pretty
unlikely to be a problem in practice, though, and rather trivial to
work around once we have the data collected and are looking into it.
Even in the really unlikely event that I was trying to fix a "top N"
problem and accidentally ended up with a "top N+2" problem, I'm still
dealing with a "real error" that users are hitting.  Any work I do to
fix it will help people facing a real problem.

In contrast, coalescing of errors to me would be a major issue.  Let's
say I look at the top error, as reported by file/function.  But that
one error is from a function that has four error paths.  If I take a
guess at one of those error paths and try to fix it, I might be
chasing ghosts and completely wasting my time.  My first step should
be to go back to the drawing board and attempt to collect data about
what error the user was actually hitting (a rather lengthy process,
especially in attempting over a period of weeks/months to cajole users
to upgrade their git versions to get the new logging) -- but that was
exactly what this trace2 stuff was supposed to be doing in the first
place, so the file/function approximation choice defeats the purpose
of this error logging.  It sounds like a deal breaker to me.

My gut instinct is that I'd take nearly any level of fragmentation
over the possible coalescing of separate errors.

I think the fragmentation solutions probably fall under the "good
enough" category.  So, for example, the file/line number might be good
enough.  It's a lot more fragmentation than different languages,
though, and it also suffers from the problem that it's hard to tell if
new git versions are fixing some of the "top N" problems (because new
git versions would have different line numbers and thus represent the
top N problems differently, whereas the fmt-based fragmentation will
at least be relatively consistent in its representation of errors
across git versions).  But if the fmt solution was super problematic
for some other reasons, I'd gladly take file/line-number over
file/function.

So, of the solutions presented so far, the "fmt" feature seems to me
to be the best reasonable effort approximation.

Anyway, just my $0.02...

Re: [RFC PATCH 19/21] usage API: use C99 macros for {usage,usagef,die,error,warning,die}*()

[Jeff Hostetler]

On 12/28/21 11:32 AM, Jeff Hostetler wrote:
> 
> 
> On 12/27/21 6:01 PM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Mon, Dec 27 2021, Jeff Hostetler wrote:
>>
>>> On 11/15/21 5:18 PM, Ævar Arnfjörð Bjarmason wrote:
>>> [...]
>>
>> So being able to say "just group on file/line" would be simpler.
>>
>> And also "mostly" because the "fmt" case also won't handle these and
>> other duplicate formats (but maybe you haven't run into them in
>> practice):
>>
>>      $ git grep -E '\b(usage|die|error|warning)(_errno)?\("%s\"' -- 
>> '*.[ch]' | wc -l
>>      90
>>
>> So I was somewhat hoping for future work that you'd be OK with the new
>> file/line grouping.
>>
>> Because keeping "fmt" would eventually need some massive coccinelle
>> search/replacement for "_(...)" -> "N_(...)" per the above, even then
>> consumers of the stream would get duplicate grouping for the likes of
>> "%s".
>>
>> Do you think if as a follow-up we had "__func__"[1] along with
>> "file/line" that the "file/__func__" combination would be good enough?
>> The advantage of that would be that we could punt that "fmt"
>> change/complexity and document:
>>
>>      If you'd like to group errors the "file/line" pair will be unique
>>      enough within a given git version to do so (sans a few codepaths 
>> that
>>      relay errors from elsewhere).
>>
>>      If you'd like a semi-stable grouping across similar git versions the
>>      "file/func" pair should be Good Enough for most purposes. Some 
>> functions
>>      might emit multiple errors, but you'd probably want to group them as
>>      similar enough anyway.
>>
>> But I realize that those things don't give you exactly the same things
>> that "fmt" does, but maybe they're good enough (or even better?), or
>> not.
>>
>> 1. https://gcc.gnu.org/onlinedocs/gcc/Function-Names.html
>>
> 
> I'll have to think about this some and get back to you.

I think I'd rather just have the "fmt" string in the log.

I don't think the file:line or func:line helps here.  Elsewhere
in this thread we've talked about having to support a user base
running various versions of Git (and don't forget to mix in GFW
and the GVFS-enabled version of Git).

Keep in mind that some users enable (or their EngSys/IT team
enables for them) brief mode (GIT_TRACE2_EVENT_BRIEF)
which omits the file:line data from the log.  This reduces the
size of the data stream on all events, so we don't have that
data for some users -- and forcing it on would send a lot more
data and cost a lot more than any savings from omitting the
somewhat redundant "fmt" field.

Just having the format string usually lets us track down the
error/die call in the code using just grep (unless it is one of
those `die("%s",...)` cases (which should be fixed independent
of this discussion)).  As Elijah mentioned elsewhere in this
thread, just having the format string doesn't mean we know exactly
which path/branch in the code lead us to that error, so local
analysis is still required if we want to kill a "top 10" item.
Having file:line doesn't help with that local effort.

As for handling translations, I'm not really worried about it.
If a post-processor really wants to get complete aggregations
independent of the users locale they could maybe build a tool
to load up the .po files, build a reverse index, and add a table
to their database that they could join with.  I'm speculating
here that anyone would want to go to that trouble, but it is
possible.  And it would keep all of this churn out of Git code.

Coalescing the format strings is the simplest by far.  So I'd
really like to leave things the way they are.

Jeff