Re: [PATCH] git-rev-list: give better diagnostic for failed write

On Mon, 25 Jun 2007, Linus Torvalds wrote:

If you really really *must* get that ENOSPC error string output, create a 
helper function like

	void flush_or_die(FILE *f, const char *desc)
	{
		if (ferror(f))
			die("write failure on %s", desc)

Actually, even this is really nasty, and it's a case where the current 
"git.c" code can also fail.

It's an example of where EPIPE can actually cause a write failure that you 
can never figure out what the reason for the error was, because the flush 
was caused by something earlier (write too much for the buffer or 
whatever), exactly because stdio throws the error away.

So after thinking about it some more, I would suggest just ignoring ferror 
entirely, and hoping that any errors are caught by the fflush().

I do hate stdio error checking. In my opinion, there really is only *one* 
correct way to use stdio error checking: ignore it. It doesn't work. The 
thing is fundamentally mis-designed for error handling.

So I think the right solution would literally be to either not do this 
broken error checking at all, or to rewrite the code that cares about 
errors to not use stdio.

There's also another issue: regular files really *are* different from 
pipes and sockets and other things. Not because of EPIPE, but because you 
want different buffering behaviour. For a regular file, we really don't 
even care about the line buffering, and we'd actually be better off (from 
a performance angle) without it.

But we don't have any sane way to save that kind of information (and we 
definitely do *not* want to do the "fstat()" thing on every flush). We 
could use the stdio buffering mode, but

 - it's too weak (we want not line buffered or block buffered, we want 
   basically "record buffered")

 - I don't think there are any portable ways to read it (only set it, 
   using "setbuf()" and friends).

Anyway, getting rid of stdio for writes we care about really *would* be a 
nice thing. But it's a lot of boring, nasty work.

So here's a patch that I think is acceptable. IT IS NOT PERFECT. Stdio 
simply cannot do a good job on errors, but it has a comment about the case 
where it just decides to ignore ferror() instead of doing what I suggest 
above.

I'm not saying this is great. But doing this right really does require 
avoiding stdio entirely.

Does this work for you?

(I pass the "desc" string thing in case there is some future use where we 
also want to use stdio, but we use it for something else than just regular 
stdout. Dunno).

		Linus

---
 builtin-rev-list.c |    2 +-
 cache.h            |    2 ++
 log-tree.c         |    1 +
 write_or_die.c     |   20 ++++++++++++++++++++
 4 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 813aadf..3980bf4 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c

@@ -100,7 +100,7 @@ static void show_commit(struct commit *commit)
 		printf("%s%c", buf, hdr_termination);
 		free(buf);
 	}
-	fflush(stdout);
+	flush_or_die(stdout, "stdout");
 	if (commit->parents) {
 		free_commit_list(commit->parents);
 		commit->parents = NULL;

diff --git a/cache.h b/cache.h
index ed83d92..aae9e2a 100644
--- a/cache.h
+++ b/cache.h

@@ -532,6 +532,8 @@ extern char git_default_name[MAX_GITNAME];
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
 
+/* IO helper functions */
+extern void flush_or_die(FILE *, const char *);
 extern int copy_fd(int ifd, int ofd);
 extern int read_in_full(int fd, void *buf, size_t count);
 extern int write_in_full(int fd, const void *buf, size_t count);

diff --git a/log-tree.c b/log-tree.c
index 0cf21bc..061ecf7 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 		shown = 1;
 	}
 	opt->loginfo = NULL;
+	flush_or_die(stdout, "stdout");
 	return shown;
 }

diff --git a/write_or_die.c b/write_or_die.c
index 5c4bc85..84f411d 100644
--- a/write_or_die.c
+++ b/write_or_die.c

@@ -1,5 +1,25 @@
 #include "cache.h"
 
+/*
+ * Some cases use stdio, but want to flush after the write
+ * to get error handling (and to get better interactive
+ * behaviour - not buffering excessively).
+ *
+ * Of course, if the flush happened within the write itself,
+ * we've already lost the error code, and cannot report it any
+ * more. So we just ignore that case instead (and hope we get
+ * the right error code on the flush).
+ */
+void flush_or_die(FILE *f, const char *desc)
+{
+	if (fflush(f)) {
+		if (errno == EPIPE)
+			exit(0);
+		die("write failure on %s: %s",
+			desc, strerror(errno));
+	}
+}
+
 int read_in_full(int fd, void *buf, size_t count)
 {
 	char *p = buf;