{"thread":{"id":"32523","subject":"[RFC/PATCH] avoid SIGPIPE warnings for aliases","startedAt":"2013-01-04T12:47:56Z","lastAt":"2014-07-21T06:45:31Z","messageCount":23,"participants":["Jeff King","Johannes Sixt","Junio C Hamano","Jonathan Nieder","mimimimi"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"205967","messageId":"20130104124756.GA402@sigill.intra.peff.net","threadId":"32523","inReplyTo":null,"subject":"[RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-04T12:47:56Z","receivedAt":"2013-01-04T12:47:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When git executes an alias that specifies an external\ncommand, it will complain if the alias dies due to a signal.\nThis is usually a good thing, as signal deaths are\nunexpected. However, SIGPIPE is not unexpected for many\ncommands which produce a lot of output; it is intended that\nthe user closing the pager would kill them them via SIGPIPE.\n\nAs a result, the user might see annoying messages in a\nscenario like this:\n\n  $ cat ~/.gitconfig\n  [alias]\n  lgbase = log --some-options\n  lg = !git lgbase --more-options\n  lg2 = !git lgbase --other-options\n\n  $ git lg -p\n  [user hits 'q' to exit pager]\n  error: git lgbase --more-options died of signal 13\n  fatal: While expanding alias 'lg': 'git lgbase --more-options': Success\n\nMany users won't see this, because we execute the external\ncommand with the shell, and a POSIX shell will silently\nrewrite the signal-death exit code into 128+signal, and we\nwill treat it like a normal exit code. However, this does\nnot always happen:\n\n  1. If the sub-command does not have shell metacharacters,\n     we will skip the shell and exec it directly, getting\n     the signal code.\n\n  2. Some shells do not do this rewriting; for example,\n     setting SHELL_PATH set to zsh will reveal this problem.\n\nThis patch squelches the message, and let's git exit\nsilently (with the same exit code that a shell would use in\ncase of a signal).\n\nThe first line of the message comes from run-command's\nwait_or_whine. To silence that message, we remain quiet\nanytime we see SIGPIPE.\n\nThe second line comes from handle_alias itself. It calls\ndie_errno whenever run_command returns a negative value.\nHowever, only -1 indicates a syscall error where errno has\nsomething useful (note that it says the useless \"success\"\nabove). Instead, we treat negative returns from run_command\n(except for -1) as a normal code to be passed to exit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI have two reservations with this patch:\n\n  1. We are ignoring SIGPIPE all the time. For an alias that is calling\n     \"log\", that is fine. But if pack-objects dies on the server side,\n     seeing that it died from SIGPIPE is useful data, and we are\n     squelching that. Maybe callers of run-command should have to pass\n     an \"ignore SIGPIPE\" flag?\n\n  2. The die_errno in handle_alias is definitely wrong. Even if we want\n     to print a message for signal death, showing errno is bogus unless\n     the return value was -1. But is it the right thing to just pass the\n     negative value straight to exit()? It works, but it is depending on\n     the fact that (unsigned char)(ret & 0xff) behaves in a certain way\n     (i.e., that we are on a twos-complement platform, and -13 becomes\n     141). That is not strictly portable, but it is probably fine in\n     practice. I'd worry more about exit() doing something weird on\n     Windows.\n\n git.c         | 2 +-\n run-command.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex d33f9b3..07edb8a 100644\n--- a/git.c\n+++ b/git.c\n@@ -175,7 +175,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\talias_argv[argc] = NULL;\n \n \t\t\tret = run_command_v_opt(alias_argv, RUN_USING_SHELL);\n-\t\t\tif (ret >= 0)   /* normal exit */\n+\t\t\tif (ret != -1)  /* normal exit */\n \t\t\t\texit(ret);\n \n \t\t\tdie_errno(\"While expanding alias '%s': '%s'\",\ndiff --git a/run-command.c b/run-command.c\nindex 757f263..01a4c9b 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -242,7 +242,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t} else if (WIFSIGNALED(status)) {\n \t\tcode = WTERMSIG(status);\n-\t\tif (code != SIGINT && code != SIGQUIT)\n+\t\tif (code != SIGINT && code != SIGQUIT && code != SIGPIPE)\n \t\t\terror(\"%s died of signal %d\", argv0, code);\n \t\t/*\n \t\t * This return value is chosen so that code & 0xff\n-- \n1.8.1.rc1.16.g6d46841\n"},{"id":"205969","messageId":"50E70976.5040001@kdbg.org","threadId":"32523","inReplyTo":"20130104124756.GA402@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-01-04T16:55:18Z","receivedAt":"2013-01-04T16:55:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 04.01.2013 13:47, schrieb Jeff King:\n> I have two reservations with this patch:\n> \n>   1. We are ignoring SIGPIPE all the time. For an alias that is calling\n>      \"log\", that is fine. But if pack-objects dies on the server side,\n>      seeing that it died from SIGPIPE is useful data, and we are\n>      squelching that. Maybe callers of run-command should have to pass\n>      an \"ignore SIGPIPE\" flag?\n\nI am of two minds. On the one hand, losing useful debugging information\nis not something we should do lightly. On the other hand, the message is\nreally noise most of the time, even on servers: when pack-objects dies\non the server side, it is most likely due to a connection that breaks\n(voluntarily or involuntarily) half-way during a transfer, and is\npresumably a frequent event, and as such not worth noting most of the time.\n\n>   2. The die_errno in handle_alias is definitely wrong. Even if we want\n>      to print a message for signal death, showing errno is bogus unless\n>      the return value was -1. But is it the right thing to just pass the\n>      negative value straight to exit()? It works, but it is depending on\n>      the fact that (unsigned char)(ret & 0xff) behaves in a certain way\n>      (i.e., that we are on a twos-complement platform, and -13 becomes\n>      141). That is not strictly portable, but it is probably fine in\n>      practice. I'd worry more about exit() doing something weird on\n>      Windows.\n\nIt did something weird on Windows until we added this line to\ncompat/mingw.h:\n\n#define exit(code) exit((code) & 0xff)\n\n-- Hannes\n"},{"id":"205980","messageId":"20130104212534.GA2304@sigill.intra.peff.net","threadId":"32523","inReplyTo":"50E70976.5040001@kdbg.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-04T21:25:34Z","receivedAt":"2013-01-04T21:25:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 04, 2013 at 05:55:18PM +0100, Johannes Sixt wrote:\n\n> Am 04.01.2013 13:47, schrieb Jeff King:\n> > I have two reservations with this patch:\n> > \n> >   1. We are ignoring SIGPIPE all the time. For an alias that is calling\n> >      \"log\", that is fine. But if pack-objects dies on the server side,\n> >      seeing that it died from SIGPIPE is useful data, and we are\n> >      squelching that. Maybe callers of run-command should have to pass\n> >      an \"ignore SIGPIPE\" flag?\n> \n> I am of two minds. On the one hand, losing useful debugging information\n> is not something we should do lightly. On the other hand, the message is\n> really noise most of the time, even on servers: when pack-objects dies\n> on the server side, it is most likely due to a connection that breaks\n> (voluntarily or involuntarily) half-way during a transfer, and is\n> presumably a frequent event, and as such not worth noting most of the time.\n\nYeah. I'd mostly be worried about a case where pack-objects prints\nnothing (because it dies due to pipe), and then the outer process is not\nsufficiently verbose (it just says something like \"pack-objects died\nabnormally\", and the user is left scratching their head. I.e., it _is_\nuninteresting, but because we are too silent, the user does not even\nknow it is uninteresting.\n\nPack-objects is already careful to check all of its writes. I really\nthink it would be fine to just ignore SIGPIPE, and then it would produce\na useful error message on EPIPE. The downside is that if we accidentally\nhave an unchecked call, we won't notice the error (we'll probably notice\nit later, but we might continue uselessly spewing data in the meantime).\nPerhaps we should catch SIGPIPE in such programs and print an error\nmessage.\n\n> >   2. The die_errno in handle_alias is definitely wrong. Even if we want\n> >      to print a message for signal death, showing errno is bogus unless\n> >      the return value was -1. But is it the right thing to just pass the\n> >      negative value straight to exit()? It works, but it is depending on\n> >      the fact that (unsigned char)(ret & 0xff) behaves in a certain way\n> >      (i.e., that we are on a twos-complement platform, and -13 becomes\n> >      141). That is not strictly portable, but it is probably fine in\n> >      practice. I'd worry more about exit() doing something weird on\n> >      Windows.\n> \n> It did something weird on Windows until we added this line to\n> compat/mingw.h:\n> \n> #define exit(code) exit((code) & 0xff)\n\nAh, makes sense. I think that hunk of my patch is probably good, then.\n\n-Peff\n"},{"id":"205987","messageId":"7vsj6gsi7v.fsf@alter.siamese.dyndns.org","threadId":"32523","inReplyTo":"20130104124756.GA402@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-04T22:20:52Z","receivedAt":"2013-01-04T22:20:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I have two reservations with this patch:\n>\n>   1. We are ignoring SIGPIPE all the time. For an alias that is calling\n>      \"log\", that is fine. But if pack-objects dies on the server side,\n>      seeing that it died from SIGPIPE is useful data, and we are\n>      squelching that. Maybe callers of run-command should have to pass\n>      an \"ignore SIGPIPE\" flag?\n\nWhat should this do:\n\n    GIT_PAGER='head -n 1' git -p -c alias.o='!cat longfile' o\n\nShould it behave just like\n\n    cat longfile | head -n 1\n\nor should it behave differently?\n\nI am having a feeling that whatever external command given as the\nvalue of alias.$cmd should choose what error status it wants to be\nreported.\n\n>   2. The die_errno in handle_alias is definitely wrong. Even if we want\n>      to print a message for signal death, showing errno is bogus unless\n>      the return value was -1. But is it the right thing to just pass the\n>      negative value straight to exit()? It works, but it is depending on\n>      the fact that (unsigned char)(ret & 0xff) behaves in a certain way\n>      (i.e., that we are on a twos-complement platform, and -13 becomes\n>      141).\n\nIsn't that what POSIX.1 guarantees us, though?\n\n    The value of status may be 0, EXIT_SUCCESS, EXIT_FAILURE, or any\n    other value, though only the least significant 8 bits (that is,\n    status & 0377) shall be available to a waiting parent process.\n"},{"id":"206024","messageId":"20130105140316.GA7272@sigill.intra.peff.net","threadId":"32523","inReplyTo":"7vsj6gsi7v.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-05T14:03:16Z","receivedAt":"2013-01-05T14:03:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 04, 2013 at 02:20:52PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I have two reservations with this patch:\n> >\n> >   1. We are ignoring SIGPIPE all the time. For an alias that is calling\n> >      \"log\", that is fine. But if pack-objects dies on the server side,\n> >      seeing that it died from SIGPIPE is useful data, and we are\n> >      squelching that. Maybe callers of run-command should have to pass\n> >      an \"ignore SIGPIPE\" flag?\n> \n> What should this do:\n> \n>     GIT_PAGER='head -n 1' git -p -c alias.o='!cat longfile' o\n> \n> Should it behave just like\n> \n>     cat longfile | head -n 1\n> \n> or should it behave differently?\n\nWith respect to error messages, I'd think they should behave the same.\nBut they don't necessarily. The latter does not print any message at\nall. But consider this version of the former:\n\n  $ cat foo\n  #!/bin/sh\n  exec cat longfile\n\n  $ git -c alias.o='!./foo' o | head -n 1\n  error: ./foo died of signal 13\n  fatal: While expanding alias 'o': './foo': Success\n\nSo why don't we see that message more often? There are two reasons.\n\nOne reason is that we piped it ourselves here. When git pipes to the\npager, it sends stderr along the same channel. So even if you did:\n\n  GIT_PAGER='head -n 1' git -p -c alias.o='!./foo' o\n\ngit writes the error, but it goes to the pager, which has already ended\n(since that is what caused the SIGPIPE in the first place). But imagine\nthat your sub-command is actually invoking git itself, and it is the\nsub-git which starts the pager. Meaning the outer git wrapper's stderr\nis _not_ redirected. Like this:\n\n  $ cat foo\n  #!/bin/sh\n  exec git log -p\n\n  $ GIT_PAGER='head -n 1' git -c alias.o='!./foo' o\n  error: ./foo died of signal 13\n  fatal: While expanding alias 'o': './foo': Success\n\nThe second reason is that most shells will \"eat\" the SIGPIPE exit\nstatus, and convert it into a high, positive error code. You can see\nthat effect here:\n\n  $ GIT_PAGER='head -n 1' git log -p\n  $ echo $?\n  141\n\nAnd since we execute aliases via the shell, we end up seeing the\nconverted exit code (141) instead of the signal death. _Unless_ we\noptimize out the shell call (which is why we see it by putting the\ncommand inside \"./foo\", which git executes directly, but not when we\ngive the literal \"cat longfile\", which git will feed to the shell).\n\nOr at least that's _one_ way to see it. Another way is to use a shell\nthat does not do such conversion. Setting SHELL_PATH to zsh seems to do\nso, and I think that is how Bart ran into it (my patch is a followup to\na Google+ posting he made).\n\n> I am having a feeling that whatever external command given as the\n> value of alias.$cmd should choose what error status it wants to be\n> reported.\n\nI suppose. It means that our \"do not run the shell if there are no\nmeta-characters\" optimization is leaky, since the exit code behaves\ndifferently depending on whether we run the shell (and depending on your\nexact shell). One solution would be to fix that leakiness, and if\nuse_shell is in effect for run-command, to convert a signal death into\nthe value that the shell would otherwise give us.\n\nIn fact, I really wonder if this code from wait_or_whine is actually\ncorrect:\n\n  code = WTERMSIG(status);\n  /*\n   * This return value is chosen so that code & 0xff\n   * mimics the exit code that a POSIX shell would report for\n   * a program that died from this signal.\n   */\n  code -= 128;\n\nIf we get signal 13, we end up with -115, because \"code\" is signed. When\nthe lower 8 bits are taken, and then converted into an unsigned value,\nwe get 141: the shell value.\n\nBut do we really want to return a negative value here? Should this\ninstead be:\n\n  code += 128\n\nwhich yields the same code when fed to exit, but internally looks like\nthe shell version to us? So we get a consistent result whether the shell\nwas actually used or not.\n\nThat makes more sense to me, and would mean that whether we converted\nthe signal number or whether it was done by a subshell, it looks the\nsame to us. Callers which care about signals (e.g., the recent changes\nto launch_editor to detect SIGINT) would have to be adjusted. But I\nthink it fixes an obscure bug there. Right now launch_editor is actually\nchecking the whether the _shell_ died from a signal, and will fail to\nnotice when an editor invoked by the shell is killed by those signals\n(this would be pretty rare, though, because typically SIGINT is\ndelivered to the shell as well as the editor).\n\nThis would also fix the code in handle_alias. It looks for a negative\nreturn code from run_command as the sign that there was an internal\nerror running the command, and that errno would be valid. But right now\na negative return can also come from signal death.\n\n> >   2. The die_errno in handle_alias is definitely wrong. Even if we want\n> >      to print a message for signal death, showing errno is bogus unless\n> >      the return value was -1. But is it the right thing to just pass the\n> >      negative value straight to exit()? It works, but it is depending on\n> >      the fact that (unsigned char)(ret & 0xff) behaves in a certain way\n> >      (i.e., that we are on a twos-complement platform, and -13 becomes\n> >      141).\n> \n> Isn't that what POSIX.1 guarantees us, though?\n> \n>     The value of status may be 0, EXIT_SUCCESS, EXIT_FAILURE, or any\n>     other value, though only the least significant 8 bits (that is,\n>     status & 0377) shall be available to a waiting parent process.\n\nSort of. I was worried about:\n\n  1. Not-quite-POSIX platforms (i.e., Windows). But JSixt has said that\n     is fine, because we already have a compatibility wrapper which\n     masks off only the low byte.\n\n  2. We are relying on the specifics of how a negative value is treated\n     by exit(). The cast I gave above is guaranteed to work in standard\n     C, but we do not know the implementation details of exit(). Still,\n     I think that is being overly paranoid. Any sane implementation will\n     do what we expect.\n\n-Peff\n"},{"id":"206027","messageId":"20130105144949.GA24479@sigill.intra.peff.net","threadId":"32523","inReplyTo":"20130105140316.GA7272@sigill.intra.peff.net","subject":"[PATCH] run-command: encode signal death as a positive integer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-05T14:49:49Z","receivedAt":"2013-01-05T14:49:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:\n\n> In fact, I really wonder if this code from wait_or_whine is actually\n> correct:\n> \n>   code = WTERMSIG(status);\n>   /*\n>    * This return value is chosen so that code & 0xff\n>    * mimics the exit code that a POSIX shell would report for\n>    * a program that died from this signal.\n>    */\n>   code -= 128;\n\nAfter looking at it some more, it is correct, but I think we could make\nlife slightly easier for callers. See the patch below.  I've tried to\nre-state the somewhat rambling argument from my previous email;\nhopefully it makes sense.\n\n-- >8 --\nSubject: [PATCH] run-command: encode signal death as a positive integer\n\nWhen a sub-command dies due to a signal, we encode the\nsignal number into the numeric exit status as \"signal -\n128\". This is easy to identify (versus a regular positive\nerror code), and when cast to an unsigned integer (e.g., by\nfeeding it to exit), matches what a POSIX shell would return\nwhen reporting a signal death in $? or through its own exit\ncode.\n\nSo we have a negative value inside the code, but once it\npasses across an exit() barrier, it looks positive (and any\ncode we receive from a sub-shell will have the positive\nform). E.g., death by SIGPIPE (signal 13) will look like\n-115 to us in inside git, but will end up as 141 when we\ncall exit() with it. And a program killed by SIGPIPE but run\nvia the shell will come to us with an exit code of 141.\n\nUnfortunately, this means that when the \"use_shell\" option\nis set, we need to be on the lookout for _both_ forms. We\nmight or might not have actually invoked the shell (because\nwe optimize out some useless shell calls). If we didn't invoke\nthe shell, we will will see the sub-process's signal death\ndirectly, and run-command converts it into a negative value.\nBut if we did invoke the shell, we will see the shell's\n128+signal exit status. To be thorough, we would need to\ncheck both, or cast the value to an unsigned char (after\nchecking that it is not -1, which is a magic error value).\n\nFortunately, most callsites do not care at all whether the\nexit was from a code or from a signal; they merely check for\na non-zero status, and sometimes propagate the error via\nexit(). But for the callers that do care, we can make life\nslightly easier by just using the consistent positive form.\n\nThis actually fixes two minor bugs:\n\n  1. In launch_editor, we check whether the editor died from\n     SIGINT or SIGQUIT. But we checked only the negative\n     form, meaning that we would fail to notice a signal\n     death exit code which was propagated through the shell.\n\n  2. In handle_alias, we assume that a negative return value\n     from run_command means that errno tells us something\n     interesting (like a fork failure, or ENOENT).\n     Otherwise, we simply propagate the exit code. Negative\n     signal death codes confuse us, and we print a useless\n     \"unable to run alias 'foo': Success\" message. By\n     encoding signal deaths using the positive form, the\n     existing code just propagates it as it would a normal\n     non-zero exit code.\n\nThe downside is that callers of run_command can no longer\ndifferentiate between a signal received directly by the\nsub-process, and one propagated. However, no caller\ncurrently cares, and since we already optimize out some\ncalls to the shell under the hood, that distinction is not\nsomething that should be relied upon by callers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/technical/api-run-command.txt | 6 ++----\n editor.c                                    | 2 +-\n run-command.c                               | 2 +-\n 3 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\nindex f18b4f4..5d7d7f2 100644\n--- a/Documentation/technical/api-run-command.txt\n+++ b/Documentation/technical/api-run-command.txt\n@@ -55,10 +55,8 @@ The functions above do the following:\n   non-zero.\n \n . If the program terminated due to a signal, then the return value is the\n-  signal number - 128, ie. it is negative and so indicates an unusual\n-  condition; a diagnostic is printed. This return value can be passed to\n-  exit(2), which will report the same code to the parent process that a\n-  POSIX shell's $? would report for a program that died from the signal.\n+  signal number + 128, ie. the same value that a POSIX shell's $? would\n+  report.  A diagnostic is printed.\n \n \n `start_async`::\ndiff --git a/editor.c b/editor.c\nindex 065a7ab..27bdecd 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n-\t\tsig = ret + 128;\n+\t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n \t\tsigchain_pop(SIGQUIT);\n \t\tif (sig == SIGINT || sig == SIGQUIT)\ndiff --git a/run-command.c b/run-command.c\nindex 757f263..cfb7274 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \t\t * mimics the exit code that a POSIX shell would report for\n \t\t * a program that died from this signal.\n \t\t */\n-\t\tcode -= 128;\n+\t\tcode += 128;\n \t} else if (WIFEXITED(status)) {\n \t\tcode = WEXITSTATUS(status);\n \t\t/*\n-- \n1.8.1.rc1.16.g6d46841\n"},{"id":"206042","messageId":"50E88421.9030605@kdbg.org","threadId":"32523","inReplyTo":"20130105144949.GA24479@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: encode signal death as a positive integer","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-01-05T19:50:57Z","receivedAt":"2013-01-05T19:50:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.01.2013 15:49, schrieb Jeff King:\n> On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:\n> \n>> In fact, I really wonder if this code from wait_or_whine is actually\n>> correct:\n>>\n>>   code = WTERMSIG(status);\n>>   /*\n>>    * This return value is chosen so that code & 0xff\n>>    * mimics the exit code that a POSIX shell would report for\n>>    * a program that died from this signal.\n>>    */\n>>   code -= 128;\n> \n> After looking at it some more, it is correct, but I think we could make\n> life slightly easier for callers. See the patch below.  I've tried to\n> re-state the somewhat rambling argument from my previous email;\n> hopefully it makes sense.\n> \n> -- >8 --\n> Subject: [PATCH] run-command: encode signal death as a positive integer\n> \n> When a sub-command dies due to a signal, we encode the\n> signal number into the numeric exit status as \"signal -\n> 128\". This is easy to identify (versus a regular positive\n> error code), and when cast to an unsigned integer (e.g., by\n> feeding it to exit), matches what a POSIX shell would return\n> when reporting a signal death in $? or through its own exit\n> code.\n> \n> So we have a negative value inside the code, but once it\n> passes across an exit() barrier, it looks positive (and any\n> code we receive from a sub-shell will have the positive\n> form). E.g., death by SIGPIPE (signal 13) will look like\n> -115 to us in inside git, but will end up as 141 when we\n> call exit() with it. And a program killed by SIGPIPE but run\n> via the shell will come to us with an exit code of 141.\n> \n> Unfortunately, this means that when the \"use_shell\" option\n> is set, we need to be on the lookout for _both_ forms. We\n> might or might not have actually invoked the shell (because\n> we optimize out some useless shell calls). If we didn't invoke\n> the shell, we will will see the sub-process's signal death\n> directly, and run-command converts it into a negative value.\n> But if we did invoke the shell, we will see the shell's\n> 128+signal exit status. To be thorough, we would need to\n> check both, or cast the value to an unsigned char (after\n> checking that it is not -1, which is a magic error value).\n> \n> Fortunately, most callsites do not care at all whether the\n> exit was from a code or from a signal; they merely check for\n> a non-zero status, and sometimes propagate the error via\n> exit(). But for the callers that do care, we can make life\n> slightly easier by just using the consistent positive form.\n> \n> This actually fixes two minor bugs:\n> \n>   1. In launch_editor, we check whether the editor died from\n>      SIGINT or SIGQUIT. But we checked only the negative\n>      form, meaning that we would fail to notice a signal\n>      death exit code which was propagated through the shell.\n> \n>   2. In handle_alias, we assume that a negative return value\n>      from run_command means that errno tells us something\n>      interesting (like a fork failure, or ENOENT).\n>      Otherwise, we simply propagate the exit code. Negative\n>      signal death codes confuse us, and we print a useless\n>      \"unable to run alias 'foo': Success\" message. By\n>      encoding signal deaths using the positive form, the\n>      existing code just propagates it as it would a normal\n>      non-zero exit code.\n> \n> The downside is that callers of run_command can no longer\n> differentiate between a signal received directly by the\n> sub-process, and one propagated. However, no caller\n> currently cares, and since we already optimize out some\n> calls to the shell under the hood, that distinction is not\n> something that should be relied upon by callers.\n\nThe idea was initially to regard death by signal as an internal error.\nBut since (1) there are no callers that are really interested in the\ndifference and (2) we get it wrong in the shell case anyway, this change\nmakes total sense.\n\nAcked-by: Johannes Sixt <j6t@kdbg.org>\n\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Documentation/technical/api-run-command.txt | 6 ++----\n>  editor.c                                    | 2 +-\n>  run-command.c                               | 2 +-\n>  3 files changed, 4 insertions(+), 6 deletions(-)\n> \n> diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\n> index f18b4f4..5d7d7f2 100644\n> --- a/Documentation/technical/api-run-command.txt\n> +++ b/Documentation/technical/api-run-command.txt\n> @@ -55,10 +55,8 @@ The functions above do the following:\n>    non-zero.\n>  \n>  . If the program terminated due to a signal, then the return value is the\n> -  signal number - 128, ie. it is negative and so indicates an unusual\n> -  condition; a diagnostic is printed. This return value can be passed to\n> -  exit(2), which will report the same code to the parent process that a\n> -  POSIX shell's $? would report for a program that died from the signal.\n> +  signal number + 128, ie. the same value that a POSIX shell's $? would\n> +  report.  A diagnostic is printed.\n>  \n>  \n>  `start_async`::\n> diff --git a/editor.c b/editor.c\n> index 065a7ab..27bdecd 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n>  \t\tsigchain_push(SIGINT, SIG_IGN);\n>  \t\tsigchain_push(SIGQUIT, SIG_IGN);\n>  \t\tret = finish_command(&p);\n> -\t\tsig = ret + 128;\n> +\t\tsig = ret - 128;\n>  \t\tsigchain_pop(SIGINT);\n>  \t\tsigchain_pop(SIGQUIT);\n>  \t\tif (sig == SIGINT || sig == SIGQUIT)\n> diff --git a/run-command.c b/run-command.c\n> index 757f263..cfb7274 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n>  \t\t * mimics the exit code that a POSIX shell would report for\n>  \t\t * a program that died from this signal.\n>  \t\t */\n> -\t\tcode -= 128;\n> +\t\tcode += 128;\n>  \t} else if (WIFEXITED(status)) {\n>  \t\tcode = WEXITSTATUS(status);\n>  \t\t/*\n> \n"},{"id":"206063","messageId":"20130105221909.GA3247@elie.Belkin","threadId":"32523","inReplyTo":"20130105144949.GA24479@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: encode signal death as a positive integer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-05T22:19:09Z","receivedAt":"2013-01-05T22:19:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> When a sub-command dies due to a signal, we encode the\n> signal number into the numeric exit status as \"signal -\n> 128\".\n[...]\n> So we have a negative value inside the code, but once it\n> passes across an exit() barrier, it looks positive (and any\n> code we receive from a sub-shell will have the positive\n> form).\n[...]\n> Unfortunately, this means that when the \"use_shell\" option\n> is set, we need to be on the lookout for _both_ forms.\n[...]\n>             for the callers that do care, we can make life\n> slightly easier by just using the consistent positive form.\n\nMakes perfect sense.\n\n>  Documentation/technical/api-run-command.txt | 6 ++----\n>  editor.c                                    | 2 +-\n>  run-command.c                               | 2 +-\n>  3 files changed, 4 insertions(+), 6 deletions(-)\n\nt/test-terminal.perl imitates the same logic.  It doesn't check for\nanything other than whether the exit status is 0, but maybe it would\nbe worth squashing in the below as a futureproofing measure\nnonetheless.\n\nAside from the launch_editor bugfix, the only observable effects of\nthe above patch I can find are some changed error messages:\n\n\terror: external filter cat failed -126\n\t-> error: external filter cat failed 130\n\n\twarning: svnrdump, returned -126\n\t-> warning: svnrdump, returned 130\n\nThose messages are equally senseless before and after the patch, so\nfor what it's worth,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\ndiff --git i/t/test-terminal.perl w/t/test-terminal.perl\nindex 10172aee..1fb373f2 100755\n--- i/t/test-terminal.perl\n+++ w/t/test-terminal.perl\n@@ -31,7 +31,7 @@ sub finish_child {\n \t} elsif ($? & 127) {\n \t\tmy $code = $? & 127;\n \t\twarn \"died of signal $code\";\n-\t\treturn $code - 128;\n+\t\treturn $code + 128;\n \t} else {\n \t\treturn $? >> 8;\n \t}\n"},{"id":"206071","messageId":"20130105231248.GA4140@sigill.intra.peff.net","threadId":"32523","inReplyTo":"20130105221909.GA3247@elie.Belkin","subject":"Re: [PATCH] run-command: encode signal death as a positive integer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-05T23:12:49Z","receivedAt":"2013-01-05T23:12:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 05, 2013 at 02:19:09PM -0800, Jonathan Nieder wrote:\n\n> >  Documentation/technical/api-run-command.txt | 6 ++----\n> >  editor.c                                    | 2 +-\n> >  run-command.c                               | 2 +-\n> >  3 files changed, 4 insertions(+), 6 deletions(-)\n> \n> t/test-terminal.perl imitates the same logic.  It doesn't check for\n> anything other than whether the exit status is 0, but maybe it would\n> be worth squashing in the below as a futureproofing measure\n> nonetheless.\n\nYeah, I think so. As you say, it does not matter, but it makes sense to\nkeep our conventions consistent.\n\n> Aside from the launch_editor bugfix, the only observable effects of\n> the above patch I can find are some changed error messages:\n> \n> \terror: external filter cat failed -126\n> \t-> error: external filter cat failed 130\n> \n> \twarning: svnrdump, returned -126\n> \t-> warning: svnrdump, returned 130\n> \n> Those messages are equally senseless before and after the patch, so\n> for what it's worth,\n\nThanks. I agree that change isn't a big deal (I would argue the positive\nreturn is slightly more coherent as it matches what the shell would\nreport, but I really think user-facing errors should probably not even\nmention the exact exit code, as it is just noise most of the time, and\nwe already complain about signals in wait_or_whine).\n\nI did try auditing the callers of finish_command (and run_command) to\nmake sure I wasn't regressing anybody, but there are a lot of call\nsites. In some cases we immediately say:\n\n  if (finish_command(&child))\n          die(\"failed...\");\n\nwhich is obviously unaffected. But in many cases we pass the exit code\nup through several levels. It usually just ends up in exit() or being\ncollapsed to an error boolean, which is fine, but I may have missed a\nspot where it matters. I'd expecting cooking this patch for a while\nwould flush out any I missed.\n\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks (and to J6t) for the review.\n\n-Peff\n"},{"id":"206076","messageId":"20130105235844.GC5686@elie.Belkin","threadId":"32523","inReplyTo":"20130105231248.GA4140@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: encode signal death as a positive integer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-05T23:58:44Z","receivedAt":"2013-01-05T23:58:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n>                        I'd expecting cooking this patch for a while\n> would flush out any I missed.\n\nHeh, probably not. ;-)  But I tried to examine all the callsites (and\nonly found the two messages I mentioned), and among the reviewers, I'm\nguessing we hit them all.\n\nCiao,\nJonathan\n"},{"id":"206114","messageId":"7v38yepz8t.fsf@alter.siamese.dyndns.org","threadId":"32523","inReplyTo":"20130105144949.GA24479@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: encode signal death as a positive integer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-06T07:05:54Z","receivedAt":"2013-01-06T07:05:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Jan 05, 2013 at 09:03:16AM -0500, Jeff King wrote:\n> ...\n> The downside is that callers of run_command can no longer\n> differentiate between a signal received directly by the\n> sub-process, and one propagated. However, no caller\n> currently cares, and since we already optimize out some\n> calls to the shell under the hood, that distinction is not\n> something that should be relied upon by callers.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nVery nicely explained.  Thanks.\n\n>  Documentation/technical/api-run-command.txt | 6 ++----\n>  editor.c                                    | 2 +-\n>  run-command.c                               | 2 +-\n>  3 files changed, 4 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\n> index f18b4f4..5d7d7f2 100644\n> --- a/Documentation/technical/api-run-command.txt\n> +++ b/Documentation/technical/api-run-command.txt\n> @@ -55,10 +55,8 @@ The functions above do the following:\n>    non-zero.\n>  \n>  . If the program terminated due to a signal, then the return value is the\n> -  signal number - 128, ie. it is negative and so indicates an unusual\n> -  condition; a diagnostic is printed. This return value can be passed to\n> -  exit(2), which will report the same code to the parent process that a\n> -  POSIX shell's $? would report for a program that died from the signal.\n> +  signal number + 128, ie. the same value that a POSIX shell's $? would\n> +  report.  A diagnostic is printed.\n>  \n>  \n>  `start_async`::\n> diff --git a/editor.c b/editor.c\n> index 065a7ab..27bdecd 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -51,7 +51,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n>  \t\tsigchain_push(SIGINT, SIG_IGN);\n>  \t\tsigchain_push(SIGQUIT, SIG_IGN);\n>  \t\tret = finish_command(&p);\n> -\t\tsig = ret + 128;\n> +\t\tsig = ret - 128;\n>  \t\tsigchain_pop(SIGINT);\n>  \t\tsigchain_pop(SIGQUIT);\n>  \t\tif (sig == SIGINT || sig == SIGQUIT)\n> diff --git a/run-command.c b/run-command.c\n> index 757f263..cfb7274 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -249,7 +249,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n>  \t\t * mimics the exit code that a POSIX shell would report for\n>  \t\t * a program that died from this signal.\n>  \t\t */\n> -\t\tcode -= 128;\n> +\t\tcode += 128;\n>  \t} else if (WIFEXITED(status)) {\n>  \t\tcode = WEXITSTATUS(status);\n>  \t\t/*\n"},{"id":"206423","messageId":"7vr4lu3wx7.fsf@alter.siamese.dyndns.org","threadId":"32523","inReplyTo":"20130104124756.GA402@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-09T20:48:20Z","receivedAt":"2013-01-09T20:48:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When git executes an alias that specifies an external\n> command, it will complain if the alias dies due to a signal.\n> This is usually a good thing, as signal deaths are\n> unexpected. However, SIGPIPE is not unexpected for many\n> commands which produce a lot of output; it is intended that\n> the user closing the pager would kill them them via SIGPIPE.\n>\n> As a result, the user might see annoying messages in a\n> scenario like this:\n>\n>   $ cat ~/.gitconfig\n>   [alias]\n>   lgbase = log --some-options\n>   lg = !git lgbase --more-options\n>   lg2 = !git lgbase --other-options\n>\n>   $ git lg -p\n>   [user hits 'q' to exit pager]\n>   error: git lgbase --more-options died of signal 13\n>   fatal: While expanding alias 'lg': 'git lgbase --more-options': Success\n>\n> Many users won't see this, because we execute the external\n> command with the shell, and a POSIX shell will silently\n> rewrite the signal-death exit code into 128+signal, and we\n> will treat it like a normal exit code. However, this does\n> not always happen:\n\nSo... with the \"flip the sign of the exit code when caught a signal\"\npatch applied to 'next', do people still see this issue?\n"},{"id":"206424","messageId":"20130109205116.GA24605@sigill.intra.peff.net","threadId":"32523","inReplyTo":"7vr4lu3wx7.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-09T20:51:16Z","receivedAt":"2013-01-09T20:51:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 09, 2013 at 12:48:20PM -0800, Junio C Hamano wrote:\n\n> >   $ git lg -p\n> >   [user hits 'q' to exit pager]\n> >   error: git lgbase --more-options died of signal 13\n> >   fatal: While expanding alias 'lg': 'git lgbase --more-options': Success\n> >\n> > Many users won't see this, because we execute the external\n> > command with the shell, and a POSIX shell will silently\n> > rewrite the signal-death exit code into 128+signal, and we\n> > will treat it like a normal exit code. However, this does\n> > not always happen:\n> \n> So... with the \"flip the sign of the exit code when caught a signal\"\n> patch applied to 'next', do people still see this issue?\n\nThey see half. The patch you've applied clears up the \"While\nexpanding...: Success\" message.\n\nBut we still say \"error: ... died of signal 13\", because that comes from\ninside wait_or_whine. So it is a separate issue whether or not\nwait_or_whine should be silent on SIGPIPE (we already are on SIGINT and\nSIGQUIT, as of some recent patches).\n\nThe upside is that it is noise in this case that we would no longer see.\nThe downside is that we may be losing a clue when debugging server\nproblems, which do not expect to die from SIGPIPE.  Should it be an\noptional run-command flag?\n\n-Peff\n"},{"id":"206430","messageId":"7vehhu3u2y.fsf@alter.siamese.dyndns.org","threadId":"32523","inReplyTo":"20130109205116.GA24605@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-09T21:49:41Z","receivedAt":"2013-01-09T21:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But we still say \"error: ... died of signal 13\", because that comes from\n> inside wait_or_whine. So it is a separate issue whether or not\n> wait_or_whine should be silent on SIGPIPE (we already are on SIGINT and\n> SIGQUIT, as of some recent patches).\n>\n> The upside is that it is noise in this case that we would no longer see.\n> The downside is that we may be losing a clue when debugging server\n> problems, which do not expect to die from SIGPIPE.  Should it be an\n> optional run-command flag?\n\nDo we know if we are upstream of a pager that reads from us through\na pipe (I think we should, especially in a case where we are the one\nwho processed the \"git -p $alias\" option)?  Is there any other case\nwhere we would want to ignore child's death by SIGPIPE?  If the\nanswers are yes and no, then perhaps we can ask pager_in_use() to\ndecide this?\n"},{"id":"206437","messageId":"20130110001844.GC21054@google.com","threadId":"32523","inReplyTo":"7vehhu3u2y.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-10T00:18:44Z","receivedAt":"2013-01-10T00:18:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n\n>> But we still say \"error: ... died of signal 13\", because that comes from\n>> inside wait_or_whine. So it is a separate issue whether or not\n>> wait_or_whine should be silent on SIGPIPE (we already are on SIGINT and\n>> SIGQUIT, as of some recent patches).\n>>\n>> The upside is that it is noise in this case that we would no longer see.\n>> The downside is that we may be losing a clue when debugging server\n>> problems, which do not expect to die from SIGPIPE.  Should it be an\n>> optional run-command flag?\n>\n> Do we know if we are upstream of a pager that reads from us through\n> a pipe (I think we should, especially in a case where we are the one\n> who processed the \"git -p $alias\" option)?  Is there any other case\n> where we would want to ignore child's death by SIGPIPE?\n\nWhen we die early by SIGPIPE because output was piped to \"head\", I\nstill think the early end of output is not notable enough to complain\nabout.\n\nI'm not sure whether there are SIGPIPE instances we really don't want\nto be silent about, though.  I suspect not. ;-)\n\nCompare <http://thread.gmane.org/gmane.comp.version-control.git/2062>,\n<http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48665>.\n\nThanks,\nJonathan\n"},{"id":"206439","messageId":"7vobgx3m8o.fsf@alter.siamese.dyndns.org","threadId":"32523","inReplyTo":"20130110001844.GC21054@google.com","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-10T00:39:03Z","receivedAt":"2013-01-10T00:39:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> I'm not sure whether there are SIGPIPE instances we really don't want\n> to be silent about, though.  I suspect not. ;-)\n>\n> Compare <http://thread.gmane.org/gmane.comp.version-control.git/2062>,\n> <http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48665>.\n\nYeah, thanks for the pointer to 48665.  Quoting from there:\n\n    So EPIPE really _is_ special: because when you write to a pipe,\n    there's no guarantee that you'll get it at all, so whenever you get\n    an EPIPE you should ask yourself:\n\n     - what would I have done if the data had fit in the 64kB kernel\n       buffer?\n\n     - should I really return a different error message or complain just \n       because I just happened to notice that the reader went away\n       _this_ \n       time, even if I might not notice it next time?\n\n    In other words, the \"exit(0)\" is actually _more_ consistent than\n    \"exit(1)\", because exiting with an error message or with an error\n    return is going to depend on luck and timing.\n\nand I think I still agree with the analysis and conclusion:\n\n    So what _should_ you do for EPIPE?\n\n    Here's what EPIPE _really_ means:\n\n     - you might as well consider the write a success, but the\n       reader isn't actually interested, so rather than go on, you\n       might as well stop early.\n\n    Notice how I very carefull avoided the word \"error\" anywhere.\n    Because it's really not an error. The reader already got\n    everything it wanted. So EPIPE should generally be seen as an\n    \"early success\" rather than as a \"failure\".\n"},{"id":"206448","messageId":"20130110104930.GA21993@sigill.intra.peff.net","threadId":"32523","inReplyTo":"7vehhu3u2y.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-10T10:49:30Z","receivedAt":"2013-01-10T10:49:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 09, 2013 at 01:49:41PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > But we still say \"error: ... died of signal 13\", because that comes from\n> > inside wait_or_whine. So it is a separate issue whether or not\n> > wait_or_whine should be silent on SIGPIPE (we already are on SIGINT and\n> > SIGQUIT, as of some recent patches).\n> >\n> > The upside is that it is noise in this case that we would no longer see.\n> > The downside is that we may be losing a clue when debugging server\n> > problems, which do not expect to die from SIGPIPE.  Should it be an\n> > optional run-command flag?\n> \n> Do we know if we are upstream of a pager that reads from us through\n> a pipe (I think we should, especially in a case where we are the one\n> who processed the \"git -p $alias\" option)?  Is there any other case\n> where we would want to ignore child's death by SIGPIPE?  If the\n> answers are yes and no, then perhaps we can ask pager_in_use() to\n> decide this?\n\nThe answer to the first is unfortunately no. Consider an alias like\n\"[alias]foo = !git log\" (which yes, you could implement as just \"log\",\nbut there are cases where you want to manipulate the environment and we\ndo not allow it).\n\nYour process tree for running \"git foo\" looks like:\n\n  git foo               (A)\n    git log             (B)\n      less              (C)\n\nThe user hits 'q', which kills process C. Process B then dies due to\nSIGPIPE, and process A sees that the alias command died due to a signal.\nBut process A has no clue that a pager is in effect; only process B,\nwhich spawned the pager, can know that. So A cannot see death-by-SIGPIPE\nand make a decision on whether a pager was in use.\n\nIf anything, it is process B's responsibility to say \"Oops, I was killed\nby SIGPIPE. But that's OK, it's not a big deal to me\". Which it could do\nby installing a SIGPIPE handler that just calls exit(0).\n\n-Peff\n"},{"id":"206451","messageId":"20130110112655.GB21993@sigill.intra.peff.net","threadId":"32523","inReplyTo":"20130110001844.GC21054@google.com","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-10T11:26:55Z","receivedAt":"2013-01-10T11:26:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 09, 2013 at 04:18:44PM -0800, Jonathan Nieder wrote:\n\n> > Do we know if we are upstream of a pager that reads from us through\n> > a pipe (I think we should, especially in a case where we are the one\n> > who processed the \"git -p $alias\" option)?  Is there any other case\n> > where we would want to ignore child's death by SIGPIPE?\n> \n> When we die early by SIGPIPE because output was piped to \"head\", I\n> still think the early end of output is not notable enough to complain\n> about.\n> \n> I'm not sure whether there are SIGPIPE instances we really don't want\n> to be silent about, though.  I suspect not. ;-)\n\nSome of our plumbing writes over pipes (e.g., pack-objects writing back\nto send-pack, which is multiplexing over the network, or receive-pack\nwriting to index-pack). We _should_ be checking the value of every\nwrite(), and your final close(), and making sure that sub-processes\nreports success. But leaving SIGPIPE on is an extra safety measure; in\ntheory it can catch an unchecked write.\n\nWhen one of those programs goes wrong, the message can be an extra\ndebugging aid. If the process died unexpectedly with no message (since\nit died by signal), seeing \"pack-objects died by signal 13\" is much\nbetter than not seeing anything at all. Usually it is accompanied by\nother messages (like \"remote end hung up unexpectedly\" or similar), but\nthe extra output has helped me track down server-side issues in the\npast.\n\n> Compare <http://thread.gmane.org/gmane.comp.version-control.git/2062>,\n> <http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48665>.\n\nThe argument there seems to be that bash is stupid for complaining about\nSIGPIPE. And I would agree for the \"alias\" case, where we are running\ncommands from the command-line in a very shell-like manner. But\nwait_or_whine is also used for connecting internal bits together.\n\nMaybe the right rule is \"if we are using the shell to execute, do not\nmention SIGPIPE\"? It seems a little iffy at first, but:\n\n  1. It tends to coincide with direct use of internal tools versus\n     external tools.\n\n  2. We do not reliably get SIGPIPE there, anyway, since most shells\n     will convert it into exit code 141 before we see it.\n\nI.e., something like:\n\ndiff --git a/run-command.c b/run-command.c\nindex 24eaad5..8bd0b08 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0)\n+static int wait_or_whine(pid_t pid, const char *argv0, int ignore_sigpipe)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n@@ -242,7 +242,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t} else if (WIFSIGNALED(status)) {\n \t\tcode = WTERMSIG(status);\n-\t\tif (code != SIGINT && code != SIGQUIT)\n+\t\tif (code != SIGINT && code != SIGQUIT &&\n+\t\t    (!ignore_sigpipe || code != SIGPIPE))\n \t\t\terror(\"%s died of signal %d\", argv0, code);\n \t\t/*\n \t\t * This return value is chosen so that code & 0xff\n@@ -433,7 +434,7 @@ fail_pipe:\n \t\t * At this point we know that fork() succeeded, but execvp()\n \t\t * failed. Errors have been reported to our stderr.\n \t\t */\n-\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n+\t\twait_or_whine(cmd->pid, cmd->argv[0], 0);\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n \t}\n@@ -538,7 +539,7 @@ int finish_command(struct child_process *cmd)\n \n int finish_command(struct child_process *cmd)\n {\n-\treturn wait_or_whine(cmd->pid, cmd->argv[0]);\n+\treturn wait_or_whine(cmd->pid, cmd->argv[0], cmd->use_shell);\n }\n \n int run_command(struct child_process *cmd)\n@@ -725,7 +726,7 @@ int finish_async(struct async *async)\n int finish_async(struct async *async)\n {\n #ifdef NO_PTHREADS\n-\treturn wait_or_whine(async->pid, \"child process\");\n+\treturn wait_or_whine(async->pid, \"child process\", 0);\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\n \n"},{"id":"206466","messageId":"7vbocw23fq.fsf@alter.siamese.dyndns.org","threadId":"32523","inReplyTo":"20130110112655.GB21993@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-10T20:22:49Z","receivedAt":"2013-01-10T20:22:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Maybe the right rule is \"if we are using the shell to execute, do not\n> mention SIGPIPE\"? It seems a little iffy at first, but:\n>\n>   1. It tends to coincide with direct use of internal tools versus\n>      external tools.\n>\n>   2. We do not reliably get SIGPIPE there, anyway, since most shells\n>      will convert it into exit code 141 before we see it.\n>\n> I.e., something like:\n\nHmph.  That may be a good heuristics, but I wonder if we also want\nto special case WIFEXITED(status) && WEXITSTATUS(status) == 141 to\npretend as if nothing went wrong, when ignore_sigpipe is in effect?\n\n> diff --git a/run-command.c b/run-command.c\n> index 24eaad5..8bd0b08 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)\n>  \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n>  }\n>  \n> -static int wait_or_whine(pid_t pid, const char *argv0)\n> +static int wait_or_whine(pid_t pid, const char *argv0, int ignore_sigpipe)\n>  {\n>  \tint status, code = -1;\n>  \tpid_t waiting;\n> @@ -242,7 +242,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n>  \t\terror(\"waitpid is confused (%s)\", argv0);\n>  \t} else if (WIFSIGNALED(status)) {\n>  \t\tcode = WTERMSIG(status);\n> -\t\tif (code != SIGINT && code != SIGQUIT)\n> +\t\tif (code != SIGINT && code != SIGQUIT &&\n> +\t\t    (!ignore_sigpipe || code != SIGPIPE))\n>  \t\t\terror(\"%s died of signal %d\", argv0, code);\n>  \t\t/*\n>  \t\t * This return value is chosen so that code & 0xff\n> @@ -433,7 +434,7 @@ fail_pipe:\n>  \t\t * At this point we know that fork() succeeded, but execvp()\n>  \t\t * failed. Errors have been reported to our stderr.\n>  \t\t */\n> -\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n> +\t\twait_or_whine(cmd->pid, cmd->argv[0], 0);\n>  \t\tfailed_errno = errno;\n>  \t\tcmd->pid = -1;\n>  \t}\n> @@ -538,7 +539,7 @@ int finish_command(struct child_process *cmd)\n>  \n>  int finish_command(struct child_process *cmd)\n>  {\n> -\treturn wait_or_whine(cmd->pid, cmd->argv[0]);\n> +\treturn wait_or_whine(cmd->pid, cmd->argv[0], cmd->use_shell);\n>  }\n>  \n>  int run_command(struct child_process *cmd)\n> @@ -725,7 +726,7 @@ int finish_async(struct async *async)\n>  int finish_async(struct async *async)\n>  {\n>  #ifdef NO_PTHREADS\n> -\treturn wait_or_whine(async->pid, \"child process\");\n> +\treturn wait_or_whine(async->pid, \"child process\", 0);\n>  #else\n>  \tvoid *ret = (void *)(intptr_t)(-1);\n>  \n"},{"id":"206471","messageId":"20130110213937.GB26138@sigill.intra.peff.net","threadId":"32523","inReplyTo":"7vbocw23fq.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-10T21:39:37Z","receivedAt":"2013-01-10T21:39:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 10, 2013 at 12:22:49PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Maybe the right rule is \"if we are using the shell to execute, do not\n> > mention SIGPIPE\"? It seems a little iffy at first, but:\n> >\n> >   1. It tends to coincide with direct use of internal tools versus\n> >      external tools.\n> >\n> >   2. We do not reliably get SIGPIPE there, anyway, since most shells\n> >      will convert it into exit code 141 before we see it.\n> >\n> > I.e., something like:\n> \n> Hmph.  That may be a good heuristics, but I wonder if we also want\n> to special case WIFEXITED(status) && WEXITSTATUS(status) == 141 to\n> pretend as if nothing went wrong, when ignore_sigpipe is in effect?\n\nWe could, but I don't see much point. There is very little to gain (because\nnobody is complaining about the exit code, only the message), and we\nmight possibly fail to propagate an error condition (unlikely, but more\nserious consequences). To be honest, I am having doubts about touching\nit at all. I had to really work to provoke the error without setting\nSHELL_PATH=zsh, so I am worried that we are getting worked up over\nsomething that just doesn't happen in practice. And I am not sure that\nsetting SHELL_PATH=zsh is a sane thing (I only knew about it because\nBart mentioned it he was using zsh).\n\nBart, do you actually set up SHELL_PATH like that? Is /bin/sh on your\nsystem zsh? If the latter, I wonder if this is actually a bug in zsh.\n\n-Peff\n"},{"id":"206472","messageId":"50EF380A.9010809@kdbg.org","threadId":"32523","inReplyTo":"7vbocw23fq.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-01-10T21:52:10Z","receivedAt":"2013-01-10T21:52:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.01.2013 21:22, schrieb Junio C Hamano:\n> Jeff King <peff@peff.net> writes:\n> \n>> Maybe the right rule is \"if we are using the shell to execute, do not\n>> mention SIGPIPE\"? It seems a little iffy at first, but:\n>>\n>>   1. It tends to coincide with direct use of internal tools versus\n>>      external tools.\n>>\n>>   2. We do not reliably get SIGPIPE there, anyway, since most shells\n>>      will convert it into exit code 141 before we see it.\n>>\n>> I.e., something like:\n> \n> Hmph.  That may be a good heuristics, but I wonder if we also want\n> to special case WIFEXITED(status) && WEXITSTATUS(status) == 141 to\n> pretend as if nothing went wrong, when ignore_sigpipe is in effect?\n\nThe purpose of Peff's patch is to remove the error message, but not to\npretend success (the return code remains 141).\n\nI looked at all instances with use_shell=1 or RUN_USING_SHELL:\n\nMost of the time, we do not care where the output of the command goes\nto, which I regard as the same case as when a shell runs a command: We\ndon't need to report the SIGPIPE death.\n\nThe interesting cases are when git reads back the output of the command.\nHere, a SIGPIPE death of the child would indicate a bug in git, I think,\nand some diagnostic would be worth it. But we can just as well declare\nthat git doesn't have bugs ;)\n\nThese are the interesting cases:\nconnect.c:640:          conn->use_shell = 1;\n    a connection to a local repository\nconvert.c:372:  child_process.use_shell = 1;\n    clean/smudge filter\ncredential.c:216:       helper.use_shell = 1;\n    credential helper\ndiff.c:4851:    child.use_shell = 1;\n    textconv\n\nAll in all, I think the heuristics makes sense.\n\n-- Hannes\n"},{"id":"206473","messageId":"7vvcb4ejog.fsf@alter.siamese.dyndns.org","threadId":"32523","inReplyTo":"50EF380A.9010809@kdbg.org","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-10T22:51:11Z","receivedAt":"2013-01-10T22:51:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> The interesting cases are when git reads back the output of the command.\n> Here, a SIGPIPE death of the child would indicate a bug in git, I think,\n> and some diagnostic would be worth it. But we can just as well declare\n> that git doesn't have bugs ;)\n>\n> These are the interesting cases:\n> connect.c:640:          conn->use_shell = 1;\n>     a connection to a local repository\n> convert.c:372:  child_process.use_shell = 1;\n>     clean/smudge filter\n> credential.c:216:       helper.use_shell = 1;\n>     credential helper\n> diff.c:4851:    child.use_shell = 1;\n>     textconv\n>\n> All in all, I think the heuristics makes sense.\n\nFair enough.  Thanks for grepping.\n"},{"id":"246432","messageId":"1405925131451-7615524.post@n2.nabble.com","threadId":"32523","inReplyTo":"20130104124756.GA402@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] avoid SIGPIPE warnings for aliases","fromName":"mimimimi","fromEmail":"mimimimimimi159@hotmail.com","sentAt":"2014-07-21T06:45:31Z","receivedAt":"2014-07-21T06:45:31Z","isPatch":true,"sender":{"key":"mimimimimimi159@hotmail.com","avatar":null},"body":"I set up a git alias like this:\n\ngit config --global alias.popmerge '!git stash pop && git merge master'\n\nThen I call it, like this:\n\ngit popmerge\n\nThe \"git stash pop\" is executed, but the \"git merge master\" is ignored.\n\nIf I run \"git merge master\" right after the \"git popmerge\"... it sumply runs\nas expected, performing the merge.\n\nI have other aliases with long sequences of commands... and they run\nflawlessly. It seems something at \"git stash pop\" makes the alias process to\nhalt... Is it possible to avoid this behavior? How?\n\nThanks.\ncode 128\n<http://www.keepdynamic.com/barcoding/asp-net-barcode-generator.shtml>  \n\n\n\n\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/RFC-PATCH-avoid-SIGPIPE-warnings-for-aliases-tp7574160p7615524.html\nSent from the git mailing list archive at Nabble.com.\n"}]}