[PATCH 1/3] usage: refactor die-recursion checks

From: Jeff King <peff@peff.net>
Date: Apr 15, 2013, 23:08 UTC
Message-ID: <20130415230802.GA11267@sigill.intra.peff.net>
In-Reply-To: <20130415230651.GA16670@sigill.intra.peff.net>

When any git code calls die(), we chain to a custom die_routine, which we expect to print a message and exit the program. To avoid infinite loops, we detect a recursive call to die() with a simple counter, and break out of the loop by printing a message and exiting ourselves, without chaining to the die_routine.

But the user does not get to see the message that would have been fed to the die_routine, which makes debugging harder. The user does not know if it was a true infinite loop, or simply a single re-entrant call, since they cannot compare the messages. Furthermore, if we are wrong about detecting the recursion, we have blocked the user from seeing the original message, which is probably the more useful one.

This patch teaches die() to print the original die message to stderr before reporting the recursion. The custom die_routine may or may not have put it the message to stderr, but this is the best we can do (it is what most handlers will do anyway, and it is where our recursion error will go).

While we're at it, let's mark the "recursion detected" message as a "BUG:", since it should never happen in practice. And let's factor out the repeated code in die and die_errno. This loses the information of which function was called to cause the recursion, but it's important; knowing the actual message fed to the function (which we now do) is much more useful, as it can generally pin-point the actual call-site that caused the recursion.

Signed-off-by: Jeff King <peff@peff.net>
---
This helped me debug the current problem. And factoring it out helps
with patch 3. :)

 usage.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/usage.c b/usage.c
index 40b3de5..c6b7ac5 100644
--- a/usage.c
+++ b/usage.c
@@ -6,8 +6,6 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
-static int dying;
-
 void vreportf(const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
@@ -80,17 +78,24 @@ void NORETURN die(const char *err, ...)
 	usagef("%s", err);
 }
 
+static void check_die_recursion(const char *fmt, va_list ap)
+{
+	static int dying;
+
+	if (!dying++)
+		return;
+
+	vreportf("fatal: ", fmt, ap);
+	fputs("BUG: recursion detected in die handler\n", stderr);
+	exit(128);
+}
+
 void NORETURN die(const char *err, ...)
 {
 	va_list params;
 
-	if (dying) {
-		fputs("fatal: recursion detected in die handler\n", stderr);
-		exit(128);
-	}
-	dying = 1;
-
 	va_start(params, err);
+	check_die_recursion(err, params);
 	die_routine(err, params);
 	va_end(params);
 }
@@ -102,13 +107,6 @@ void NORETURN die_errno(const char *fmt, ...)
 	char str_error[256], *err;
 	int i, j;
 
-	if (dying) {
-		fputs("fatal: recursion detected in die_errno handler\n",
-			stderr);
-		exit(128);
-	}
-	dying = 1;
-
 	err = strerror(errno);
 	for (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {
 		if ((str_error[j++] = err[i++]) != '%')
@@ -126,6 +124,7 @@ void NORETURN die_errno(const char *fmt, ...)
 	snprintf(fmt_with_err, sizeof(fmt_with_err), "%s: %s", fmt, str_error);
 
 	va_start(params, fmt);
+	check_die_recursion(fmt_with_err, params);
 	die_routine(fmt_with_err, params);
 	va_end(params);
 }

-- 
1.8.2.8.g44e4c28