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;
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
