Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:

> The write_or_die function will always die on an error,
> including EPIPE. However, it currently treats EPIPE
> specially by suppressing any error message, and by exiting
> with exit code 0.
>
> Suppressing the error message makes some sense; a pipe death
> may just be a sign that the other side is not interested in
> what we have to say. However, exiting with a successful
> error code is not a good idea, as write_or_die is frequently
> used in cases where we want to be careful about having
> written all of the output, and we may need to signal to our
> caller that we have done so (e.g., you would not want a push
> whose other end has hung up to report success).
>
> This distinction doesn't typically matter in git, because we
> do not ignore SIGPIPE in the first place. Which means that
> we will not get EPIPE, but instead will just die when we get
> a SIGPIPE. But it's possible for a default handler to be set
> by a parent process,

Not so much "default" as "insane inherited", as in the example
of old versions of Python's subprocess.Popen.

I suspect this used exit(0) instead of raise(SIGPIPE) in the first
place to work around a bash bug (too much verbosity about SIGPIPE).
If any programs still have that kind of bug, I'd rather put pressure
on them to fix it by *not* working around it.  So the basic idea here
looks good to me.

[...]
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -1,5 +1,15 @@
>  #include "cache.h"
>  
> +static void check_pipe(int err)
> +{
> +	if (err == EPIPE) {
> +		signal(SIGPIPE, SIG_DFL);
> +		raise(SIGPIPE);
> +		/* Should never happen, but just in case... */
> +		exit(141);

How about

		die("BUG: another thread changed SIGPIPE handling behind my back!");

to make it easier to find and fix such problems?

Thanks,
Jonathan