Re: (resend) [PATCH] Don't ignore write failure from git-diff, git-log, etc.

Jim Meyering [off-list ref] writes:

From: Jim Meyering <redacted>
Date: Sat, 26 May 2007 13:43:07 +0200
Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.

Currently, when git-diff writes to a full device or gets an I/O error,
it fails to detect the write error:
...
Also, to be consistent with e.g., write_or_die, do not
diagnose EPIPE write failures.

I still do not like the fact that this patch makes an error from
the final stdout flushing override the return value from p->fn()
even when the function already diagnosed an error, but otherwise
I think it is a good change, as it allows us to catch one error
case that we currently don't, without introducing an annoying
EPIPE diagnosis.

Naks, or vetoes?

On Sun, 24 Jun 2007, Junio C Hamano wrote:

I still do not like the fact that this patch makes an error from
the final stdout flushing override the return value from p->fn()
even when the function already diagnosed an error

Yeah.

I also don't think it's very _pretty_ code, and it violates my personal 
coding standards by adding way too deep indentation for the new error 
cases. It was already three indents deep (reasonably fine, but that 
NOT_BARE test wass already pretty ugly), but now it becomes five 
indentation levels deep at its deepest, which is just a sign that things 
should be split up.

I'd also like to know why it does that fcntl() is done, and I also wonder 
about that "ferror()" call: it is entirely possible that ferror() is set 
due to EPIPE, and in that case, it will *not* set errno to EPIPE at all, 
so it will *still* complain about what I consider an invalid situation.

I dunno. I think the ENOSPC worry is a very real and valid one, but I 
would really tend prefer something different.

How about this following series of two patches instead, which I'll send as 
replies to this email..

		Linus

This should change no code at all, it just moves the definition of "struct 
cmd_struct" out, and then splits out the running of the right command into 
the "run_command()" function.

It also removes the long-unused 'envp' pointer passing.

This is just preparation for adding some more error checking.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

 git.c |   52 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/git.c b/git.c
index 29b55a1..6c728e4 100644
--- a/git.c
+++ b/git.c

@@ -216,14 +216,34 @@ const char git_version_string[] = GIT_VERSION;
  */
 #define NOT_BARE 	(1<<2)
 
-static void handle_internal_command(int argc, const char **argv, char **envp)
+struct cmd_struct {
+	const char *cmd;
+	int (*fn)(int, const char **, const char *);
+	int option;
+};
+
+static NORETURN void run_command(struct cmd_struct *p, int argc, const char **argv)
+{
+	const char *prefix;
+
+	prefix = NULL;
+	if (p->option & RUN_SETUP)
+		prefix = setup_git_directory();
+	if (p->option & USE_PAGER)
+		setup_pager();
+	if (p->option & NOT_BARE) {
+		if (is_bare_repository() || is_inside_git_dir())
+			die("%s must be run in a work tree", p->cmd);
+	}
+	trace_argv_printf(argv, argc, "trace: built-in: git");
+
+	exit(p->fn(argc, argv, prefix));
+}
+
+static void handle_internal_command(int argc, const char **argv)
 {
 	const char *cmd = argv[0];
-	static struct cmd_struct {
-		const char *cmd;
-		int (*fn)(int, const char **, const char *);
-		int option;
-	} commands[] = {
+	static struct cmd_struct commands[] = {
 		{ "add", cmd_add, RUN_SETUP | NOT_BARE },
 		{ "annotate", cmd_annotate, RUN_SETUP | USE_PAGER },
 		{ "apply", cmd_apply },

@@ -307,25 +327,13 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
 		struct cmd_struct *p = commands+i;
-		const char *prefix;
 		if (strcmp(p->cmd, cmd))
 			continue;
-
-		prefix = NULL;
-		if (p->option & RUN_SETUP)
-			prefix = setup_git_directory();
-		if (p->option & USE_PAGER)
-			setup_pager();
-		if ((p->option & NOT_BARE) &&
-				(is_bare_repository() || is_inside_git_dir()))
-			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));
+		run_command(p, argc, argv);
 	}
 }
 
-int main(int argc, const char **argv, char **envp)
+int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] ? argv[0] : "git-help";
 	char *slash = strrchr(cmd, '/');

@@ -358,7 +366,7 @@ int main(int argc, const char **argv, char **envp)
 	if (!prefixcmp(cmd, "git-")) {
 		cmd += 4;
 		argv[0] = cmd;
-		handle_internal_command(argc, argv, envp);
+		handle_internal_command(argc, argv);
 		die("cannot handle %s internally", cmd);
 	}
 

@@ -390,7 +398,7 @@ int main(int argc, const char **argv, char **envp)
 
 	while (1) {
 		/* See if it's an internal command */
-		handle_internal_command(argc, argv, envp);
+		handle_internal_command(argc, argv);
 
 		/* .. then try the external ones */
 		execv_git_cmd(argv);

This is trying to implement the strict IO error checks that Jim Meyering 
suggested, but explicitly limits it to just regular files. If a pipe gets 
closed on us, we shouldn't complain about it.

[ Side note: feel free to change the S_ISREG() to a !S_ISFIFO(). That 
  would allow easier testing with /dev/full, which tends to be a character 
  special device. That said, I think sockets and pipes are generally 
  interchangeable, which is why I limited it to just regular files. ]

If the subcommand already returned an error, that takes precedence (and we 
assume that the subcommand already printed out any relevant messages 
relating to it)

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Hmm? I'm not saying this is the only way to do this, but I think this is 
at least likely to be obviously just an improvement, and it leaves room 
for further tweaking of the logic if Jim or others find other cases that 
should be handled.

Side note: I think I made a mistake in making the run_command() a NORETURN 
function and putting the exit() into it. It's probably better to instead 
just make it return "int", and make the caller do

	exit(run_command(...));

and that makes it much prettier to have "run_command()" just return early 
if an error happens (or doesn't happen).

For example, then we could just do

	status = p->fn(...);
	if (status)
		return status;
	/* Somebody closed stdout? */
	if (fstat(fileno(stdout), &st))
		return 0;
	/* Ignore write errors for pipes and sockets.. */
	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
		return 0;

which makes it easy to explain what's going on, and avoids having any deep 
indentation at all.

I dunno. This passes all the tests, but it's not like we currently test 
for ENOSPC/EIO anyway, or even can do that. If we _just_ disable the thing 
for pipes/sockets, we could add a test using /dev/full.

I did check that changing it to !S_ISFIFO() gets the right behaviour, and 
"git log | head" doesn't complain, while "git log > /dev/full" does. So 
this has gotten some very rudimentary testing, but not in the exact form 
I'm actually sending it out.

 git.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 6c728e4..db0118a 100644
--- a/git.c
+++ b/git.c

@@ -224,6 +224,8 @@ struct cmd_struct {
 
 static NORETURN void run_command(struct cmd_struct *p, int argc, const char **argv)
 {
+	int status;
+	struct stat st;
 	const char *prefix;
 
 	prefix = NULL;

@@ -237,7 +239,18 @@ static NORETURN void run_command(struct cmd_struct *p, int argc, const char **ar
 	}
 	trace_argv_printf(argv, argc, "trace: built-in: git");
 
-	exit(p->fn(argc, argv, prefix));
+	status = p->fn(argc, argv, prefix);
+	if (status)
+		exit(status);
+
+	/* Check for ENOSPC and EIO errors.. */
+	if (!fstat(fileno(stdout), &st) && S_ISREG(st.st_mode)) {
+		if (ferror(stdout))
+			die("write failure on standard output");
+		if (fflush(stdout) || fclose(stdout))
+			die("write failure on standard output: %s", strerror(errno));
+	}
+	exit(0);
 }
 
 static void handle_internal_command(int argc, const char **argv)

Linus Torvalds [off-list ref] writes:

Side note: I think I made a mistake in making the run_command() a NORETURN 
function and putting the exit() into it. It's probably better to instead 
just make it return "int", and make the caller do

	exit(run_command(...));

and that makes it much prettier to have "run_command()" just return early 
if an error happens (or doesn't happen).

For example, then we could just do

	status = p->fn(...);
	if (status)
		return status;
	/* Somebody closed stdout? */
	if (fstat(fileno(stdout), &st))
		return 0;
	/* Ignore write errors for pipes and sockets.. */
	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
		return 0;

which makes it easy to explain what's going on, and avoids having any deep 
indentation at all.

I took the liberty of munging your two patches to follow your
comments above (it was a perfect guinea-pig opportunity for
Johannes's "rebase -i").

The changes to git.c (run_command) conflicted with GIT_WORK_TREE
changes in a minor way.  Matthias, could you sanity check the
result once I push it out to 'next', please?

Hi Junio,

On Mon, 25 Jun 2007, Junio C Hamano wrote:

I took the liberty of munging your two patches to follow your comments 
above (it was a perfect guinea-pig opportunity for Johannes's "rebase 
-i").

And? How was that experience?

I am actually quite pleased how it worked out in the end; I never 
understood the syntax of rebase, and stayed away from it for that reason. 
By reworking patch-series into rebase -i, I learnt it on the way, and find 
it actually quite useful.

One thing we might consider, however: when rebasing, the current branch 
gets updated at each step. Some might consider this a bug, and prefer 
rebase to work on a detached HEAD, and only update the branch at the end, 
so that <branchname>@{1} refers to the state _before_ rebase.

Thoughts?

Ciao,
Dscho

Johannes Schindelin wrote:

One thing we might consider, however: when rebasing, the current branch
gets updated at each step. Some might consider this a bug, and prefer
rebase to work on a detached HEAD, and only update the branch at the end,
so that <branchname>@{1} refers to the state _before_ rebase.

YESSSS!

Finding the commit before the rebase in the reflog is a nightmare.

-- Hannes

Junio C Hamano [off-list ref] wrote:

The changes to git.c (run_command) conflicted with GIT_WORK_TREE
changes in a minor way.  Matthias, could you sanity check the
result once I push it out to 'next', please?

The changes look fine, the tests pass and my own short manual test
passed.