Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
From: Marco Roeland <hidden>
Date: 2016-06-15 22:43:13
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc. · Linus Torvalds <torvalds@linux-foundation.org>
- 2016-06-15 · Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc. · Jim Meyering <hidden>
- 2016-06-15 · Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc. · Marco Roeland <hidden>
- 2016-06-15 · Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc. · Jim Meyering <hidden>
- 2016-06-15 · Re: [PATCH] Don't ignore write failure from git-diff, git-log, etc. · Jim Meyering <hidden>
On monday May 28th 2007 at 15:14 Jim Meyering wrote:
quoted
I don't know how many people remember all the _stupid_ problems we had exactly because many versions of bash are crap, crap, crap, and people (including you) don't realize that EPIPE is _different_ from other write errors. Just do a google search for "broken pipe" bash and not only will you see a lot of complaints, but the #5 entry is a complaint for a git issue that we had tons of problems with. See for example http://www.gelato.unsw.edu.au/archives/git/0504/2602.htmlWhether bash should print a diagnostic when it kills a process with SIGPIPE is _different_ from whether the writing process should diagnose its own write failure arising from a handled SIGPIPE. I suspect that git's special treatment of EPIPE was a shoot-the-messenger reaction to the work-around (trap '' PIPE) required to avoid diagnostics from porcelain being interpreted by what would now be a 2-year-old version of bash. It is time to remove that work-around, because it can obscure real errors, and removing it will be largely unnoticed.
Good point. But also notice that when you are stuck with a shell that does complain about SIGPIPE it is _really_ annoying! On current Debian 'sid' with your patch this does not seem to be the case. The second chunk of your patch (to git.c) contains a small copy-paste accident methinks:
quoted hunk ↗ jump to hunk
@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp) die("%s must be run in a work tree", cmd); trace_argv_printf(argv, argc, "trace: built-in: git"); - exit(p->fn(argc, argv, prefix)); + status = p->fn(argc, argv, prefix); + + /* Close stdout if necessary, and diagnose any failure. */ + if (fcntl(fileno (stdout), F_GETFD) >= 0) + && (ferror(stdout) || fclose(stdout))) + die("write failure on standard output: %s", + strerror(errno)); + + exit(status);
The if statement with 'fcntl' is missing a brace, it should be: + if ((fcntl(fileno (stdout), F_GETFD) >= 0) + && (ferror(stdout) || fclose(stdout))) -- Marco Roeland