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

From: Jeff King <peff@peff.net>
Date: Jun 20, 2017, 16:15 UTC
Message-ID: <20170620161514.ygbflanx4pldc7n7@sigill.intra.peff.net>
In-Reply-To: <20170620155459.a6e7pypxx6vpdbvs@sigill.intra.peff.net>

On Tue, Jun 20, 2017 at 11:54:59AM -0400, Jeff King wrote:

Show 10 quoted lines

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

To clarify, I think anytime we spawn worker threads that might run die() we probably need to be installing not just a custom recursion handler but a custom die function.

It's weird to see:

  $ git grep ...
  fatal: some error
  fatal: some error
  fatal: some error

Or even:

  $ git grep ...
  fatal: some error
  some actual results
  more actual results

I'm not sure what the _right_ thing is there, but it probably involves recording the per-thread errors in individual buffers, waiting for the master to pthread_join() them all, and then doing something like:

  /* this also covers the case of only having 1 error */
  int all_errors_identical = 1;
  for (i = 1; i < nr_errors; i++) {
	if (strcmp(errors[i], errors[i-1])) {
		all_errors_identical = 0;
		break;
	}
  }

  if (all_errors_identical) {
	/* just show it */
	die("%s", errors[0]);
  } else {
	for (i = 0; i < nr_errors; i++)
		error("%s", errors[i]);
	die("multiple errors encountered");
  }

I don't know if we'd want to actually get into the details of what "multiple errors" means. It isn't _all_ of the possible errors, because each thread stopped running when it hit the die(). But it's also not just one error.

Actually, I guess we could just pick one error and show only that one. That would most closely match the non-threaded case. And it's way simpler than what I wrote above.

Hrm. I guess you could even do that without buffering if you allow the thread to take down the whole process. The only thing you'd need to do is teach die() to take a mutex so that we don't racily show multiple errors.

That seems like the best option (I almost just deleted my entire email, but maybe the thought process in leading there is useful).

-Peff