{"thread":{"id":"34008","subject":"[PATCH] run-command: simplify wait_or_whine","startedAt":"2013-06-01T13:51:55Z","lastAt":"2013-06-01T21:41:36Z","messageCount":15,"participants":["Felipe Contreras","Duy Nguyen","Thomas Rast","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"219134","messageId":"1370094715-2684-1-git-send-email-felipe.contreras@gmail.com","threadId":"34008","inReplyTo":null,"subject":"[PATCH] run-command: simplify wait_or_whine","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T13:51:55Z","receivedAt":"2013-06-01T13:51:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Nobody is checking for specific error codes; it's the errno that's\nimportant.\n\nSigned-off-by: Felipe Contreras \n---\n run-command.c | 14 ++------------\n 1 file changed, 2 insertions(+), 12 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1b32a12..e54e943 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \t\tcode = WTERMSIG(status);\n \t\tif (code != SIGINT && code != SIGQUIT)\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-\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} else if (WIFEXITED(status)) {\n \t\tcode = WEXITSTATUS(status);\n-\t\t/*\n-\t\t * Convert special exit code when execvp failed.\n-\t\t */\n-\t\tif (code == 127) {\n-\t\t\tcode = -1;\n+\t\t/* convert special exit code when execvp failed. */\n+\t\tif (code == 127)\n \t\t\tfailed_errno = ENOENT;\n-\t\t}\n \t} else {\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n-- \n1.8.3.358.g5a91d05\n"},{"id":"219136","messageId":"CACsJy8BRYHkSj1YR9mW=_tdKX_+xKNxpVx+RNYEdoW3S36w=8Q@mail.gmail.com","threadId":"34008","inReplyTo":"1370094715-2684-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-01T14:03:31Z","receivedAt":"2013-06-01T14:03:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n wrote:\n> Nobody is checking for specific error codes; it's the errno that's\n> important.\n\nHave you just disregarded the in-code comment you just removed with\none statement? Did you check all its callers?\n\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> run-command.c | 14 ++------------\n> 1 file changed, 2 insertions(+), 12 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 1b32a12..e54e943 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n> code = WTERMSIG(status);\n> if (code != SIGINT && code != SIGQUIT)\n> error(\"%s died of signal %d\", argv0, code);\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> } else if (WIFEXITED(status)) {\n> code = WEXITSTATUS(status);\n> - /*\n> - * Convert special exit code when execvp failed.\n> - */\n> - if (code == 127) {\n> - code = -1;\n> + /* convert special exit code when execvp failed. */\n> + if (code == 127)\n> failed_errno = ENOENT;\n> - }\n> } else {\n> error(\"waitpid is confused (%s)\", argv0);\n> }\n> --\n> 1.8.3.358.g5a91d05\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n\n--\nDuy\n"},{"id":"219138","messageId":"CAMP44s1jtqGXhFN+KqODR-ooe1BiyQNVD=08nDF0poBc+madxw@mail.gmail.com","threadId":"34008","inReplyTo":"CACsJy8BRYHkSj1YR9mW=_tdKX_+xKNxpVx+RNYEdoW3S36w=8Q@mail.gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T14:06:37Z","receivedAt":"2013-06-01T14:06:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 1, 2013 at 9:03 AM, Duy Nguyen wrote:\n> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n> wrote:\n>> Nobody is checking for specific error codes; it's the errno that's\n>> important.\n>\n> Have you just disregarded the in-code comment you just removed with\n> one statement?\n\nWho cares about the comment? As I said, nobody is checking for those codes.\n\n> Did you check all its callers?\n\nYes, that's why I said nobody is checking for those codes.\n\n-- \nFelipe Contreras\n"},{"id":"219139","messageId":"CACsJy8D=oZ0Y0F8_WFfVxgFtnsRJtxxp9y2oQgoxpxd3zU+rsw@mail.gmail.com","threadId":"34008","inReplyTo":"CAMP44s1jtqGXhFN+KqODR-ooe1BiyQNVD=08nDF0poBc+madxw@mail.gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-01T14:08:52Z","receivedAt":"2013-06-01T14:08:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 1, 2013 at 9:06 PM, Felipe Contreras\n wrote:\n> On Sat, Jun 1, 2013 at 9:03 AM, Duy Nguyen wrote:\n>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n>> wrote:\n>>> Nobody is checking for specific error codes; it's the errno that's\n>>> important.\n>>\n>> Have you just disregarded the in-code comment you just removed with\n>> one statement?\n>\n> Who cares about the comment? As I said, nobody is checking for those codes.\n\nApparently I do.\n\n>> Did you check all its callers?\n>\n> Yes, that's why I said nobody is checking for those codes.\n\nThanks. If would be a few mails less if you stated so in the original message.\n--\nDuy\n"},{"id":"219140","messageId":"87wqqdoqvp.fsf@hexa.v.cablecom.net","threadId":"34008","inReplyTo":"1370094715-2684-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-01T14:19:22Z","receivedAt":"2013-06-01T14:19:22Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> Nobody is checking for specific error codes; it's the errno that's\n> important.\n[...]\n> -\t\t/*\n> -\t\t * This return value is chosen so that code & 0xff\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\nHave you checked the callers? There are lots of callers of\nfinish_command(), which returns the value from wait_or_whine()\nunmodified.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"219141","messageId":"CAMP44s0uFLDanLP=w2ozVG665rCFq5kK7b5Mk_RWYEv39YqCqw@mail.gmail.com","threadId":"34008","inReplyTo":"CACsJy8D=oZ0Y0F8_WFfVxgFtnsRJtxxp9y2oQgoxpxd3zU+rsw@mail.gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T14:20:41Z","receivedAt":"2013-06-01T14:20:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 1, 2013 at 9:08 AM, Duy Nguyen wrote:\n> On Sat, Jun 1, 2013 at 9:06 PM, Felipe Contreras\n> wrote:\n>> On Sat, Jun 1, 2013 at 9:03 AM, Duy Nguyen wrote:\n>>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n>>> wrote:\n>>>> Nobody is checking for specific error codes; it's the errno that's\n>>>> important.\n>>>\n>>> Have you just disregarded the in-code comment you just removed with\n>>> one statement?\n>>\n>> Who cares about the comment? As I said, nobody is checking for those codes.\n>\n> Apparently I do.\n\nWhy do you care about code comments that have no relation to reality?\n\n>>> Did you check all its callers?\n>>\n>> Yes, that's why I said nobody is checking for those codes.\n>\n> Thanks. If would be a few mails less if you stated so in the original message.\n\nSo why did you think I said that that nobody is checking for those codes?\n\nAnyway, apparently somebody added code that checks for the specific\ncode since I wrote this patch:\n\n1250857 launch_editor: propagate signals from editor to git\n\nTo my knowledge this is the only place where the specific number us\nactually checked.\n\n-- \nFelipe Contreras\n"},{"id":"219142","messageId":"CACsJy8Df-O=D5LQBXCbiVLB=uHfn6ETbAxADWq3hd-9pPX4+mg@mail.gmail.com","threadId":"34008","inReplyTo":"1370094715-2684-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-01T14:21:33Z","receivedAt":"2013-06-01T14:21:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n wrote:\n> Nobody is checking for specific error codes; it's the errno that's\n> important.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> run-command.c | 14 ++------------\n> 1 file changed, 2 insertions(+), 12 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 1b32a12..e54e943 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n> code = WTERMSIG(status);\n> if (code != SIGINT && code != SIGQUIT)\n> error(\"%s died of signal %d\", argv0, code);\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> } else if (WIFEXITED(status)) {\n\nThe original commit that introduces this says\n\n run_command: encode deadly signal number in the return value\n\n We now write the signal number in the error message if the program\n terminated by a signal. The negative return value is constructed such that\n after truncation to 8 bits it looks like a POSIX shell's $?:\n\n $ echo 0000 | { git upload-pack .; echo $? >&2; } | :\n error: git-upload-pack died of signal 13\n 141\n\n Previously, the exit code was 255 instead of 141.\n\nSo this is part of the interface to the user. With your changes, the\nexit code is now different. I tested by force segfaulting upload-pack.\n$? returned 11. So NAK.\n--\nDuy\n"},{"id":"219143","messageId":"CAMP44s3GpZNdpF3j-GSthwx0vY37+yfeVFtnHfPaQgZasJZt7w@mail.gmail.com","threadId":"34008","inReplyTo":"87wqqdoqvp.fsf@hexa.v.cablecom.net","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T14:23:33Z","receivedAt":"2013-06-01T14:23:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 1, 2013 at 9:19 AM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> Nobody is checking for specific error codes; it's the errno that's\n>> important.\n> [...]\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> Have you checked the callers? There are lots of callers of\n> finish_command(), which returns the value from wait_or_whine()\n> unmodified.\n\nYes I did. Most of them simply check that the number is not zero.\n\nHowever, that was at the time I wrote the patch, and it seems there's\nnow one instance where the code is checked.\n\n-- \nFelipe Contreras\n"},{"id":"219144","messageId":"CAMP44s0L1M+_s2eDM=Ogy=rxLhpZYwSb8qWTuEe30pB4KGDVtA@mail.gmail.com","threadId":"34008","inReplyTo":"CACsJy8Df-O=D5LQBXCbiVLB=uHfn6ETbAxADWq3hd-9pPX4+mg@mail.gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T14:30:50Z","receivedAt":"2013-06-01T14:30:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 1, 2013 at 9:21 AM, Duy Nguyen wrote:\n> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n> wrote:\n>> Nobody is checking for specific error codes; it's the errno that's\n>> important.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> run-command.c | 14 ++------------\n>> 1 file changed, 2 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 1b32a12..e54e943 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -244,21 +244,11 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n>> code = WTERMSIG(status);\n>> if (code != SIGINT && code != SIGQUIT)\n>> error(\"%s died of signal %d\", argv0, code);\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>> } else if (WIFEXITED(status)) {\n>\n> The original commit that introduces this says\n>\n> run_command: encode deadly signal number in the return value\n>\n> We now write the signal number in the error message if the program\n> terminated by a signal. The negative return value is constructed such that\n> after truncation to 8 bits it looks like a POSIX shell's $?:\n>\n> $ echo 0000 | { git upload-pack .; echo $? >&2; } | :\n> error: git-upload-pack died of signal 13\n> 141\n>\n> Previously, the exit code was 255 instead of 141.\n>\n> So this is part of the interface to the user. With your changes, the\n> exit code is now different. I tested by force segfaulting upload-pack.\n> $? returned 11. So NAK.\n\nYeah, and last year we returned a different code. The world didn't\nend, because nobody is checking for the specific code. But if you want\nto retain complexity forever, suit yourselves.\n\ncommit 709ca730f8e093005cc882bfb86c0ca9c83d345b\nAuthor: Jeff King \nDate: Sat Jan 5 09:49:49 2013 -0500\n\n 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\n Fix the same logic in t/test-terminal.perl for consistency [jc:\n raised by Jonathan in the discussion].\n\n Signed-off-by: Jeff King \n Acked-by: Johannes Sixt \n Reviewed-by: Jonathan Nieder \n Signed-off-by: Junio C Hamano \n\n-- \nFelipe Contreras\n"},{"id":"219145","messageId":"CACsJy8BFv7kJkymJ_rj9dwaN-zMcHtS9sjmqDSpLWB5TsUU_kg@mail.gmail.com","threadId":"34008","inReplyTo":"CAMP44s0L1M+_s2eDM=Ogy=rxLhpZYwSb8qWTuEe30pB4KGDVtA@mail.gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-01T14:36:13Z","receivedAt":"2013-06-01T14:36:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 1, 2013 at 9:30 PM, Felipe Contreras\n wrote:\n> On Sat, Jun 1, 2013 at 9:21 AM, Duy Nguyen wrote:\n>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n>> wrote:\n> Yeah, and last year we returned a different code. The world didn't\n> end, because nobody is checking for the specific code. But if you want\n> to retain complexity forever, suit yourselves.\n\nAnd that was changed for a reason, compared to this change \"because I\nlike it\". I maintain my NAK (not that it matters) until you justify\nyour change better than \"nobody is using it\".\n\n> commit 709ca730f8e093005cc882bfb86c0ca9c83d345b\n> Author: Jeff King \n> Date: Sat Jan 5 09:49:49 2013 -0500\n>\n> run-command: encode signal death as a positive integer\n-- \nDuy\n"},{"id":"219146","messageId":"CAMP44s2cBGc+uKH0t-KZko-5GYkZUK54+7wiYbWim55B7KGaGA@mail.gmail.com","threadId":"34008","inReplyTo":"CACsJy8BFv7kJkymJ_rj9dwaN-zMcHtS9sjmqDSpLWB5TsUU_kg@mail.gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T15:01:49Z","receivedAt":"2013-06-01T15:01:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 1, 2013 at 9:36 AM, Duy Nguyen wrote:\n> On Sat, Jun 1, 2013 at 9:30 PM, Felipe Contreras\n> wrote:\n>> On Sat, Jun 1, 2013 at 9:21 AM, Duy Nguyen wrote:\n>>> On Sat, Jun 1, 2013 at 8:51 PM, Felipe Contreras\n>>> wrote:\n>> Yeah, and last year we returned a different code. The world didn't\n>> end, because nobody is checking for the specific code. But if you want\n>> to retain complexity forever, suit yourselves.\n>\n> And that was changed for a reason, compared to this change \"because I\n> like it\". I maintain my NAK (not that it matters) until you justify\n> your change better than \"nobody is using it\".\n\nWho said the reason was \"because I like it\"? You don't agree that\nmaking the code simpler and more maintainable is a good reason for any\nchange?\n\nAnyway, if you care so much about the current behavior, why isn't\nthere any tests that check for this?\n\nMy patch passes *all* the tests.\n\n-- \nFelipe Contreras\n"},{"id":"219148","messageId":"20130601170147.GA19234@sigill.intra.peff.net","threadId":"34008","inReplyTo":"CAMP44s0L1M+_s2eDM=Ogy=rxLhpZYwSb8qWTuEe30pB4KGDVtA@mail.gmail.com","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-01T17:01:48Z","receivedAt":"2013-06-01T17:01:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 01, 2013 at 09:30:50AM -0500, Felipe Contreras wrote:\n\n> > The original commit that introduces this says\n> >\n> > run_command: encode deadly signal number in the return value\n> >\n> > We now write the signal number in the error message if the program\n> > terminated by a signal. The negative return value is constructed such that\n> > after truncation to 8 bits it looks like a POSIX shell's $?:\n> >\n> > $ echo 0000 | { git upload-pack .; echo $? >&2; } | :\n> > error: git-upload-pack died of signal 13\n> > 141\n> >\n> > Previously, the exit code was 255 instead of 141.\n> >\n> > So this is part of the interface to the user. With your changes, the\n> > exit code is now different. I tested by force segfaulting upload-pack.\n> > $? returned 11. So NAK.\n> \n> Yeah, and last year we returned a different code. The world didn't\n> end, because nobody is checking for the specific code. But if you want\n> to retain complexity forever, suit yourselves.\n\nLast year we returned a different code from the function that other C\ncode saw. But what got returned via exit() to exterior programs was\nalways 141 in the SIGPIPE case, both before and after my 709ca730. That\nis explained in the first two paragraphs here:\n\n> commit 709ca730f8e093005cc882bfb86c0ca9c83d345b\n> Author: Jeff King \n> Date: Sat Jan 5 09:49:49 2013 -0500\n> \n> 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\nYour patch changes the error code that is propagated via exit() in this\ncase. We cannot know \"nobody is checking for the specific code\", because\nthe list of callers is every shell script or program which execs git.\nSome of them do care about the exit code. I can give an example of a\ncase I have that cares, but I do not think it is even important. The\npoint is that we would be regressing an existing interface, and cannot\nknow who is broken by it.\n\n-Peff\n"},{"id":"219150","messageId":"20130601172441.GB19234@sigill.intra.peff.net","threadId":"34008","inReplyTo":"CAMP44s2cBGc+uKH0t-KZko-5GYkZUK54+7wiYbWim55B7KGaGA@mail.gmail.com","subject":"[PATCH] t0005: test git exit code from signal death","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-01T17:24:41Z","receivedAt":"2013-06-01T17:24:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 01, 2013 at 10:01:49AM -0500, Felipe Contreras wrote:\n\n> Anyway, if you care so much about the current behavior, why isn't\n> there any tests that check for this?\n> \n> My patch passes *all* the tests.\n\nThe test suite has never been (and probably never will be) a complete\nspecification of git's behavior. Noticing that a desired behavior is not\nin the test suite is an opportunity to improve its coverage, not argue\nthat a change which breaks the desired behavior must therefore be\nacceptable.\n\nWe could do something like the patch below, with two caveats:\n\n 1. The test immediately before it checks for exit codes other than\n \"143\" on various platforms. I do not know to what degree we need to\n deal with that here. Git is doing the interpretation here rather\n than the shell, so the ksh case should not matter. But I do not\n know what part of Windows converts the exit code to 3. Do we need\n to be looking for 3? Or 131 (128+3)?\n\n 2. The test detects a total breakage of the exit code, like the one\n caused by your patch. But I do not know if it would reliably detect\n us failing to convert the code at all, as the shell running the\n test harness would presumably convert it to shell-convention\n itself. Getting around that would require a custom C program\n checking the status returned by waitpid().\n\n On the other hand, perhaps it is not the conversion we care about;\n as long as we end up with 143, we are fine. We just want to make\n sure that signal death is recorded in _one_ of the potential signal\n formats. So we do not care if git or the shell did the conversion,\n as long as we end up with 143. The real breakage is exiting with\n code 15, which is losing information.\n\n-- >8 --\nSubject: [PATCH] t0005: test git exit code from signal death\n\nWhen a sub-process dies with a signal, we convert the exit\ncode to the shell convention of 128+sig. Callers of git may\nbe relying on this behavior, so let's make sure it does not\nbreak.\n\nSigned-off-by: Jeff King \n---\n t/t0005-signals.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t0005-signals.sh b/t/t0005-signals.sh\nindex 93e58c0..63f9764 100755\n--- a/t/t0005-signals.sh\n+++ b/t/t0005-signals.sh\n@@ -20,4 +20,11 @@ test_expect_success 'sigchain works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'signals are propagated using shell convention' '\n+\t# we use exec here to avoid any sub-shell interpretation\n+\t# of the exit code\n+\tgit config alias.sigterm \"!exec test-sigchain\" &&\n+\ttest_expect_code 143 git sigterm\n+'\n+\n test_done\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"219155","messageId":"CAMP44s078Vn6CMRyqAkgE6rFBgUyJV9wJaYqkQgz-w3WnEi-vQ@mail.gmail.com","threadId":"34008","inReplyTo":"20130601170147.GA19234@sigill.intra.peff.net","subject":"Re: [PATCH] run-command: simplify wait_or_whine","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T21:35:14Z","receivedAt":"2013-06-01T21:35:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 1, 2013 at 12:01 PM, Jeff King wrote:\n> On Sat, Jun 01, 2013 at 09:30:50AM -0500, Felipe Contreras wrote:\n\n>> commit 709ca730f8e093005cc882bfb86c0ca9c83d345b\n>> Author: Jeff King \n>> Date: Sat Jan 5 09:49:49 2013 -0500\n>>\n>> 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> Your patch changes the error code that is propagated via exit() in this\n> case. We cannot know \"nobody is checking for the specific code\", because\n> the list of callers is every shell script or program which execs git.\n> Some of them do care about the exit code. I can give an example of a\n> case I have that cares, but I do not think it is even important. The\n> point is that we would be regressing an existing interface, and cannot\n> know who is broken by it.\n\nOf course we can know, by going forward with the change, and quite\noften that's the only way to know for sure.\n\nBut if it's true what you said that we haven't changed what the\nprocess returns, then it doesn't make sense to attempt going forward,\nbecause we have no reference point about the likelihood of scripts\nrelying on specific exit codes.\n\n-- \nFelipe Contreras\n"},{"id":"219157","messageId":"CAMP44s0skh8LGFpNNfTfYn0sMrmKi=s0hV=SZwW2mT_F8KKSzg@mail.gmail.com","threadId":"34008","inReplyTo":"20130601172441.GB19234@sigill.intra.peff.net","subject":"Re: [PATCH] t0005: test git exit code from signal death","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-01T21:41:36Z","receivedAt":"2013-06-01T21:41:36Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 1, 2013 at 12:24 PM, Jeff King wrote:\n> On Sat, Jun 01, 2013 at 10:01:49AM -0500, Felipe Contreras wrote:\n>\n>> Anyway, if you care so much about the current behavior, why isn't\n>> there any tests that check for this?\n>>\n>> My patch passes *all* the tests.\n>\n> The test suite has never been (and probably never will be) a complete\n> specification of git's behavior. Noticing that a desired behavior is not\n> in the test suite is an opportunity to improve its coverage, not argue\n> that a change which breaks the desired behavior must therefore be\n> acceptable.\n\nNobody did such a thing. I asked a question, and it's good that you\nare answering by filling the missing test-case.\n\n-- \nFelipe Contreras\n"}]}