Re: [PATCH] die routine: change recursion limit from 1 to 1024

From: Jeff King <peff@peff.net>
Date: Jun 20, 2017, 15:54 UTC
Message-ID: <20170620155459.a6e7pypxx6vpdbvs@sigill.intra.peff.net>
In-Reply-To: <20170619220036.22656-1-avarab@gmail.com>

On Mon, Jun 19, 2017 at 10:00:36PM +0000, Ævar Arnfjörð Bjarmason wrote:

Show 8 quoted lines

> Change the recursion limit for the default die routine from a *very*
> low 1 to 1024. This ensures that infinite recursions are broken, but
> doesn't lose error messages.
> 
> The intent of the existing code, as explained in commit
> cd163d4b4e ("usage.c: detect recursion in die routines and bail out
> immediately", 2012-11-14), is to break infinite recursion in cases
> where the die routine itself dies.

I agree that was the original intent, but I think it also does something else. Anytime die() recurses, even a single level, we're going to cover up the original failure with the one that happened inside die(), which is almost certainly the less interesting of the two.

E.g., if I

  die_errno("unable to open %s", filename);

and then the die handler calls malloc() and fails, you'd much rather see that first message than "out of memory".

To be fair, "die handler is recursing" is _also_ not helpful, but at least it's clear that this is a bug (and IMHO it should be marked with BUG()). Saying "out of memory" tells you about the second error, but it doesn't tell you that we've masked the first error. So it may lead to more confusion in the long run.

I wonder if we can get the best of both, though. Can we make the logic more like:

  if (!dying) {
	/* ok, normal */
	return 0;
  } else if (dying < 1024) {
	/* only show the warning once */
	if (dying == 1)
		warning("I heard you liked errors, so I put a die() in your die()");
	return 0; /* don't bail yet */
  } else {
	BUG("recursion detected in die handler");
  }

> Now, git-grep could make use of the pluggable error facility added in
> commit c19a490e37 ("usage: allow pluggable die-recursion checks",
> 2013-04-16).

Yeah, I think this is a bug in git-grep and should be fixed, independent of this commit. You should be able to use as a template the callbacks added by the child of c19a490e37:

  1ece66bc9 (run-command: use thread-aware die_is_recursing routine,
  2013-04-16)

-Peff