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

From: John Szakmeister <john@szakmeister.net>
Date: Nov 4, 2014, 19:43 UTC
Message-ID: <CAEBDL5XUhEgbrGZHqrVu3d8QuhX_B9KT_h8pUW4LK_MWr-7KUQ@mail.gmail.com>
In-Reply-To: <20141101040443.GB8307@peff.net>

On Sat, Nov 1, 2014 at 12:04 AM, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

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

Show 13 quoted lines

>> +# 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]

Show 11 quoted lines

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