{"thread":{"id":"37856","subject":"[PATCH] diff-highlight: exit when a pipe is broken","startedAt":"2014-10-31T11:04:04Z","lastAt":"2014-11-04T19:43:56Z","messageCount":3,"participants":["John Szakmeister","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251265","messageId":"1414753444-68653-1-git-send-email-john@szakmeister.net","threadId":"37856","inReplyTo":null,"subject":"[PATCH] diff-highlight: exit when a pipe is broken","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2014-10-31T11:04:04Z","receivedAt":"2014-10-31T11:04:04Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"While using diff-highlight with other tools, I have discovered that Python\nignores SIGPIPE by default.  Unfortunately, this also means that tools\nattempting to launch a pager under Python--and don't realize this is\nhappening--means that the subprocess inherits this setting.  In this case, it\nmeans diff-highlight will be launched with SIGPIPE being ignored.  Let's work\nwith those broken scripts by explicitly setting up a SIGPIPE handler and exiting\nthe process.\n\nSigned-off-by: John Szakmeister <john@szakmeister.net>\n---\n contrib/diff-highlight/diff-highlight | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight\nindex c4404d4..dfcc35a 100755\n--- a/contrib/diff-highlight/diff-highlight\n+++ b/contrib/diff-highlight/diff-highlight\n@@ -14,6 +14,15 @@ my @removed;\n my @added;\n my $in_hunk;\n \n+# Some scripts may not realize that SIGPIPE is being ignored when launching the\n+# pager--for instance scripts written in Python.  Setting $SIG{PIPE} = 'DEFAULT'\n+# doesn't work in these instances, so we install our own signal handler instead.\n+sub pipe_handler {\n+    exit(0);\n+}\n+\n+$SIG{PIPE} = \\&pipe_handler;\n+\n while (<>) {\n \tif (!$in_hunk) {\n \t\tprint;\n-- \n2.0.1\n"},{"id":"251283","messageId":"20141101040443.GB8307@peff.net","threadId":"37856","inReplyTo":"1414753444-68653-1-git-send-email-john@szakmeister.net","subject":"Re: [PATCH] diff-highlight: exit when a pipe is broken","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-01T04:04:43Z","receivedAt":"2014-11-01T04:04:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 31, 2014 at 07:04:04AM -0400, John Szakmeister wrote:\n\n> While using diff-highlight with other tools, I have discovered that Python\n> ignores SIGPIPE by default.  Unfortunately, this also means that tools\n> attempting to launch a pager under Python--and don't realize this is\n> happening--means that the subprocess inherits this setting.  In this case, it\n> means diff-highlight will be launched with SIGPIPE being ignored.  Let's work\n> with those broken scripts by explicitly setting up a SIGPIPE handler and exiting\n> the process.\n\nMy first thought was that this should be handled already by 7559a1b\n(unblock and unignore SIGPIPE, 2014-09-18), but after re-reading your\nmessage, it sounds like you are using diff-highlight with non-git\nprograms?\n\n> +# Some scripts may not realize that SIGPIPE is being ignored when launching the\n> +# pager--for instance scripts written in Python.  Setting $SIG{PIPE} = 'DEFAULT'\n> +# doesn't work in these instances, so we install our own signal handler instead.\n\nWhy doesn't $SIG{PIPE} = 'DEFAULT' work? I did some limited testing and\nit seemed to work fine for me. Though I simulated the condition with:\n\n  (\n    trap '' PIPE\n    perl -e '$|=1; print \"foo\\n\"; print STDERR \"bar\\n\"'\n  ) | true\n\nwhich should not ever print \"bar\".\n\nIs Python doing something more aggressive, like using sigprocmask to\nblock the signal? I would think setting $SIG{PIPE} would handle that,\nbut maybe not in some versions of perl. I dunno. Modern perl\nsignal-handling is weird, as it catches everything and then defers\npropagation until a safe point in the script (if you strace the script\nabove, you can see that it actually gets EPIPE from the write call!)\nI've no clue what implications all that has for the case you're\naddressing.\n\n> +sub pipe_handler {\n> +    exit(0);\n> +}\n\nCan we exit 141 here? If we are part of a pipeline to a pager, it should\nnot matter either way, but I'd rather not lose the exit code if we can\navoid it (in case of running the script standalone).\n\n> +$SIG{PIPE} = \\&pipe_handler;\n\nA minor nit, but would:\n\n  $SIG{PIPE} = sub { ... };\n\nbe nicer to avoid polluting the function namespace?\n\n-Peff\n"},{"id":"251379","messageId":"CAEBDL5XUhEgbrGZHqrVu3d8QuhX_B9KT_h8pUW4LK_MWr-7KUQ@mail.gmail.com","threadId":"37856","inReplyTo":"20141101040443.GB8307@peff.net","subject":"Re: [PATCH] diff-highlight: exit when a pipe is broken","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2014-11-04T19:43:56Z","receivedAt":"2014-11-04T19:43:56Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Sat, Nov 1, 2014 at 12:04 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Oct 31, 2014 at 07:04:04AM -0400, John Szakmeister wrote:\n>\n>> While using diff-highlight with other tools, I have discovered that Python\n>> ignores SIGPIPE by default.  Unfortunately, this also means that tools\n>> attempting to launch a pager under Python--and don't realize this is\n>> happening--means that the subprocess inherits this setting.  In this case, it\n>> means diff-highlight will be launched with SIGPIPE being ignored.  Let's work\n>> with those broken scripts by explicitly setting up a SIGPIPE handler and exiting\n>> the process.\n>\n> My first thought was that this should be handled already by 7559a1b\n> (unblock and unignore SIGPIPE, 2014-09-18), but after re-reading your\n> message, it sounds like you are using diff-highlight with non-git\n> programs?\n\nYes, that's correct.  It's useful, so with a few tools that use diffs,\nI like to run the output through diff-highlight.\n\n>> +# Some scripts may not realize that SIGPIPE is being ignored when launching the\n>> +# pager--for instance scripts written in Python.  Setting $SIG{PIPE} = 'DEFAULT'\n>> +# doesn't work in these instances, so we install our own signal handler instead.\n>\n> Why doesn't $SIG{PIPE} = 'DEFAULT' work? I did some limited testing and\n> it seemed to work fine for me. Though I simulated the condition with:\n>\n>   (\n>     trap '' PIPE\n>     perl -e '$|=1; print \"foo\\n\"; print STDERR \"bar\\n\"'\n>   ) | true\n>\n> which should not ever print \"bar\".\n\nHehe, now that I see you right it out, I realize my mistake: I didn't\ncapitalize 'default'.  Trying it out again, it does appear that does\nthe trick.\n\n[snip]\n> Can we exit 141 here? If we are part of a pipeline to a pager, it should\n> not matter either way, but I'd rather not lose the exit code if we can\n> avoid it (in case of running the script standalone).\n>\n>> +$SIG{PIPE} = \\&pipe_handler;\n>\n> A minor nit, but would:\n>\n>   $SIG{PIPE} = sub { ... };\n>\n> be nicer to avoid polluting the function namespace?\n\nSorry, my Perl-fu is kind of low these days.  I used to use it all the\ntime but switched away from it quite a while ago.  Given that\n'DEFAULT' does the trick, I'll just re-roll my patch to use that.\nDoes that sound fair?\n\n-John\n\nPS  Sorry for the late response, I've been traveling.\n"}]}