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

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

Reply via email to