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

From: Jeff King <peff@peff.net>
Date: Nov 1, 2014, 04:04 UTC
Message-ID: <20141101040443.GB8307@peff.net>
In-Reply-To: <1414753444-68653-1-git-send-email-john@szakmeister.net>

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

Show 7 quoted lines

> 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