Re: [PATCH] Don't fflush(stdout) when it's not helpful

Jeff King [off-list ref] writes:

On Thu, Jun 28, 2007 at 11:48:38PM -0400, Theodore Tso wrote:

quoted

+void maybe_flush_or_die(FILE *f, const char *desc)
+{
+	static int stdout_is_file = -1;
+	struct stat st;
+	char *cp;
+
+	if (f == stdout) {
+		if (stdout_is_file < 0) {
+			cp = getenv("GIT_FLUSH");
+			if (cp)
+				stdout_is_file = (atoi(cp) == 0);
+			else if ((fstat(fileno(stdout), &st) == 0) &&
+				 S_ISREG(st.st_mode))
+				stdout_is_file = 1;
...

Looks much better to me, but I have one minor nit: stdout_is_file is a
poor name,...

Thanks for bringing it up, as I had the same "Huh?" moment.
I would probably call that simply "do_not_flush".  Or name the
variable "flush_stdout" and swap all the logic.

	if (f == stdout) {
        	if (flush_stdout < 0) {
                	cp = getenv("GIT_FLUSH_STDOUT");
                        if (cp)
                        	flush_stdout = !!atoi(cp);
			else if ((fstat(fileno(stdout), &st) == 0) &&
				!S_ISREG(st.st_mode))
				flush_stdout = 0;
			else
                        	flush_stdout = 1;
		}
                if (!flush_stdout)
                	return;
	}

On Fri, 29 Jun 2007, Junio C Hamano wrote:

Thanks for bringing it up, as I had the same "Huh?" moment.
I would probably call that simply "do_not_flush".  Or name the
variable "flush_stdout" and swap all the logic.

I think that patch looks fine, but I also think that there is a more 
fundamental problem with this approach:

 - all these patches basically break the whole _point_ of Jim's original 
   reason for wanting this!

So remember, we had two different reasons for flushing:

 - interactivity over a pipe. 

   Tools like "gitk" and "git gui blame" all run waiting for input, and we 
   want to give them the commit data as soon as possible.

 - error checking on a filesystem.

   Yes, "ferror()" _after_ the write shows an error happened, but doesn't 
   show what error it was. Doing a fflush() is a lot more likely to 
   actually give the right errors.

So non-files want flushing due to latency issues, and files want flushing 
due to error handling. And the patch under discussion basically broke the 
error handling.

(It turns out that it doesn't break it for the trivial "/dev/full" 
testcase, since that doesn't show up as a file, but it would break it for 
the *real* case of a filesystem that becomes full).

One option is to change the git.c thing to do

	if (fflush(stdout))
		die("write failure on standard output: %s", strerror(errno));
	if (ferror(stdout))
		die("unknown write failure on standard output");
	if (fclose(stdout))
		die("close failure on standard output: %s", strerror(errno));

where the "fflush()" is done first (regardless of ferror), just in the 
(probably futile) hope of getting the right errno.

		Linus

On Fri, Jun 29, 2007 at 09:06:22AM -0700, Linus Torvalds wrote:

I think that patch looks fine, but I also think that there is a more 
fundamental problem with this approach:

 - all these patches basically break the whole _point_ of Jim's original 
   reason for wanting this!

Yeah, I pointed that out in my first patch.  It had seemed that
interactivity over a pipe was considered more important, though when
we started talking about things.  :-)

It looks like from my reading of the standard that ferror(f) should
not change the state of the file handle f.  So the following patch I
think should work; it checks ferror(f), and if it indicates that there
is an error, we try a flush to get the error message.  I've tested
under Linux and it gives the correct error message in the "git log >
/mnt/full-filesystem" case, and I believe it should DTRT on other
systems.

Comments?

						- Ted

commit 93a96f94028106687412acbb771bb18ee7ec5560
Author: Theodore Ts'o [off-list ref]
Date:   Thu Jun 28 14:10:58 2007 -0400

    Don't fflush(stdout) when it's not helpful
    
    This patch arose from a discussion started by Jim Meyering's patch
    whose intention was to provide better diagnostics for failed writes.
    Linus proposed a better way to do things, which also had the added
    benefit that adding a fflush() to git-log-* operations and incremental
    git-blame operations could improve interactive respose time feel, at
    the cost of making things a bit slower when we aren't piping the
    output to a downstream program.
    
    This patch skips the fflush() calls when stdout is a regular file, or
    if the environment variable GIT_FLUSH is set to "0".  This latter can
    speed up a command such as:
    
    GIT_FLUSH=0 strace -c -f -e write time git-rev-list HEAD | wc -l
    
    a tiny amount.
    
    Cc: Linus Torvalds [off-list ref]
    Signed-off-by: "Theodore Ts'o" [off-list ref]

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 20b5b7b..8269148 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -396,6 +396,16 @@ other
 'GIT_PAGER'::
 	This environment variable overrides `$PAGER`.
 
+'GIT_FLUSH'::
+	If this environment variable is set to "1", then commands such
+	as git-blame (in incremental mode), git-rev-list, git-log,
+	git-whatchanged, etc., will force a flush of the output stream
+	after each commit-oriented record have been flushed.   If this
+	variable is set to "0", the output of these commands will be done
+	using completely buffered I/O.   If this environment variable is
+	not set, git will choose buffered or record-oriented flushing
+	based on whether stdout appears to be redirected to a file or not.
+
 'GIT_TRACE'::
 	If this variable is set to "1", "2" or "true" (comparison
 	is case insensitive), git will print `trace:` messages on

diff --git a/builtin-blame.c b/builtin-blame.c
index f7e2c13..da23a6f 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c

@@ -1459,6 +1459,7 @@ static void found_guilty_entry(struct blame_entry *ent)
 				printf("boundary\n");
 		}
 		write_filename_info(suspect->path);
+		maybe_flush_or_die(stdout, "stdout");
 	}
 }
 

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 813aadf..86db8b0 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);
+	maybe_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..0525c4e 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 maybe_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..ced3f33 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;
+	maybe_flush_or_die(stdout, "stdout");
 	return shown;
 }

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

@@ -1,5 +1,45 @@
 #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).
+ *
+ * If the file handle is stdout, and stdout is a file, then skip the
+ * flush entirely since it's not needed.
+ */
+void maybe_flush_or_die(FILE *f, const char *desc)
+{
+	static int skip_stdout_flush = -1;
+	struct stat st;
+	char *cp;
+
+	if (f == stdout) {
+		if (skip_stdout_flush < 0) {
+			cp = getenv("GIT_FLUSH");
+			if (cp)
+				skip_stdout_flush = (atoi(cp) == 0);
+			else if ((fstat(fileno(stdout), &st) == 0) &&
+				 S_ISREG(st.st_mode))
+				skip_stdout_flush = 1;
+			else
+				skip_stdout_flush = 0;
+		}
+		if (skip_stdout_flush && !ferror(f))
+			return;
+	}
+	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;

On Fri, 29 Jun 2007, Theodore Tso wrote:

Comments?

Looks ok to me. 

This should probably be paired up with the change to git.c (in "next") to 
do the "fflush()" before the "ferror()" too, in case the error is pending.

		Linus