{"thread":{"id":"8324","subject":"[PATCH] Don't ignore write failure from git-diff, git-log, etc.","startedAt":"2007-05-26T11:45:42Z","lastAt":"2007-05-30T19:43:47Z","messageCount":29,"participants":["Jim Meyering","Linus Torvalds","Junio C Hamano","Nicolas Pitre","Marco Roeland","Petr Baudis"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"43344","messageId":"87bqg724gp.fsf@rho.meyering.net","threadId":"8324","inReplyTo":null,"subject":"[PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-26T11:45:42Z","receivedAt":"2007-05-26T11:45:42Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Currently, when git-diff writes to a full device or gets an I/O error,\nit fails to detect the write error:\n\n $ git-diff |wc -c\n 3984\n $ git-diff > /dev/full && echo ignored write failure\n ignored write failure\n\ngit-log does the same thing:\n\n $ git-log -n1 > /dev/full && echo ignored write failure\n ignored write failure\n\nEach git command should report such a failure.\nSome already do, but with the patch below, they all do, and we\nwon't have to rely on code in each command's implementation to\nperform the right incantation.\n\n $ ./git-log -n1 > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n $ ./git-diff > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nYou can demonstrate this with git's own --version output, too:\n(but git --help detects the failure without this patch)\n\n $ ./git --version > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nNote that the fcntl test (for whether the fileno may be closed) is\nrequired in order to avoid EBADF upon closing an already-closed stdout,\nas would happen for each git command that already closes stdout; I think\nupdate-index was the one I noticed in the failure of t5400, before I\nadded that test.\n\nSigned-off-by: Jim Meyering \n---\n git.c | 11 ++++++++++-\n 1 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 29b55a1..a7d6515 100644\n--- a/git.c\n+++ b/git.c\n@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n \t\tstruct cmd_struct *p = commands+i;\n \t\tconst char *prefix;\n+\t\tint status;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n\n@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \t\t\tdie(\"%s must be run in a work tree\", cmd);\n \t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n\n-\t\texit(p->fn(argc, argv, prefix));\n+\t\tstatus = p->fn(argc, argv, prefix);\n+\n+\t\t/* Close stdout if necessary, and diagnose any failure. */\n+\t\tif (0 <= fcntl(fileno (stdout), F_GETFD)\n+\t\t && (ferror(stdout) || fclose(stdout)))\n+\t\t\tdie(\"write failure on standard output: %s\",\n+\t\t\t strerror(errno));\n+\n+\t\texit(status);\n \t}\n }\n\n--\n1.5.2.73.g18bece\n"},{"id":"43353","messageId":"alpine.LFD.0.98.0705260910220.26602@woody.linux-foundation.org","threadId":"8324","inReplyTo":"87bqg724gp.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-26T16:18:20Z","receivedAt":"2007-05-26T16:18:20Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 26 May 2007, Jim Meyering wrote:\n>\n> Each git command should report such a failure.\n> Some already do, but with the patch below, they all do, and we\n> won't have to rely on code in each command's implementation to\n> perform the right incantation.\n\nThe patch is wrong.\n\nSome write errors are expected and GOOD.\n\nFor example, EPIPE should not be reported. It's normal. The user got \nbored. It might be hidden by the SIGPIPE killing us, but regardless, \nreporting it for the normal log/diff thing is just not correct. EPIPE \nisn't an error, it's a \"ok, nobody is listening any more\".\n\nAlso, PLEASE don't do this:\n\n> +\t\tif (0 <= fcntl(fileno (stdout), F_GETFD)\n\nThat's totally unreadable to any normal human.\n\nYou don't say \"if zero is smaller or equal to X\". You say \"if X is larger \nthan or equal to zero\". Stop messing with peoples minds, dammit!\n\nAnybody who thinks that code like this causes fewer errors is just fooling \nhimself. It causes *more* bugs, because people have a harder time reading \nit.\n\nMaybe you and Junio have taught yourself bad manners, but you're a tiny \ntiny part of humanity or the development community. Junio can do it just \nbecause while he's just a single person, he's a big part of the git coding \nbase, but anybody else who does it should just be shot.\n\n\t\t\tLinus\n"},{"id":"43355","messageId":"7vk5uvjy0g.fsf@assigned-by-dhcp.cox.net","threadId":"8324","inReplyTo":"alpine.LFD.0.98.0705260910220.26602@woody.linux-foundation.org","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-26T17:27:43Z","receivedAt":"2007-05-26T17:27:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Also, PLEASE don't do this:\n>\n>> +\t\tif (0 <= fcntl(fileno (stdout), F_GETFD)\n>\n> That's totally unreadable to any normal human.\n>\n> You don't say \"if zero is smaller or equal to X\". You say \"if X is larger \n> than or equal to zero\". Stop messing with peoples minds, dammit!\n>\n> Anybody who thinks that code like this causes fewer errors is just fooling \n> himself. It causes *more* bugs, because people have a harder time reading \n> it.\n>\n> Maybe you and Junio have taught yourself bad manners, but you're a tiny \n> tiny part of humanity or the development community. Junio can do it just \n> because while he's just a single person, he's a big part of the git coding \n> base, but anybody else who does it should just be shot.\n\nWhew, that is a blast from the past.\n\ncf. http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3906\n\n (1) Maybe Jim was just being nice, trying to make the code look\n like surrounding code;\n\n (2) Maybe Jim and the person I learned the style from worked\n together for a long time and they picked it up from the\n same source;\n\n (3) Maybe I am not alone, and it is not native language -\n mother tongue issue as some suspected in the quoted thread.\n\nIn any case, I think my recent code have much less \"textual\norder should reflect actual order\" convention than before,\nbecause I have been forcing myself to say aloud \"if X is larger\"\nor \"if X is smaller\" before writing my comparisons, in order to\nmatch the \"peoples minds\" expectation you mentioned above.\n\nThis initially slowed me down and made my head hurt quite a bit,\nand sometimes it still does.\n\nOnce you learn to _visualize_ the ordering relationship in \"X op\nY\" by relying on \"op\" being always < or <=, you will get the\n\"number line\" pop in your head whenever you see a comparision\nexpression, without even having to think about it, and you \"see\"\nX and Y on the number line:\n\n ... -2 -1 0 1 2 ... \n ---------+---------+---------+---------+---------+---------\n true: 0 <= fcntl(...)\n\n\n ... -2 -1 0 1 2 ... \n ---------+---------+---------+---------+---------+---------\n false: (0 <= fcntl(...))\n\nWhat the comparison is doing comes naturally to you, without\neven having to translate it back to human language \"X is larger\n(or smaller) than this constant\". The ordering is right there,\nin front of your eyes, before you vocalize it.\n\nIn a sense, just like it is hard to go back from git to CVS (or\nit is hard to go back to not knowing the power of the index), it\nis very hard to go back once you learn to do this.\n"},{"id":"43387","messageId":"alpine.LFD.0.99.0705262243530.3366@xanadu.home","threadId":"8324","inReplyTo":"7vk5uvjy0g.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-05-27T03:03:00Z","receivedAt":"2007-05-27T03:03:00Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sat, 26 May 2007, Junio C Hamano wrote:\n\n> Once you learn to _visualize_ the ordering relationship in \"X op\n> Y\" by relying on \"op\" being always < or <=, you will get the\n> \"number line\" pop in your head whenever you see a comparision\n> expression, without even having to think about it, and you \"see\"\n> X and Y on the number line:\n> \n> ... -2 -1 0 1 2 ... \n> ---------+---------+---------+---------+---------+---------\n> true: 0 <= fcntl(...)\n> \n> \n> ... -2 -1 0 1 2 ... \n> ---------+---------+---------+---------+---------+---------\n> false: (0 <= fcntl(...))\n> \n> What the comparison is doing comes naturally to you, without\n> even having to translate it back to human language \"X is larger\n> (or smaller) than this constant\". The ordering is right there,\n> in front of your eyes, before you vocalize it.\n\nWell... it probably depends on how your brain is wired up.\n\nI completely agree with your reasoning. It _should_ indeed be natural \nand more obvious to always put things in increasing order.\n\nBUT it is not how my brain is connected, and after many attempts I just \ncannot work efficiently with your method. It simply doesn't come out \nlogical for me and I have to spend an unusual amount of time on every \noccasion I encounter this structure to really get it. To me it always \nlooks backward.\n\nAnd I suspect the majority of people who just cannot train their brain \nwith the arguably superior representation are many, probably the \nmajority. It appears to be the case for Linus. It is definitely the \ncase for me.\n\n\nNicolas, who apologizes for his defective brain.\n"},{"id":"43400","messageId":"87odk6y6cd.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"alpine.LFD.0.98.0705260910220.26602@woody.linux-foundation.org","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-27T09:16:18Z","receivedAt":"2007-05-27T09:16:18Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n> On Sat, 26 May 2007, Jim Meyering wrote:\n>>\n>> Each git command should report such a failure.\n>> Some already do, but with the patch below, they all do, and we\n>> won't have to rely on code in each command's implementation to\n>> perform the right incantation.\n>\n> The patch is wrong.\n\nWhat you should have said is that the patch is fine in principle, since\nit does fix a pretty serious bug (important tools ignoring ENOSPC),\nbut you'd prefer that it continue to ignore EPIPE.\n\nWith a name like yours, being more positive would go a long way toward\nencouraging (or rather *not discouraging*) contributions.\n\n> Some write errors are expected and GOOD.\n>\n> For example, EPIPE should not be reported. It's normal. The user got\n> bored. It might be hidden by the SIGPIPE killing us, but regardless,\n> reporting it for the normal log/diff thing is just not correct. EPIPE\n> isn't an error, it's a \"ok, nobody is listening any more\".\n\nI have to disagree. There may be precedent for hiding EPIPE errors,\nbut that is not the norm among command line tools wrt piped stdout.\n\nFirst of all, one has to work just to get such an error. These days,\nmost people use a shell that doesn't ignore, handle or block SIGPIPE, so\ndirect use of a program like git-log or git-diff gets the signal directly,\nand there's no EPIPE error. E.g., try \"cat\", and you see it gets SIGPIPE\n(141=128+SIGPIPE(13)):\n\n $ seq 90000|(cat; echo $? >&2) | head -1 > /dev/null\n 141\n\nHowever, you can tweak your shell to handle/ignore SIGPIPE. Also,\nsome porcelain scripts do ignore SIGPIPE. Then, a program run in that\nenvironment does see EPIPE. Consider how a few other non-interactive\nprograms work when one of their stdout-writing syscalls fails with EPIPE:\n\nTry GNU diff and sed:\n[now, using shorter \":\" in place of more realistic head -1]\n\n $ (trap '' PIPE; seq 90000|diff - /dev/null;echo $? >&2)| :\n diff: standard output: Broken pipe\n 2\n $ (trap '' PIPE; seq 90000|sed s/a/b/; echo $? 1>&2)| :\n /bin/sed: couldn't write 5 items to stdout: Broken pipe\n seq: write error: Broken pipe\n 4\n\nTry tee (from GNU coreutils):\n\n $ (trap '' PIPE; seq 90000|tee /dev/null; echo $? >&2) | :\n tee: standard output: Broken pipe\n tee: write error\n 1\n\nsort, tac, cut, fold, od, head, tail, tr, uniq, etc. all work the same\nway, if you're using the coreutils. But perhaps that's not fair, since\nI maintain the coreutils. And there is some variance among how other-\nvendor versions of those tools work. E.g., Solaris 10's /bin/cat\ndiagnoses the error, but neither /bin/sort nor /bin/diff do.\n\nAs for version control tools, monotone does what I'd expect in\nthis situation: \"mtn diff\" reports the failure and exits nonzero.\n\nsvn and cvs also report the error, although they both exit successfully\nin spite of that. I tried both log and diff commands for each tool.\ncvs catches the signal, svn doesn't.\n\nmercurial and darcs totally ignore the write error and SIGPIPE,\nso there is no way to determine from stderr or exit code whether\ntheir writes complete normally.\n\nFor any tool whose output might be piped to another, the pipe-writing\ntool should exit nonzero for any write error. Otherwise, its exit code\nends up being a lie, pretending success but, in effect, covering up for\na failure. In general, I've found that papering over syscall failures\nmakes higher-level problems harder to diagnose.\n\nDo you really want git-log to continue to do this?\n\n $ (trap '' PIPE; git-log; echo $? >&2 ) | :\n 0\n\nWith my patch, it does this:\n\n $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :\n fatal: write failure on standard output: Broken pipe\n 128\n\n> Also, PLEASE don't do this:\n>\n>> +\t\tif (0 <= fcntl(fileno (stdout), F_GETFD)\n\nSince Junio is making an effort to \"conform\",\nI too will make the effort when contributing to git.\n"},{"id":"43441","messageId":"alpine.LFD.0.98.0705270904240.26602@woody.linux-foundation.org","threadId":"8324","inReplyTo":"87odk6y6cd.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-27T16:17:58Z","receivedAt":"2007-05-27T16:17:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 27 May 2007, Jim Meyering wrote:\n> \n> I have to disagree. There may be precedent for hiding EPIPE errors,\n> but that is not the norm among command line tools wrt piped stdout.\n\n.. and this is a PROBLEM. Which is why I think your patch was really \nwrong.\n\nI don't know how many people remember all the _stupid_ problems we had \nexactly because many versions of bash are crap, crap, crap, and people \n(including you) don't realize that EPIPE is _different_ from other write \nerrors.\n\nJust do a google search for\n\n\t\"broken pipe\" bash\n\nand not only will you see a lot of complaints, but the #5 entry is a \ncomplaint for a git issue that we had tons of problems with. See for \nexample\n\n\thttp://www.gelato.unsw.edu.au/archives/git/0504/2602.html\n\nThe reason? Some _idiotic_ versions of bash don't have DONT_REPORT_SIGPIPE \non by default. \n\nSo I do get upset when people then make the same error with git.\n\n> Do you really want git-log to continue to do this?\n> \n> $ (trap '' PIPE; git-log; echo $? >&2 ) | :\n> 0\n> \n> With my patch, it does this:\n> \n> $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :\n> fatal: write failure on standard output: Broken pipe\n> 128\n\nThat error return is fine. The annoying error report, however, is NOT.\n\nFor _exactly_ the same reason that a bash that doesn't have \nDONT_REPORT_SIGPIPE enabled is a piece of crap.\n\nAnd your arguments that \"others do it wrong, so we can too\" is so broken \nas to be really really sad. If you cannot see the serious problem with \nthat argument, I don't know what to tell you.\n\nTry this:\n\n\ttrap '' PIPE; ./git-log | head\n\nand dammit, if you get an error message from that, your program is BROKEN.\n\nAnd if you cannot understand that, then I don't even know what to say.\n\nBut _exiting_ is fine. It's the bogus error reporting that isn't. The \nabove command like should NOT cause the user to have to skip stderr - \nbecause no error happened!\n\n(Whether the error code is 0 or some error, I dunno. I'd argue that if you \nignore SIGPIPE, you'd probably also want to do \"exit(0)\" for EPIPE, but \nit's not nearly as annoying as writing bogus error messages to stderr.\n\n\t\t\tLinus\n"},{"id":"43492","messageId":"87sl9hw0o0.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"alpine.LFD.0.98.0705270904240.26602@woody.linux-foundation.org","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-28T13:14:07Z","receivedAt":"2007-05-28T13:14:07Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n> On Sun, 27 May 2007, Jim Meyering wrote:\n>>\n>> I have to disagree. There may be precedent for hiding EPIPE errors,\n>> but that is not the norm among command line tools wrt piped stdout.\n>\n> .. and this is a PROBLEM. Which is why I think your patch was really\n> wrong.\n>\n> I don't know how many people remember all the _stupid_ problems we had\n> exactly because many versions of bash are crap, crap, crap, and people\n> (including you) don't realize that EPIPE is _different_ from other write\n> errors.\n>\n> Just do a google search for\n>\n> \t\"broken pipe\" bash\n>\n> and not only will you see a lot of complaints, but the #5 entry is a\n> complaint for a git issue that we had tons of problems with. See for\n> example\n>\n> \thttp://www.gelato.unsw.edu.au/archives/git/0504/2602.html\n\nWhether bash should print a diagnostic when it kills a process with\nSIGPIPE is _different_ from whether the writing process should diagnose\nits own write failure arising from a handled SIGPIPE.\n\nI suspect that git's special treatment of EPIPE was a shoot-the-messenger\nreaction to the work-around (trap '' PIPE) required to avoid diagnostics\nfrom porcelain being interpreted by what would now be a 2-year-old\nversion of bash. It is time to remove that work-around, because it\ncan obscure real errors, and removing it will be largely unnoticed.\n\n...\n>> Do you really want git-log to continue to do this?\n>>\n>> $ (trap '' PIPE; git-log; echo $? >&2 ) | :\n>> 0\n>>\n>> With my patch, it does this:\n>>\n>> $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :\n>> fatal: write failure on standard output: Broken pipe\n>> 128\n>\n> That error return is fine. The annoying error report, however, is NOT.\n\nThat error message serves to indicate a REAL FLAW, some of the time.\nIn such cases, the diagnostic is likely to be welcome, not annoying.\nWhich is more important: avoiding annoyance in some now-very-unusual\ncircumstances, or allowing robust porcelain to diagnose real errors?\n\n> And your arguments that \"others do it wrong, so we can too\" is so broken\n> as to be really really sad. If you cannot see the serious problem with\n> that argument, I don't know what to tell you.\n\nQuestionable debate tactic: misrepresent an opponent's argument with a\nstatement that is obviously foolish, then proceed to argue that anyone\nwho doesn't recognise the silliness of the restated argument leaves\nyou speechless.\n\nMy argument is \"If so many other tools do it RIGHT,\nwhy can't git do it right, too?\".\n\n> Try this:\n>\n> \ttrap '' PIPE; ./git-log | head\n>\n> and dammit, if you get an error message from that, your program is BROKEN.\n>\n> And if you cannot understand that, then I don't even know what to say.\n\nOf course error messages are annoying when your short-pipe-read is\n_deliberate_ (tho, most real uses of git tools will actually get no\nmessage to be annoyed about[*]), but what if there really *is* a mistake?\nTry this:\n\n # You want to force git to ignore the error.\n $ trap '' PIPE; git-rev-list HEAD | sync\n $\n\n # I want to diagnose the error (the exit 128 is from zsh, bash gets 0):\n $ trap '' PIPE; git-rev-list HEAD | sync\n fatal: write failure on standard output: Broken pipe\n [Exit 128]\n\nThe use of \"sync\" above is obviously a mistake.\nWith the existing git tools, most write-to-stdout errors are ignored,\nincluding EPIPE, so this erroneous command completes successfully, just as\nit would when writing to a full or corrupt disk. Even if you use bash's\n\"set -o pipefail\" option, there is no indication of the failure. With my\npatch, write errors are reported, even EPIPE, so that in this case, the\nuser of the above will get an error message. And with bash's pipefail\noption, the git-rev-list write failure can propagate \"out\" to the script.\n\nSince using \"sync\" is contrived, for a little more realism, imagine the\nSHA1 strings are being written to a FIFO, and you don't have access to\nthe program running on the other side. The sender should have a way to\ndetect when the other end closes the pipe prematurely. Exempting EPIPE,\nit CANNOT detect failure:\n\n $ mkfifo F && head -1 F > /dev/null & sleep 1\n $ trap '' PIPE; git-rev-list HEAD > F && echo bad: ignored write failure\n bad: ignored write failure\n\nHandling EPIPE like other write errors, it CAN detect failure:\n\n $ mkfifo F && head -1 F > /dev/null & sleep 1\n $ trap '' PIPE; ./git-rev-list HEAD > F || echo ok\n fatal: write failure on standard output: Broken pipe\n ok\n\n> But _exiting_ is fine. It's the bogus error reporting that isn't. The\n> above command like should NOT cause the user to have to skip stderr -\n> because no error happened!\n\nDon't worry about the diagnostic. It probably won't show up much at all\n[*] these days, since most shells now let SIGPIPE kill the writer (silently).\nAnd if the message does appear once in a while, there are cleaner ways\nto suppress it than hamstringing all of the git plumbing for everyone.\n\nIn fact, in this vein, the existing EPIPE exceptions in write_or_die.c\nand merge-recursive.c should be removed. Here's a revised patch to do\nthat, too. Junio, if you see fit to accept any part of this, I'll be\nhappy to write test case additions demonstrating before/after improvements.\n\n========================================================================\nSubject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.\n\nCurrently, when git-diff writes to a full device or gets an I/O error,\nit fails to detect the write error:\n\n $ git-diff |wc -c\n 3984\n $ git-diff > /dev/full && echo ignored write failure\n ignored write failure\n\ngit-log does the same thing:\n\n $ git-log -n1 > /dev/full && echo ignored write failure\n ignored write failure\n\nEach and every git command should report such a failure.\nSome already do, but with the patch below, they all do, and we\nwon't have to rely on code in each command's implementation to\nperform the right incantation.\n\n $ ./git-log -n1 > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n $ ./git-diff > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nYou can demonstrate this with git's own --version output, too:\n(but git --help detects the failure without this patch)\n\n $ ./git --version > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nNote that the fcntl test (for whether the fileno may be closed) is\nrequired in order to avoid EBADF upon closing an already-closed stdout,\nas would happen for each git command that already closes stdout; I think\nupdate-index was the one I noticed in the failure of t5400, before I\nadded that test.\n\nAlso, to be consistent, don't ignore EPIPE write failures.\n\nSigned-off-by: Jim Meyering \n---\n git.c | 11 ++++++++++-\n merge-recursive.c | 3 ---\n write_or_die.c | 4 ----\n 3 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 29b55a1..e176ab4 100644\n--- a/git.c\n+++ b/git.c\n@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n \t\tstruct cmd_struct *p = commands+i;\n \t\tconst char *prefix;\n+\t\tint status;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n\n@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \t\t\tdie(\"%s must be run in a work tree\", cmd);\n \t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n\n-\t\texit(p->fn(argc, argv, prefix));\n+\t\tstatus = p->fn(argc, argv, prefix);\n+\n+\t\t/* Close stdout if necessary, and diagnose any failure. */\n+\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0)\n+\t\t && (ferror(stdout) || fclose(stdout)))\n+\t\t\tdie(\"write failure on standard output: %s\",\n+\t\t\t strerror(errno));\n+\n+\t\texit(status);\n \t}\n }\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8f72b2c..bfa4335 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -523,9 +523,6 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)\n \twhile (size > 0) {\n \t\tlong ret = write_in_full(fd, buf, size);\n \t\tif (ret < 0) {\n-\t\t\t/* Ignore epipe */\n-\t\t\tif (errno == EPIPE)\n-\t\t\t\tbreak;\n \t\t\tdie(\"merge-recursive: %s\", strerror(errno));\n \t\t} else if (!ret) {\n \t\t\tdie(\"merge-recursive: disk full?\");\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..fadfcaa 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -41,8 +41,6 @@ int write_in_full(int fd, const void *buf, size_t count)\n void write_or_die(int fd, const void *buf, size_t count)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n \t\tdie(\"write error (%s)\", strerror(errno));\n \t}\n }\n@@ -50,8 +48,6 @@ void write_or_die(int fd, const void *buf, size_t count)\n int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n \t\tfprintf(stderr, \"%s: write error (%s)\\n\",\n \t\t\tmsg, strerror(errno));\n \t\treturn 0;\n"},{"id":"43496","messageId":"20070528154630.GA9176@fiberbit.xs4all.nl","threadId":"8324","inReplyTo":"87sl9hw0o0.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Marco Roeland","fromEmail":"marco.roeland@xs4all.nl","sentAt":"2007-05-28T15:46:30Z","receivedAt":"2007-05-28T15:46:30Z","isPatch":true,"sender":{"key":"marco.roeland@xs4all.nl","avatar":null},"body":"On monday May 28th 2007 at 15:14 Jim Meyering wrote:\n\n> > I don't know how many people remember all the _stupid_ problems we had\n> > exactly because many versions of bash are crap, crap, crap, and people\n> > (including you) don't realize that EPIPE is _different_ from other write\n> > errors.\n> >\n> > Just do a google search for\n> >\n> > \t\"broken pipe\" bash\n> >\n> > and not only will you see a lot of complaints, but the #5 entry is a\n> > complaint for a git issue that we had tons of problems with. See for\n> > example\n> >\n> > \thttp://www.gelato.unsw.edu.au/archives/git/0504/2602.html\n> \n> Whether bash should print a diagnostic when it kills a process with\n> SIGPIPE is _different_ from whether the writing process should diagnose\n> its own write failure arising from a handled SIGPIPE.\n> \n> I suspect that git's special treatment of EPIPE was a shoot-the-messenger\n> reaction to the work-around (trap '' PIPE) required to avoid diagnostics\n> from porcelain being interpreted by what would now be a 2-year-old\n> version of bash. It is time to remove that work-around, because it\n> can obscure real errors, and removing it will be largely unnoticed.\n\nGood point. But also notice that when you are stuck with a shell that\ndoes complain about SIGPIPE it is _really_ annoying!\n\nOn current Debian 'sid' with your patch this does not seem to be the\ncase.\n\nThe second chunk of your patch (to git.c) contains a small copy-paste\naccident methinks:\n\n> @@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n> \t\t\tdie(\"%s must be run in a work tree\", cmd);\n> \t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n> \n> -\t\texit(p->fn(argc, argv, prefix));\n> +\t\tstatus = p->fn(argc, argv, prefix);\n> +\n> +\t\t/* Close stdout if necessary, and diagnose any failure. */\n> +\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0)\n> +\t\t && (ferror(stdout) || fclose(stdout)))\n> +\t\t\tdie(\"write failure on standard output: %s\",\n> +\t\t\t strerror(errno));\n> +\n> +\t\texit(status);\n\nThe if statement with 'fcntl' is missing a brace, it should be:\n\n+\t\tif ((fcntl(fileno (stdout), F_GETFD) >= 0)\n+\t\t && (ferror(stdout) || fclose(stdout)))\n\n-- \nMarco Roeland\n"},{"id":"43498","messageId":"alpine.LFD.0.98.0705280929140.26602@woody.linux-foundation.org","threadId":"8324","inReplyTo":"87sl9hw0o0.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-28T16:32:06Z","receivedAt":"2007-05-28T16:32:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 28 May 2007, Jim Meyering wrote:\n> \n> I suspect that git's special treatment of EPIPE was a shoot-the-messenger\n> reaction to the work-around (trap '' PIPE) required to avoid diagnostics\n> from porcelain being interpreted by what would now be a 2-year-old\n> version of bash.\n\nNo. You don't seem to realize. That was the *default* behaviour of bash.\n\nFor all I know, it might _still_ be the default behaviour.\n\nThe only reason not everybody ever even noticed, was that most \ndistributions were clueful enough to have figured out that it was broken, \nand configured bash separately. But \"most\" does not mean \"all\", and I had \nthis problem on powerpc, and others had it on Debian, I htink (might have \nbeen slackware). I think RH and SuSE had both fixed it explicitly.\n\n> diff --git a/write_or_die.c b/write_or_die.c\n> index 5c4bc85..fadfcaa 100644\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -41,8 +41,6 @@ int write_in_full(int fd, const void *buf, size_t count)\n> void write_or_die(int fd, const void *buf, size_t count)\n> {\n> \tif (write_in_full(fd, buf, count) < 0) {\n> -\t\tif (errno == EPIPE)\n> -\t\t\texit(0);\n> \t\tdie(\"write error (%s)\", strerror(errno));\n\nNack. Nack. NACK.\n\nI think this patch is fundamentally WRONG. This fragment is just a prime \nexample of why the whole patch is crap. The old code was correct, and you \nbroke it.\n\n\t\tLinus\n"},{"id":"43508","messageId":"87646cx13d.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"20070528154630.GA9176@fiberbit.xs4all.nl","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-28T18:19:34Z","receivedAt":"2007-05-28T18:19:34Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Marco Roeland wrote:\n> The if statement with 'fcntl' is missing a brace, it should be:\n>\n> +\t\tif ((fcntl(fileno (stdout), F_GETFD) >= 0)\n> +\t\t && (ferror(stdout) || fclose(stdout)))\n\nThank you.\nThat was a stale patch (from before I compiled/tested, obviously).\nI intended this:\n\n\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0\n\t\t && (ferror(stdout) || fclose(stdout)))\n\nHere's the correct one:\n\nSubject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.\n\nCurrently, when git-diff writes to a full device or gets an I/O error,\nit fails to detect the write error:\n\n $ git-diff |wc -c\n 3984\n $ git-diff > /dev/full && echo ignored write failure\n ignored write failure\n\ngit-log does the same thing:\n\n $ git-log -n1 > /dev/full && echo ignored write failure\n ignored write failure\n\nEach and every git command should report such a failure.\nSome already do, but with the patch below, they all do, and we\nwon't have to rely on code in each command's implementation to\nperform the right incantation.\n\n $ ./git-log -n1 > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n $ ./git-diff > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nYou can demonstrate this with git's own --version output, too:\n(but git --help detects the failure without this patch)\n\n $ ./git --version > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nNote that the fcntl test (for whether the fileno may be closed) is\nrequired in order to avoid EBADF upon closing an already-closed stdout,\nas would happen for each git command that already closes stdout; I think\nupdate-index was the one I noticed in the failure of t5400, before I\nadded that test.\n\nAlso, to be consistent, don't ignore EPIPE write failures.\n\nSigned-off-by: Jim Meyering \n---\n git.c | 11 ++++++++++-\n merge-recursive.c | 3 ---\n write_or_die.c | 4 ----\n 3 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 29b55a1..7b45a73 100644\n--- a/git.c\n+++ b/git.c\n@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n \t\tstruct cmd_struct *p = commands+i;\n \t\tconst char *prefix;\n+\t\tint status;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n\n@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \t\t\tdie(\"%s must be run in a work tree\", cmd);\n \t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n\n-\t\texit(p->fn(argc, argv, prefix));\n+\t\tstatus = p->fn(argc, argv, prefix);\n+\n+\t\t/* Close stdout if necessary, and diagnose any failure. */\n+\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0\n+\t\t && (ferror(stdout) || fclose(stdout)))\n+\t\t\tdie(\"write failure on standard output: %s\",\n+\t\t\t strerror(errno));\n+\n+\t\texit(status);\n \t}\n }\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8f72b2c..bfa4335 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -523,9 +523,6 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)\n \twhile (size > 0) {\n \t\tlong ret = write_in_full(fd, buf, size);\n \t\tif (ret < 0) {\n-\t\t\t/* Ignore epipe */\n-\t\t\tif (errno == EPIPE)\n-\t\t\t\tbreak;\n \t\t\tdie(\"merge-recursive: %s\", strerror(errno));\n \t\t} else if (!ret) {\n \t\t\tdie(\"merge-recursive: disk full?\");\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..fadfcaa 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -41,8 +41,6 @@ int write_in_full(int fd, const void *buf, size_t count)\n void write_or_die(int fd, const void *buf, size_t count)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n \t\tdie(\"write error (%s)\", strerror(errno));\n \t}\n }\n@@ -50,8 +48,6 @@ void write_or_die(int fd, const void *buf, size_t count)\n int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n \t\tfprintf(stderr, \"%s: write error (%s)\\n\",\n \t\t\tmsg, strerror(errno));\n \t\treturn 0;\n"},{"id":"43510","messageId":"20070528190529.GA10656@fiberbit.xs4all.nl","threadId":"8324","inReplyTo":"87646cx13d.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Marco Roeland","fromEmail":"marco.roeland@xs4all.nl","sentAt":"2007-05-28T19:05:29Z","receivedAt":"2007-05-28T19:05:29Z","isPatch":true,"sender":{"key":"marco.roeland@xs4all.nl","avatar":null},"body":"On monday May 28th 2007 at 20:19 Jim Meyering wrote:\n\n> ...\n> \n> Also, to be consistent, don't ignore EPIPE write failures.\n\nIn practice I agree with someone else on this thread that EPIPE _is_\ndifferent. In a way the responsibility doesn't lie with the writer but\nwith the reader.\n\nBut just out of curiosity is there an easy way to test the EPIPE\nbehaviour? I cite a piece of the \"changelog.Debian\" file from the\nDebian version of the bash shell. In Debian, as earlier in many other\ndistributions, the annoying EPIPE error was \"fixed\" in version 2.0.3-3\nfrom 19 dec 1999.\n\n========================================================================\n* Define DONT_REPORT_SIGPIPE: We don't want to see `Broken pipe' messages\n when a job like `cat jobs.c | exit 1' is executed. Fixes part of #7047,\n #10259, #10433 and #10494. Comment from the upstream author: \"The default\n bash behavior with respect to the exit status of a pipeline will not\n change. Changing it as suggested in the discussion of #10494 would render\n bash incompatible with every other shell out there.\". Closed these reports.\n\n -- Matthias Klose Sun, 19 Dec 1999 15:58:43 +0100\n========================================================================\n\nThe mentioned \"test-case\" as used in \"git log -n1 | exit 1\" doesn't\nproduce an error in my Debian 'sid' bash, either with or without your\npatch, so it doesn't seem to have any effect there? Whereas probably in\na \"default\" bash (don't know if upstream has changed it's mind already!)\nwith your patch (i.e. the EPIPE special casing removal) it will again\nprobably introduce these annoying (for interactive use) errors.\n\nThanks for your patch anyway, the \"fcntl\" diagnosis is a really useful\ntechnique to know, and IMVHO also useful for git; although perhaps not\nvery portable for all platforms.\n-- \nMarco Roeland\n"},{"id":"43511","messageId":"871wh0ww80.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"alpine.LFD.0.98.0705280929140.26602@woody.linux-foundation.org","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-28T20:04:47Z","receivedAt":"2007-05-28T20:04:47Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n> On Mon, 28 May 2007, Jim Meyering wrote:\n>>\n>> I suspect that git's special treatment of EPIPE was a shoot-the-messenger\n>> reaction to the work-around (trap '' PIPE) required to avoid diagnostics\n>> from porcelain being interpreted by what would now be a 2-year-old\n>> version of bash.\n\nHi Linus!\n\nThanks for all the encouragement.\n\n> No. You don't seem to realize. That was the *default* behaviour of bash.\n\nOf course it was the default. Because it was the default, it\nprovoked contortions like using `trap '' PIPE' in shell scripts,\nwhich in turn provoked EPIPE diagnostics from git, which\nprompted the EPIPE-ignoring changes in git plumbing.\nAnd those changes can now OBSCURE REAL ERRORS, as I've shown.\n\nNote: it was the SIGPIPE-ignoring work-around that caused the trouble.\nThe bash bug didn't cause trouble with git _directly_.\n\nIf anyone can find a mainstream distro (I didn't) on which my\npatch causes trouble, please let us all know.\n\nBash changed its default first in bash-3.1-alpha1.\nThe next stable release was bash-3.1, in Dec 2005:\n\n r. By default, the shell no longer reports processes dying from SIGPIPE.\n\nIt looks like most major distros had fixed it long before.\nThe latest stable release is bash-3.2 from October, 2006.\n\n> For all I know, it might _still_ be the default behaviour.\n\nIt's not. See above.\nEasy to test: run this: seq 99999|head -1\nif all you see is a single line with \"1\" on it, and an exit status of 0,\nthen there's no problem.\n\nHowever, the version of bash you use is IRRELEVANT to the question of\nEPIPE. SIGPIPE has always been delivered by default. The only difference\nlay in whether bash _reported_ the delivery. It's only the work-around\nignoring of SIGPIPE that used to provoke EPIPE \"broken pipe\" errors.\nNow, all of the git porcelain shell code that did that appears to be gone,\nprobably converted to perl or C.\n\nYou can get an EPIPE diagnostic with my patch any time the affected\nplumbing is invoked from an environment in which SIGPIPE is ignored.\nThat environment could be your shell (if you put \"trap '' PIPE\" in a\nstart-up file -- though no one does *that*), or porcelain that does that,\nor the perlish $SIG{PIPE} = 'IGNORE'. The following are the only parts\nof git I've found that ignore SIGPIPE:\n\n git-archimport.perl\n git-cvsimport.perl\n git-svn.perl\n git-svnimport.perl\n\nAnd nothing in cogito does.\nSo, now, I *really* don't see why there's any fuss about EPIPE.\n\n> The only reason not everybody ever even noticed, was that most\n> distributions were clueful enough to have figured out that it was broken,\n> and configured bash separately. But \"most\" does not mean \"all\", and I had\n> this problem on powerpc, and others had it on Debian, I htink (might have\n> been slackware). I think RH and SuSE had both fixed it explicitly.\n\nPrecisely. That behavior in bash was so annoying that people were\nmotivated to fix it quickly. But all of that was resolved long ago.\n\n...\n> Nack. Nack. NACK.\n>\n> I think this patch is fundamentally WRONG. This fragment is just a prime\n> example of why the whole patch is crap. The old code was correct, and you\n> broke it.\n\nUm... maybe you've forgotten that this patch fixes a hole in the\n\"old code\" (git.c). Many git tools ignore write (ENOSPC) failures.\nCompared to that aspect of the fix, I would have thought EPIPE-\nhandling would be a minor detail. But now, the whole patch has\nbecome \"crap\"?\n\nConsider the EPIPE-related risks/choice:\n\n 1) Continue to ignore EPIPE write failure: can obscure real errors.\n BTW, Linus, don't you agree? You never commented on this point.\n\n 2) Remove the EPIPE exclusion: *might* make git give a \"broken pipe\"\n diagnostic, if you run git in a SIGPIPE-ignoring environment.\n\n#2 seems to be the lesser of two evils, considering that we can fix or\nwork around the occasional \"broken pipe\" error, but we can't work around\nan unconditionally-ignored EPIPE.\n\nJim\n"},{"id":"43512","messageId":"87veecvgsn.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"20070528190529.GA10656@fiberbit.xs4all.nl","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-28T20:23:20Z","receivedAt":"2007-05-28T20:23:20Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Marco Roeland wrote:\n> On monday May 28th 2007 at 20:19 Jim Meyering wrote:\n>> Also, to be consistent, don't ignore EPIPE write failures.\n>\n> In practice I agree with someone else on this thread that EPIPE _is_\n> different. In a way the responsibility doesn't lie with the writer but\n> with the reader.\n\nDo you think it's ok for git-rev-list _not_ to diagnose an erroneous\ncommand like this (i.e., to exit(0)):\n\n git-rev-list HEAD | sync\n\nwhere \"sync\" could be any command that exits successfully\nwithout reading any input?\n\nIs it ok that it is currently *impossible* to diagnose that\nfailure by looking at exit codes?\n\n> But just out of curiosity is there an easy way to test the EPIPE\n> behaviour? I cite a piece of the \"changelog.Debian\" file from the\n\nThere are some examples here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48617\n\n...\n> The mentioned \"test-case\" as used in \"git log -n1 | exit 1\" doesn't\n> produce an error in my Debian 'sid' bash, either with or without your\n> patch, so it doesn't seem to have any effect there? Whereas probably in\n> a \"default\" bash (don't know if upstream has changed it's mind already!)\n> with your patch (i.e. the EPIPE special casing removal) it will again\n> probably introduce these annoying (for interactive use) errors.\n\nAs I just said in reply to Linus, the EPIPE handling difference\nis independent of what version of bash you use.\n\n> Thanks for your patch anyway, the \"fcntl\" diagnosis is a really useful\n> technique to know, and IMVHO also useful for git; although perhaps not\n> very portable for all platforms.\n\nIt appears to be portable enough. fcntl/F_GETFD support is required\nby POSIX, and has been around for ages. FWIW, it's also used in git's\ndaemon.c and sha1_file.c.\n"},{"id":"43514","messageId":"7v4plwd6f0.fsf@assigned-by-dhcp.cox.net","threadId":"8324","inReplyTo":"87646cx13d.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-28T20:44:51Z","receivedAt":"2007-05-28T20:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.\n> ...\n> You can demonstrate this with git's own --version output, too:\n> (but git --help detects the failure without this patch)\n>\n> $ ./git --version > /dev/full\n> fatal: write failure on standard output: No space left on device\n> [Exit 128]\n>\n> Note that the fcntl test (for whether the fileno may be closed) is\n> required in order to avoid EBADF upon closing an already-closed stdout,\n> as would happen for each git command that already closes stdout; I think\n> update-index was the one I noticed in the failure of t5400, before I\n> added that test.\n>\n> Also, to be consistent, don't ignore EPIPE write failures.\n>\n> Signed-off-by: Jim Meyering \n\nI do not think anybody has much objection about the change to\nhandle_internal_command() in git.c you made. Earlier we relied\non exit(3) to close still open filehandles (while ignoring\nerrors), and you made the close explicit in order to detect\nerrors.\n\nBut \"to be consistent\" above is not a very good justification.\n\nIn the presense of shells that give \"annoying\" behaviour (we\nhave to remember that not everybody have enough privilege,\nexpertise, or motivation to update /bin/sh or install their own\ncopy in $HOME/bin/sh), \"EPIPE is different from other errors\" is\na more practical point of view, I'd have to say. IOW, it is not\nclear if it is a good thing \"to be consistent\" to begin with.\n\nI would have appreciated if this were two separate patches. I\nthink the EPIPE one is an independent issue. We could even make\nit a configuration option to ignore EPIPE ;-)\n"},{"id":"43523","messageId":"7v1wh0bpv2.fsf@assigned-by-dhcp.cox.net","threadId":"8324","inReplyTo":"87sl9hw0o0.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-28T21:27:45Z","receivedAt":"2007-05-28T21:27:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> Of course error messages are annoying when your short-pipe-read is\n> _deliberate_ (tho, most real uses of git tools will actually get no\n> message to be annoyed about[*]), but what if there really *is* a mistake?\n> Try this:\n>\n> # You want to force git to ignore the error.\n> $ trap '' PIPE; git-rev-list HEAD | sync\n> $\n\nIt is perfectly valid (although it is stupid) for a Porcelain\nscript to do this:\n\n latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)\n case \"$latest_by_jim\" in\n '') echo \"No commit by Jim\" ;;\n *) # do something interesting on the commit\n ;;;\n esac\n\nIn such a case, it is a bit too much for my taste to force the\nscript to redirect what comes out of fd 2 of the upstream of the\npipe, so that it can filter out only the \"write error\" message\nbut still show other kinds of error messages. You could do so\nby elaborate shell magic, perhaps like this:\n\n filter_pipe_error () {\n exec 3>&1\n (eval \"$1\" 2>&1 1>&3 | grep >&2 -v 'Broken pipe')\n }\n\n\tlatest_by_jim=$(filter_pipe_error \\\n \t'git log --pretty=oneline --author='\\''Jim'\\'' | head -n 1'\n\t)\n\nbut what's the point?\n\nI think something like this instead might be more palatable.\n\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..fadfcaa 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -41,8 +41,8 @@ int write_in_full(int fd, const void *buf, size_t count)\n void write_or_die(int fd, const void *buf, size_t count)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-\t\tif (errno == EPIPE)\n-\t\t\texit(0);\n- \t\tdie(\"write error (%s)\", strerror(errno));\n+\t\tif (errno != EPIPE)\n+\t\t\tdie(\"write error (%s)\", strerror(errno));\n+\t\texit(1);\n \t}\n }\n"},{"id":"43529","messageId":"20070528234103.GV4489@pasky.or.cz","threadId":"8324","inReplyTo":"87veecvgsn.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2007-05-28T23:41:03Z","receivedAt":"2007-05-28T23:41:03Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":" (I think that funnily enough, Linus is to a degree to the Git\ncommunity something like Al Viro and Chris Hellwig are to the Linux\nkernel community. Don't get too derailed by his blunt^Whonest criticism,\nwhich is however usually quite valid. ;-)\n\nOn Mon, May 28, 2007 at 10:23:20PM CEST, Jim Meyering wrote:\n> Marco Roeland wrote:\n> > On monday May 28th 2007 at 20:19 Jim Meyering wrote:\n> >> Also, to be consistent, don't ignore EPIPE write failures.\n> >\n> > In practice I agree with someone else on this thread that EPIPE _is_\n> > different. In a way the responsibility doesn't lie with the writer but\n> > with the reader.\n> \n> Do you think it's ok for git-rev-list _not_ to diagnose an erroneous\n> command like this (i.e., to exit(0)):\n> \n> git-rev-list HEAD | sync\n> \n> where \"sync\" could be any command that exits successfully\n> without reading any input?\n> \n> Is it ok that it is currently *impossible* to diagnose that\n> failure by looking at exit codes?\n\n Actually, yes!\n\n Because there's no \"failure\" per se. The command we piped the output\ninto just decided that he isn't actually interested in any (for whatever\nreason; it might decide dynamically based on some parameters etc.). I\ncan't think of why it could be considered a failure for git-rev-list if\nits customer doesn't happily eat all the output it generates. It's the\ncustomer's job to report any real trouble that happenned and might be\ncause of the premature end (or maybe the premature end was totally\nvalid).\n\n Maybe it could expose some (IMHO contrived) error scenarios, but in\nmost cases I think it will end up just spitting out bogus error\nmessages. And what will people do? They won't bother to filter out this\nparticular one (which isn't even that easy if the strerror() is\nlocalized, furthermore). They will just 2>/dev/null it. And cause the\n*real* error messages go to the land of void as well. There's enough of\nimpossible-to-diagnose-error-conditions-because-stderr-goes-to-null\nscripts in the land of UNIX already and this patch, while actually\nwell-meant to do the opposite, might well actually increase their number\nbecause of this.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nEver try. Ever fail. No matter. // Try again. Fail again. Fail better.\n\t\t-- Samuel Beckett\n"},{"id":"43536","messageId":"alpine.LFD.0.98.0705281957160.26602@woody.linux-foundation.org","threadId":"8324","inReplyTo":"871wh0ww80.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-29T03:01:15Z","receivedAt":"2007-05-29T03:01:15Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 28 May 2007, Jim Meyering wrote:\n> \n> Um... maybe you've forgotten that this patch fixes a hole in the\n> \"old code\" (git.c). Many git tools ignore write (ENOSPC) failures.\n\nMaybe you have not noticed, but my argument has ben about EPIPE.\n\n> Compared to that aspect of the fix, I would have thought EPIPE-\n> handling would be a minor detail. But now, the whole patch has\n> become \"crap\"?\n\nAbout half the patch was _removing_ EPIPE stuff - not at all about the \nENOSPC stuff you claim.\n\nAnd the ENOSPC code could have added the same *correct* code that does the \nright thing for EPIPE.\n\n> 1) Continue to ignore EPIPE write failure: can obscure real errors.\n> BTW, Linus, don't you agree? You never commented on this point.\n\nTHAT'S THE ONLY THING I'VE BEEN COMMENTING ON!\n\nThey aren't \"obscure real errors\". EPIPE is neither obscure _nor_ an \nerror.\n\nThe code-paths where you removed EPIPE handlign have two cases:\n - SIGPIPE happens: you made no change\n - SIGPIPE diesn't happen: you broke the code.\n\nSo remind me again, why the hell do you think your patch is so great and \nso important, considering that it broke real code, and made things worse?\n\nAnd why don't you just admit that EPIPE is special, isn't an error, and \nshouldn't be complained about? If you get EPIPE on the write, it means \n\"the other end didn't care\". It does NOT mean \"I should now do a really \nannoying message\".\n\nIt's that simple. You seem to admit that SIGPIPE handling in bash should \nhave been fixed, and that it was annoying to complain about it there. Why \ncan't you just admit that it's annoyign and wrong to complain about the \nsame thing when it's EPIPE?\n\n\t\t\tLinus\n"},{"id":"43540","messageId":"alpine.LFD.0.98.0705282031280.26602@woody.linux-foundation.org","threadId":"8324","inReplyTo":"7v1wh0bpv2.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-29T03:47:00Z","receivedAt":"2007-05-29T03:47:00Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 28 May 2007, Junio C Hamano wrote:\n> \n> I think something like this instead might be more palatable.\n> \n> diff --git a/write_or_die.c b/write_or_die.c\n> index 5c4bc85..fadfcaa 100644\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -41,8 +41,8 @@ int write_in_full(int fd, const void *buf, size_t count)\n> void write_or_die(int fd, const void *buf, size_t count)\n> {\n> \tif (write_in_full(fd, buf, count) < 0) {\n> -\t\tif (errno == EPIPE)\n> -\t\t\texit(0);\n> - \t\tdie(\"write error (%s)\", strerror(errno));\n> +\t\tif (errno != EPIPE)\n> +\t\t\tdie(\"write error (%s)\", strerror(errno));\n> +\t\texit(1);\n> \t}\n> }\n\nIt's certainly less annoying, but I don't actually think it's *correct*.\n\nI think the current behaviour is simply _superior_.\n\nThink about what happens whenever somebody does\n\n\tgit cmd | head\n\nand then you want to see whether the end result was successful or not.\n\nWhat would the error code from the \"git cmd\" thing mean?\n\nI claim that exiting with SUCCESS is actually the rigt thing to do for the \ngit command in question. It did what it was asked for.\n\nAnd more importantly, considering that a pipe is of indeterminate size, \nwhat happens if it actuall yonly had 15 lines to be printed out. Guess \nwhat? If it writes them fast enough, they'll go into the pipe, and \"head\" \nwill exit only after the write, and we'll never even *know* that the last \nfive lines were ignored, ie there won't be a EPIPE at all.\n\nIn other words, even Jim's example of\n\n\tgit log | sync\n\nwill actually *succeed* even with Jim's patch, if the output fit in the \nkernel pipe buffer, and the \"git log\" command ran quickly enough that \n\"sync\" took longer (which is not at all unlikely).\n\nSo EPIPE really _is_ special: because when you write to a pipe, there's no \nguarantee that you'll get it at all, so whenever you get an EPIPE you \nshould ask yourself:\n\n - what would I have done if the data had fit in the 64kB kernel 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 _this_ \n time, even if I might not notice it next time?\n\nIn other words, the \"exit(0)\" is actually _more_ consistent than \n\"exit(1)\", because exiting with an error message or with an error return \nis going to depend on luck and timing.\n\nFor example, I just did a\n\n\tstrace git show | sync\n\non my kernel archive, and strace shows me that I had:\n\n\t...\n\twrite(1, \"commit c420bc9f09a0926b708c3edb2\"..., 736) = 736\n\texit_group(0)\n\nthe first three times I tried it, and then\n\n\twrite(1, \"commit c420bc9f09a0926b708c3edb2\"..., 736) = -1 EPIPE (Broken pipe)\n\t--- SIGPIPE (Broken pipe) @ 0 (0) ---\n\t+++ killed by SIGPIPE +++\n\nthe fourth time.\n\nWhat should we learn from this? \n\n - a shell that talks about SIGPIPE is going to be universally *hated*, \n because it's really a total crap-shoot. Even with something like \n \"sync\", that doesn't read a single byte from the input, most of the \n time I didn't actually get EPIPE/SIGPIPE at all!\n\n - by exactly the same token, considering \"EPIPE\" as anything else than a \n \"success\" is always just going to lead you to random behaviour.\n\nSo what _should_ you do for EPIPE?\n\nHere's what EPIPE _really_ means:\n\n - you might as well consider the write a success, but the reader isn't \n actually interested, so rather than go on, you might as well stop \n early.\n\nNotice how I very carefull avoided the word \"error\" anywhere. Because it's \nreally not an error. The reader already got everything it wanted. So EPIPE \nshould generally be seen as an \"early success\" rather than as a \"failure\".\n\n\t\tLinus\n"},{"id":"43582","messageId":"87veebs84i.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"7v4plwd6f0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-29T20:11:09Z","receivedAt":"2007-05-29T20:11:09Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>> Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.\n>> ...\n...\n> I do not think anybody has much objection about the change to\n> handle_internal_command() in git.c you made. Earlier we relied\n> on exit(3) to close still open filehandles (while ignoring\n> errors), and you made the close explicit in order to detect\n> errors.\n\nHi Junio,\n\n> But \"to be consistent\" above is not a very good justification.\n\nI know that well, now, especially after all of my fruitless\njustification on this list.\n\n> In the presense of shells that give \"annoying\" behaviour (we\n> have to remember that not everybody have enough privilege,\n> expertise, or motivation to update /bin/sh or install their own\n> copy in $HOME/bin/sh), \"EPIPE is different from other errors\" is\n> a more practical point of view, I'd have to say. IOW, it is not\n> clear if it is a good thing \"to be consistent\" to begin with.\n\nIn case you haven't seen it in the rest of this thread, the version of\nbash you use does not change git's EPIPE-handling behavior. Using stock\nbash-3.0 or bash-2.05b, you will get \"Broken pipe\" messages from some\nscripts, but those are from bash, when it delivers the SIGPIPE signal,\nand not from any application like git. The EPIPE-handling behavior can\ncome into play only when SIGPIPE is *ignored*.\n\n> I would have appreciated if this were two separate patches. I\n> think the EPIPE one is an independent issue. We could even make\n> it a configuration option to ignore EPIPE ;-)\n\nOk. Even though I'm still convinced that ignoring EPIPE is no longer\njustified, I've hamstrung my patch to do what people here want.\n\nNote that I've also changed it not to print strerror(errno) when the\nferror(stdout) test is triggered. In that case, errno may well be\nirrelevant. The ugliness of this addition is pretty striking, compared\nto what I'm used to. FWIW, I would have liked to handle closing stdout\nhere with the same one-liner I use in coreutils: atexit (close_stdout);\nbut that requires autoconf/automake/gnulib infrastructure.\n\n-----------------------------\nFrom 42e3a6f676e9ae4e9640bc2ff36b7ab0b061a60e Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Sat, 26 May 2007 13:43:07 +0200\nSubject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.\n\nCurrently, when git-diff writes to a full device or gets an I/O error,\nit fails to detect the write error:\n\n $ git-diff |wc -c\n 3984\n $ git-diff > /dev/full && echo ignored write failure\n ignored write failure\n\ngit-log does the same thing:\n\n $ git-log -n1 > /dev/full && echo ignored write failure\n ignored write failure\n\nEach and every git command should report such a failure.\nSome already do, but with the patch below, they all do, and we\nwon't have to rely on code in each command's implementation to\nperform the right incantation.\n\n $ ./git-log -n1 > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n $ ./git-diff > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nYou can demonstrate this with git's own --version output, too:\n(but git --help detects the failure without this patch)\n\n $ ./git --version > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nNote that the fcntl test (for whether the fileno may be closed) is\nrequired in order to avoid EBADF upon closing an already-closed stdout,\nas would happen for each git command that already closes stdout; I think\nupdate-index was the one I noticed in the failure of t5400, before I\nadded that test.\n\nAlso, to be consistent with e.g., write_or_die, do not\ndiagnose EPIPE write failures.\n\nSigned-off-by: Jim Meyering \n---\n git.c | 19 ++++++++++++++++++-\n 1 files changed, 18 insertions(+), 1 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 29b55a1..8258885 100644\n--- a/git.c\n+++ b/git.c\n@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n \t\tstruct cmd_struct *p = commands+i;\n \t\tconst char *prefix;\n+\t\tint status;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n\n@@ -321,7 +322,23 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \t\t\tdie(\"%s must be run in a work tree\", cmd);\n \t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n\n-\t\texit(p->fn(argc, argv, prefix));\n+\t\tstatus = p->fn(argc, argv, prefix);\n+\n+\t\t/* Close stdout if necessary, and diagnose any failure\n+\t\t other than EPIPE. */\n+\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0) {\n+\t\t\terrno = 0;\n+\t\t\tif ((ferror(stdout) || fclose(stdout))\n+\t\t\t && errno != EPIPE) {\n+\t\t\t\tif (errno == 0)\n+\t\t\t\t\tdie(\"write failure on standard output\");\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"write failure on standard output\"\n+\t\t\t\t\t \": %s\", strerror(errno));\n+\t\t\t}\n+\t\t}\n+\n+\t\texit(status);\n \t}\n }\n\n--\n1.5.2.73.g18bece\n"},{"id":"43583","messageId":"87r6ozs7q5.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"alpine.LFD.0.98.0705281957160.26602@woody.linux-foundation.org","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-29T20:19:46Z","receivedAt":"2007-05-29T20:19:46Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n\n> On Mon, 28 May 2007, Jim Meyering wrote:\n>>\n>> Um... maybe you've forgotten that this patch fixes a hole in the\n>> \"old code\" (git.c). Many git tools ignore write (ENOSPC) failures.\n>\n> Maybe you have not noticed, but my argument has ben about EPIPE.\n\nHa ha. That's a good one.\nThe point was that even you must see that your\n\"[Jim's] WHOLE patch is crap\" statement was wrong.\n\n>> 1) Continue to ignore EPIPE write failure: can obscure real errors.\n>> BTW, Linus, don't you agree? You never commented on this point.\n>\n> THAT'S THE ONLY THING I'VE BEEN COMMENTING ON!\n>\n> They aren't \"obscure real errors\". EPIPE is neither obscure _nor_ an\n> error.\n\nReread #1, above.\n\"ignore EPIPE write failure: can obscure real errors\" (using \"obscure\"\nas a verb, not an adjective) means that ignoring EPIPE failure can cause\ngit commands to hide/mask/paper-over a real, conceptual error like this:\n\n # This is obviously bogus shell code:\n # \"cat\" with an argument like this doesn't read stdin\n git-rev-list HEAD | cat foo | ...\n\nWe're really on the head of a pin here, since the EPIPE test\nmakes a difference only when SIGPIPE is being ignored (unusual).\nNote that ignoring SIGPIPE can cause gross inefficiencies or\neven expose bugs. E.g., On Solaris 10, this infloops:\n\n (trap '' PIPE; /bin/yes) |head -1\n\nso there are good reasons *not* to ignore SIGPIPE. Add to that the fact\nthat the condition provoking an EPIPE is not *that* common, and you\nbegin to see that even if EPIPE is a little different, it doesn't matter\nenough to justify polluting every application file-close test with an\nEPIPE exemption.\n\n> The code-paths where you removed EPIPE handlign have two cases:\n> - SIGPIPE happens: you made no change\n> - SIGPIPE diesn't happen: you broke the code.\n>\n> So remind me again, why the hell do you think your patch is so great and\n> so important,\n\nWhoa! I guess you had a bad day, yesterday.\n\nI try to be humble, and certainly have not been crowing that this\ntiny patch is \"so great and so important.\" However, I did defend it\nwhen you claimed that the whole thing was crap.\n\n> considering that it broke real code, and made things worse?\n\nI believe it is an IMPROVEMENT to make such mistakes detectable (exit\nnonzero), and that the risk of annoying users with EPIPE diagnostics is\nminimal. You seem to think there would be some outpouring of \"broken\npipe\" errors, but since so few Porcelains ignore SIGPIPE, I disagree.\n\nHowever, it's an improvement only in the unusual event that SIGPIPE is\nbeing ignored, which is also when an application may output the \"broken\npipe\" error. IMHO, these conditions are rare enough (now) that there's\nno point in making an exception for EPIPE everywhere.\n\n> And why don't you just admit that EPIPE is special, isn't an error, and\n> shouldn't be complained about? If you get EPIPE on the write, it means\n> \"the other end didn't care\". It does NOT mean \"I should now do a really\n> annoying message\".\n\nSure, EPIPE is special, but it is so unusual now that it's not\nworth even the small added complexity to treat it specially in all\napplication code.\n\n> It's that simple. You seem to admit that SIGPIPE handling in bash should\n> have been fixed, and that it was annoying to complain about it there. Why\n\nYes. When bash-3.0 announced each process-killing-via-SIGPIPE with e.g.,\n /some/script: line 2: 31994 Broken pipe seq 99999\nthat was a big deal because it affected many scripts.\n\n> can't you just admit that it's annoyign and wrong to complain about the\n> same thing when it's EPIPE?\n\nIf it happened a lot, it *would* be annoying, but that's just it:\nit doesn't happen much at all anymore.\n\nAlso, no one is complaining about EPIPE diagnostics from any of\nthe GNU coreutils, and I take that as a good indication that there\nis no problem.\n"},{"id":"43585","messageId":"alpine.LFD.0.98.0705291412060.26602@woody.linux-foundation.org","threadId":"8324","inReplyTo":"87r6ozs7q5.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-29T21:19:41Z","receivedAt":"2007-05-29T21:19:41Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 29 May 2007, Jim Meyering wrote:\n> >\n> > Maybe you have not noticed, but my argument has ben about EPIPE.\n> \n> Ha ha. That's a good one.\n> The point was that even you must see that your\n> \"[Jim's] WHOLE patch is crap\" statement was wrong.\n\nEhh. That's a rather edited version of what I said, isn't it?\n\nThat's after I explicitly _quoted_ the part where you actively removed the \ncode that said \"EPIPE is right\", and also after I had told you several \ntimes that you should consider EPIPE as a special case in your other part.\n\nIn other words, yes, EVERY SINGLE HUNK of your patch was wrong, and I had \ntold you exactly why.\n\nHow wrong does a patch have to be to be \"crap\"? Maybe I have higher \nstandards than you do (apparently so), but \"every single hunk was wrong\" \nshould certainly be a damn good reason to consider _any_ patch crap, \nwouldn't you say?\n\nAnd now you have trouble accepting that, even after you have sent out a \nfixed patch without the crap. Thanks for finally bothering to get the \npatch right, but I don't see why you have to try to make-believe that it \nwas ever about anything but EPIPE.\n\nSo go back and read my emails. You'll see that in every single one I made \nit very clear that EPIPE was special. From the very first one (where I \ndidn't call your patch crap, btw: I said it was wrong, and that some \nerrors are expected and good, and I explicitly told you about EPIPE).\n\nSo what did you do? Instead of acknowledging that EPIPE was different, you \nactually *expanded* on that original patch, and made the other places \nwhere we _did_ handle EPIPE correctly, and made those places handle it \n_incorrectly_.\n\nAnd then you expect me to be _polite_ about it? Grow up. I was polite \nbefore you started explicitly doing the reverse of what I told you you \nshould do. At that point, your patch went from \"meant well, but the patch \nwas wrong\" to \"That's just obviously crap\".\n\n\t\tLinus\n"},{"id":"43590","messageId":"7v646b5gw5.fsf@assigned-by-dhcp.cox.net","threadId":"8324","inReplyTo":"87veebs84i.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-29T23:50:18Z","receivedAt":"2007-05-29T23:50:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> ...\n> Also, to be consistent with e.g., write_or_die, do not\n> diagnose EPIPE write failures.\n>\n> Signed-off-by: Jim Meyering \n> ---\n> git.c | 19 ++++++++++++++++++-\n> 1 files changed, 18 insertions(+), 1 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 29b55a1..8258885 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n> \tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n> \t\tstruct cmd_struct *p = commands+i;\n> \t\tconst char *prefix;\n> +\t\tint status;\n> \t\tif (strcmp(p->cmd, cmd))\n> \t\t\tcontinue;\n>\n> @@ -321,7 +322,23 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n> \t\t\tdie(\"%s must be run in a work tree\", cmd);\n> \t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n>\n> -\t\texit(p->fn(argc, argv, prefix));\n> +\t\tstatus = p->fn(argc, argv, prefix);\n> +\n> +\t\t/* Close stdout if necessary, and diagnose any failure\n> +\t\t other than EPIPE. */\n> +\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0) {\n> +\t\t\terrno = 0;\n> +\t\t\tif ((ferror(stdout) || fclose(stdout))\n> +\t\t\t && errno != EPIPE) {\n> +\t\t\t\tif (errno == 0)\n> +\t\t\t\t\tdie(\"write failure on standard output\");\n> +\t\t\t\telse\n> +\t\t\t\t\tdie(\"write failure on standard output\"\n> +\t\t\t\t\t \": %s\", strerror(errno));\n> +\t\t\t}\n\nThis makes the final write failure trump the breakage p->fn()\nalready diagnosed, doesn't it? Maybe if (fcntrl(...) >=0 )\nshould read if (!status && fcntrl(...) >= 0).\n\n> +\t\t}\n> +\n> +\t\texit(status);\n> \t}\n> }\n>\n> --\n> 1.5.2.73.g18bece\n"},{"id":"43625","messageId":"874pluss22.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"7v646b5gw5.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-30T07:12:53Z","receivedAt":"2007-05-30T07:12:53Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n>> -\t\texit(p->fn(argc, argv, prefix));\n>> +\t\tstatus = p->fn(argc, argv, prefix);\n>> +\n>> +\t\t/* Close stdout if necessary, and diagnose any failure\n>> +\t\t other than EPIPE. */\n>> +\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0) {\n>> +\t\t\terrno = 0;\n>> +\t\t\tif ((ferror(stdout) || fclose(stdout))\n>> +\t\t\t && errno != EPIPE) {\n>> +\t\t\t\tif (errno == 0)\n>> +\t\t\t\t\tdie(\"write failure on standard output\");\n>> +\t\t\t\telse\n>> +\t\t\t\t\tdie(\"write failure on standard output\"\n>> +\t\t\t\t\t \": %s\", strerror(errno));\n>> +\t\t\t}\n>\n> This makes the final write failure trump the breakage p->fn()\n> already diagnosed, doesn't it?\n\nYes. Are there circumstances in which a nonzero status from\nsome cmd_* function would mean something so grave that you\nwouldn't also want to know that standard output is incomplete\nor corrupt (and possibly use a different exit status)?\nSo far, after a quick and incomplete survey, I haven't found any.\n\nHowever, if some git command is documented to exit with\nstatus N for some listed values of N, e.g.,\n 1 A happened\n 2 B happened\n 3 any other failure\nthen the above choice of dying with \"die\" would be wrong.\n\nE.g. git-diff's --exit-code comes close:\n\n --exit-code\n Make the program exit with codes similar to diff(1). That is, it\n exits with 1 if there were differences and 0 means no differences.\n\nbut doesn't say how it handles errors.\n\n[ OT: Perhaps that documentation should be changed to look more like diff's,\n so that it says there is a different exit code for the third\n case (some failure):\n\n $ diff --help|tail -3|head -1\n Exit status is 0 if inputs are the same, 1 if different, 2 if trouble.\n]\n\n> Maybe if (fcntrl(...) >=0 )\n> should read if (!status && fcntrl(...) >= 0).\n\nNo, because then something like git-diff's --exit-code could hide\na write error.\n\nIf you want to preserve the exit status, then it should be enough\nto call set_die_routine with a function that will work just like\n\"die\" but exit with a specified (status) value.\n"},{"id":"43627","messageId":"87myzmr152.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"7v1wh0bpv2.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-30T11:39:37Z","receivedAt":"2007-05-30T11:39:37Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>\n>> Of course error messages are annoying when your short-pipe-read is\n>> _deliberate_ (tho, most real uses of git tools will actually get no\n>> message to be annoyed about[*]), but what if there really *is* a mistake?\n>> Try this:\n>>\n>> # You want to force git to ignore the error.\n>> $ trap '' PIPE; git-rev-list HEAD | sync\n>> $\n>\n> It is perfectly valid (although it is stupid) for a Porcelain\n> script to do this:\n>\n> latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)\n> case \"$latest_by_jim\" in\n> '') echo \"No commit by Jim\" ;;\n> *) # do something interesting on the commit\n> ;;;\n> esac\n\nHi Junio,\n\nThe above snippet (prepending a single #!/bin/bash line) doesn't provoke\nan EPIPE diagnostic from my patched git. In fact, even if you're using\nan old, unpatched version of bash, it provokes *no* diagnostic at all.\n\nTo provoke a diagnostic (from bash, not git), using old unpatched bash,\nyou need a script doing output from a subshell, e.g.:\n\n #!/tmp/bash-3.0/bash\n for x in 1; do\n git-log\n done | head -1\n\nWith unpatched bash-3.0, it does this:\n\n commit 42e3a6f676e9ae4e9640bc2ff36b7ab0b061a60e\n /tmp/bp-demo: line 2: 24864 Broken pipe git-log\n\nIt's only if you try to avoid the above and change your script to\nignore SIGPIPE do you finally get an offending EPIPE diagnostic:\n\n #!/tmp/bash-3.0/bash\n trap '' PIPE\n for x in 1; do\n ./git-log; echo $? 1>&2\n done | head -1\n\nHere's its output, using my patch:\n\n commit 42e3a6f676e9ae4e9640bc2ff36b7ab0b061a60e\n fatal: write failure on standard output\n 128\n\nThat trap has the nasty side effect of making the poorly written script\nwait until \"git-log\" has completed (before, it was interrupted right\naway), so it can take a lot longer. With my patch, it also gives a\ndiagnostic, which might serve to inform someone that they should not\nignore SIGPIPE.\n\nNo porcelain (modulo [*]) in git proper or cogito ignores SIGPIPE,\nso I don't see how EPIPE error diagnostics can be a problem.\n\n[*] These scripts do ignore SIGPIPE, but either don't need to,\nor can/should be fixed not to:\n\n git-archimport.perl\n git-cvsimport.perl\n git-svnimport.perl\n\nAnd, yes, I'd be happy to fix them, if anyone is interested.\n\n> In such a case, it is a bit too much for my taste to force the\n> script to redirect what comes out of fd 2 of the upstream of the\n> pipe, so that it can filter out only the \"write error\" message\n> but still show other kinds of error messages. You could do so\n> by elaborate shell magic, perhaps like this:\n>\n> filter_pipe_error () {\n> exec 3>&1\n> (eval \"$1\" 2>&1 1>&3 | grep >&2 -v 'Broken pipe')\n> }\n>\n> \tlatest_by_jim=$(filter_pipe_error \\\n> \t'git log --pretty=oneline --author='\\''Jim'\\'' | head -n 1'\n> \t)\n\nI agree that would be extreme. But it's not necessary, since the\n'Broken pipe' diagnostic appears now only in contrived circumstances.\n\n> but what's the point?\n>\n> I think something like this instead might be more palatable.\n\n...[patch to make git/EPIPE exit nonzero, but with no diagnostic]\n\nThank you for taking the time to reply and to come up with a compromise.\nAt first I thought this would be a step in the right direction, but,\nnow that I understand how infrequently EPIPE actually comes into play,\nI think it'd be better to avoid a half-measure fix, since that would\njust perpetuate the idea that EPIPE is worth handling specially.\n\nJim\n"},{"id":"43630","messageId":"87k5uqqz0y.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"alpine.LFD.0.98.0705291412060.26602@woody.linux-foundation.org","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-30T12:25:17Z","receivedAt":"2007-05-30T12:25:17Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n\n> On Tue, 29 May 2007, Jim Meyering wrote:\n>> >\n>> > Maybe you have not noticed, but my argument has ben about EPIPE.\n>>\n>> Ha ha. That's a good one.\n>> The point was that even you must see that your\n>> \"[Jim's] WHOLE patch is crap\" statement was wrong.\n>\n> Ehh. That's a rather edited version of what I said, isn't it?\n\nNo. I'm glad to see that perhaps even you are surprised by your words.\nThe only editing was to capitalize WHOLE. Here's what you wrote:\n\n > I think this patch is fundamentally WRONG. This fragment is just a prime\n > example of why the whole patch is crap. The old code was correct, and you\n > broke it.\n\nUmm... are the above three lines the only part of my message you're\nprepared to talk about? You haven't addressed any of the interesting\n(technical) parts.\n\n> That's after I explicitly _quoted_ the part where you actively removed the\n> code that said \"EPIPE is right\", and also after I had told you several\n> times that you should consider EPIPE as a special case in your other part.\n>\n> In other words, yes, EVERY SINGLE HUNK of your patch was wrong, and I had\n> told you exactly why.\n\nAnd I told you why I think my patch was on the right track.\ni.e., why it now appears to be fine to treat EPIPE like any other error.\n\nIt is interesting to see that you've elided all of my arguments rather\nthan make an attempt to rebut them. I'm trying to keep my side of this\ndiscussion professional, so I'll ignore parts of what you've written\nbelow.\n\n> How wrong does a patch have to be to be \"crap\"? Maybe I have higher\n> standards than you do...\n\n> And now you have trouble accepting that, even after you have sent out a\n> fixed patch without the crap. Thanks for finally bothering to get the\n> patch right, but I don't see why you have to try to make-believe that it\n> was ever about anything but EPIPE.\n\nMy original patch was about ENOSPC and EIO. EPIPE was mostly incidental.\nI don't care about EPIPE, and think it deserves no special treatment.\nYou appear to be obsessed with it now, perhaps because SIGPIPE-ignoring\nporcelain (now long gone) once caused trouble.\n\n> So go back and read my emails.\n\nDid you read mine? I explained why EPIPE is no longer a problem for\ngit, even if you're using stock bash-2.05b or bash-3.0.\n\n> You'll see that in every single one I made\n> it very clear that EPIPE was special.\n\nNo. You merely *said* it was special.\nYou haven't demonstrated that it's special enough (and still common\nenough) to pollute all code that tests for file-close failure. I hear\nthat it *used to be* common enough to merit such treatment. Now, it is\na much harder case to make. But from what I've seen, you haven't even\ntried to do that much. Hmm... or maybe you did try to make the case,\nand came up short. Is that why you are resorting to hyperbole, and ad\nhominem arguments?\n\n> From the very first one (where I\n> didn't call your patch crap, btw: I said it was wrong, and that some\n> errors are expected and good, and I explicitly told you about EPIPE).\n>\n> So what did you do? Instead of acknowledging that EPIPE was different, you\n> actually *expanded* on that original patch, and made the other places\n> where we _did_ handle EPIPE correctly, and made those places handle it\n> _incorrectly_.\n>\n> And then you expect me to be _polite_ about it? Grow up. I was polite\n> before you started explicitly doing the reverse of what I told you you\n> should do. At that point, your patch went from \"meant well, but the patch\n> was wrong\" to \"That's just obviously crap\".\n\nLet's see... I dared to post code contrary to your unsubstantiated claim,\nand therefore you describe that code as \"obviously crap\".\n\nJust because you are Linus doesn't mean you can decree that\n\"EPIPE must be ignored\" and make everyone take it on faith.\n\nCan you substantiate your claim that my proposed changes cause trouble\n*in practice*? So far, all I've heard is FUD, and all of my explanations\nof why EPIPE no longer matters seem to have been ignored.\n"},{"id":"43643","messageId":"alpine.LFD.0.98.0705300832410.26602@woody.linux-foundation.org","threadId":"8324","inReplyTo":"87k5uqqz0y.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-05-30T15:40:57Z","receivedAt":"2007-05-30T15:40:57Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 30 May 2007, Jim Meyering wrote:\n> \n> No. I'm glad to see that perhaps even you are surprised by your words.\n\nIf you thought I was a polite person and am surpised by calling things \ncrap after the fact, I'm afraid you have a few shocking moments coming to \nyou. My reply was fairly polite by my standards. My tag-line is often \"On \nthe internet, nobody can hear you being subtle\".\n\nSo let me rephrase my message to you:\n\n\tI think your INTERMEDIATE PATCH WAS TOTAL AND UTTER CRAP.\n\n\tEVERY SINGLE HUNK WAS SH*T. \n\n\tYou expressly IGNORED my point that some errors aren't errors, and \n\tMADE GIT WORSE.\n\nAre we on the same page now? \n\nIn contrast, your final patch was fine. The one where you finally fixed \nthe issue that I complained about FROM THE VERY BEGINNING.\n\nComprende?\n\n> The only editing was to capitalize WHOLE. Here's what you wrote:\n> \n> > I think this patch is fundamentally WRONG. This fragment is just a prime\n> > example of why the whole patch is crap. The old code was correct, and you\n> > broke it.\n> \n> Umm... are the above three lines the only part of my message you're\n> prepared to talk about? You haven't addressed any of the interesting\n> (technical) parts.\n\nUmm. Your final patch was a few trivial lines. I addressed all interesting \ntechnical parts IN MY ORIGINAL REPLY WHEN YOU FIRST POSTED IT.\n\nWhich you ignored (or rather, explicitly chose to disagree with, and \nadded MORE crap to the patch).\n\nGo away. I'm not interested in flaming you any more. The patch wasn't that \ninteresting to begin with, and you have shown that you're more interested \nin being contrary than to actually fix the problems that were pointed out \nto you _immediately_ and without any flames. \n\n\t\t\tLinus\n"},{"id":"43645","messageId":"87k5uqp9xu.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"alpine.LFD.0.98.0705300832410.26602@woody.linux-foundation.org","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-30T16:12:29Z","receivedAt":"2007-05-30T16:12:29Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n[...big flame!...]\n\nWow! This is like the scene from the Wizard of Oz:\nI've just raised the curtain and seen who's behind it.\n\nJim\n"},{"id":"43651","messageId":"7vd50i2o9c.fsf@assigned-by-dhcp.cox.net","threadId":"8324","inReplyTo":"87myzmr152.fsf@rho.meyering.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-30T17:51:43Z","receivedAt":"2007-05-30T17:51:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> Junio C Hamano wrote:\n>> Jim Meyering writes:\n>>\n>>> Of course error messages are annoying when your short-pipe-read is\n>>> _deliberate_ (tho, most real uses of git tools will actually get no\n>>> message to be annoyed about[*]), but what if there really *is* a mistake?\n>>> Try this:\n>>>\n>>> # You want to force git to ignore the error.\n>>> $ trap '' PIPE; git-rev-list HEAD | sync\n>>> $\n>>\n>> It is perfectly valid (although it is stupid) for a Porcelain\n>> script to do this:\n>>\n>> latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)\n>> case \"$latest_by_jim\" in\n>> '') echo \"No commit by Jim\" ;;\n>> *) # do something interesting on the commit\n>> ;;;\n>> esac\n>\n> Hi Junio,\n>\n> The above snippet (prepending a single #!/bin/bash line) doesn't provoke\n> an EPIPE diagnostic from my patched git. In fact, even if you're using\n> an old, unpatched version of bash, it provokes *no* diagnostic at all.\n\n> To provoke a diagnostic (from bash, not git), using old unpatched bash,\n> you need a script doing output from a subshell, e.g.:\n>\n> #!/tmp/bash-3.0/bash\n> for x in 1; do\n> git-log\n> done | head -1\n\nI haven't thought it through, but isn't the above example only\ntalking about the \"Broken pipe\" message? Surely, you would get\nthat message from older Bash if you have a shell loop on the\nupstream side of the pipe no matter what we (the command that is\nrun by the shell loop) do, and trap is needed to squelch it.\n\nBut I do not see how this pipeline, where git-rev-list produces\nmore than what fits in the in-kernel pipe buffer:\n\n\t\"git-rev-list a lot of data | head -n 1\"\n\nwould not catch EPIPE and say \"Broken Pipe\" with your patch.\nEspecially if the downstream is sufficiently slow (say, replace\nit with \"(sleep 10 && head -n 1)\", perhaps), wouldn't the\nupstream produce enough without being read, gets stuck on write,\nand when the downstream exits, it would notice its write(2)\nfailed with EPIPE, wouldn't it?\n\nMaybe you are talking about your updated patch?\n\n> ...[patch to make git/EPIPE exit nonzero, but with no diagnostic]\n>\n> Thank you for taking the time to reply and to come up with a compromise.\n> At first I thought this would be a step in the right direction, but,\n> now that I understand how infrequently EPIPE actually comes into play,\n> I think it'd be better to avoid a half-measure fix, since that would\n> just perpetuate the idea that EPIPE is worth handling specially.\n\nAfter having read what Linus said about how the \"fixed\" one\nwould behave differently, depending on the amount of data we\nproduce before the consumer says \"I've seen enough\" and\ndepending on the amount of data that would fit in the in-kernel\npipe buffer, I no longer think the compromise patch you mention\nabove is improvement anymore.\n"},{"id":"43659","messageId":"87abvmp05o.fsf@rho.meyering.net","threadId":"8324","inReplyTo":"7vd50i2o9c.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-05-30T19:43:47Z","receivedAt":"2007-05-30T19:43:47Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>> Junio C Hamano wrote:\n>>> Jim Meyering writes:\n>>>\n>>>> Of course error messages are annoying when your short-pipe-read is\n>>>> _deliberate_ (tho, most real uses of git tools will actually get no\n>>>> message to be annoyed about[*]), but what if there really *is* a mistake?\n>>>> Try this:\n>>>>\n>>>> # You want to force git to ignore the error.\n>>>> $ trap '' PIPE; git-rev-list HEAD | sync\n>>>> $\n>>>\n>>> It is perfectly valid (although it is stupid) for a Porcelain\n>>> script to do this:\n>>>\n>>> latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)\n>>> case \"$latest_by_jim\" in\n>>> '') echo \"No commit by Jim\" ;;\n>>> *) # do something interesting on the commit\n>>> ;;;\n>>> esac\n>>\n>> Hi Junio,\n>>\n>> The above snippet (prepending a single #!/bin/bash line) doesn't provoke\n>> an EPIPE diagnostic from my patched git. In fact, even if you're using\n>> an old, unpatched version of bash, it provokes *no* diagnostic at all.\n>\n>> To provoke a diagnostic (from bash, not git), using old unpatched bash,\n>> you need a script doing output from a subshell, e.g.:\n>>\n>> #!/tmp/bash-3.0/bash\n>> for x in 1; do\n>> git-log\n>> done | head -1\n>\n> I haven't thought it through, but isn't the above example only\n> talking about the \"Broken pipe\" message? Surely, you would get\n\nI'm not sure which \"Broken pipe\" message you mean.\n\nThere are two types of \"Broken pipe\" messages.\nThere's the old, verbose one from bash that includes script line-number,\npid, and killed command name. Old, unpatched versions of bash\nprint that message whenever bash kills a process with SIGPIPE.\n\nThen there's the application (EPIPE) one, that can be printed\nby a writing application like git,cat,seq,etc. only when SIGPIPE\nstops a write but doesn't kill the writer.\nIn that case, the write syscall fails with errno==EPIPE,\nand if it's diagnosed by the application, you get e.g.,\n\n seq: write error: Broken pipe\n\nSince the script above is not ignoring SIGPIPE, the git-log process\nis killed by bash-3.0, and that old version of bash announces the killing\nwith the verbose message:\n\n /t/bp-demo: line 2: 14474 Broken pipe git-log\n\nAny other patched or newer version of bash will print the\nsingle requested line on stdout and nothing on stderr.\n\n> that message from older Bash if you have a shell loop on the\n> upstream side of the pipe no matter what we (the command that is\n> run by the shell loop) do, and trap is needed to squelch it.\n\nRight.\nThat's why no one is using such broken shells anymore.\nAnd why no porcelain tools incur the penalty of ignoring\nSIGQUIT anymore either.\n\n> But I do not see how this pipeline, where git-rev-list produces\n> more than what fits in the in-kernel pipe buffer:\n>\n> \t\"git-rev-list a lot of data | head -n 1\"\n>\n> would not catch EPIPE and say \"Broken Pipe\" with your patch.\n\nI haven't debugged the old bash to see why that first one\nfails to provoke a broken pipe message (from bash).\nUnless you add a line like \"trap '' PIPE\" before it, bash kills\nthe writer with SIGPIPE, and so my patch is irrelevant,\nbecause the failing write syscall never returns.\n\n> Especially if the downstream is sufficiently slow (say, replace\n> it with \"(sleep 10 && head -n 1)\", perhaps), wouldn't the\n> upstream produce enough without being read, gets stuck on write,\n> and when the downstream exits, it would notice its write(2)\n> failed with EPIPE, wouldn't it?\n\nAre you presuming SIGPIPE is ignored?\n\n> Maybe you are talking about your updated patch?\n\nI was talking about the initial patch, or the one that\nalso removed the errno == EPIPE tests.\n"}]}