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

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

[John Szakmeister]

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.

Signed-off-by: John Szakmeister <john@szakmeister.net>
---
 contrib/diff-highlight/diff-highlight | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index c4404d4..dfcc35a 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -14,6 +14,15 @@ my @removed;
 my @added;
 my $in_hunk;
 
+# 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.
+sub pipe_handler {
+    exit(0);
+}
+
+$SIG{PIPE} = \&pipe_handler;
+
 while (<>) {
 	if (!$in_hunk) {
 		print;
-- 
2.0.1

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

[Jeff King]

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

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

[John Szakmeister]

On Sat, Nov 1, 2014 at 12:04 AM, Jeff King <peff@peff.net> wrote:
> 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?

Yes, that's correct.  It's useful, so with a few tools that use diffs,
I like to run the output through diff-highlight.

>> +# 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".

Hehe, now that I see you right it out, I realize my mistake: I didn't
capitalize 'default'.  Trying it out again, it does appear that does
the trick.

[snip]
> 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?

Sorry, my Perl-fu is kind of low these days.  I used to use it all the
time but switched away from it quite a while ago.  Given that
'DEFAULT' does the trick, I'll just re-roll my patch to use that.
Does that sound fair?

-John

PS  Sorry for the late response, I've been traveling.