On Mon, Jun 19, 2017 at 10:00:36PM +0000, Ævar Arnfjörð Bjarmason wrote:
> 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
