Re: [PATCH] diff-highlight: exit when a pipe is broken

[Jeff King <peff@peff.net>]

On Fri, Oct 31, 2014 at 07:04:04AM -0400, John Szakmeister wrote:

> While using diff-highlight with other tools, I have discovered that Python
> ignores SIGPIPE by default.  Unfortunately, this also means that tools
> attempting to launch a pager under Python--and don't realize this is
> happening--means that the subprocess inherits this setting.  In this case, it
> means diff-highlight will be launched with SIGPIPE being ignored.  Let's work
> with those broken scripts by explicitly setting up a SIGPIPE handler and exiting
> the process.

My first thought was that this should be handled already by 7559a1b
(unblock and unignore SIGPIPE, 2014-09-18), but after re-reading your
message, it sounds like you are using diff-highlight with non-git
programs?

> +# Some scripts may not realize that SIGPIPE is being ignored when launching the
> +# pager--for instance scripts written in Python.  Setting $SIG{PIPE} = 'DEFAULT'
> +# doesn't work in these instances, so we install our own signal handler instead.

Why doesn't $SIG{PIPE} = 'DEFAULT' work? I did some limited testing and
it seemed to work fine for me. Though I simulated the condition with:

  (
    trap '' PIPE
    perl -e '$|=1; print "foo\n"; print STDERR "bar\n"'
  ) | true

which should not ever print "bar".

Is Python doing something more aggressive, like using sigprocmask to
block the signal? I would think setting $SIG{PIPE} would handle that,
but maybe not in some versions of perl. I dunno. Modern perl
signal-handling is weird, as it catches everything and then defers
propagation until a safe point in the script (if you strace the script
above, you can see that it actually gets EPIPE from the write call!)
I've no clue what implications all that has for the case you're
addressing.

> +sub pipe_handler {
> +    exit(0);
> +}

Can we exit 141 here? If we are part of a pipeline to a pager, it should
not matter either way, but I'd rather not lose the exit code if we can
avoid it (in case of running the script standalone).

> +$SIG{PIPE} = \&pipe_handler;

A minor nit, but would:

  $SIG{PIPE} = sub { ... };

be nicer to avoid polluting the function namespace?

-Peff