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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: May 29, 2007, 03:47 UTC
Message-ID: <alpine.LFD.0.98.0705282031280.26602@woody.linux-foundation.org>
In-Reply-To: <7v1wh0bpv2.fsf@assigned-by-dhcp.cox.net>

On Mon, 28 May 2007, Junio C Hamano wrote:

Show 19 quoted lines

> 
> I think something like this instead might be more palatable.
> 
> diff --git a/write_or_die.c b/write_or_die.c
> index 5c4bc85..fadfcaa 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -41,8 +41,8 @@ int write_in_full(int fd, const void *buf, size_t count)
>  void write_or_die(int fd, const void *buf, size_t count)
>  {
>  	if (write_in_full(fd, buf, count) < 0) {
> -		if (errno == EPIPE)
> -			exit(0);
> - 		die("write error (%s)", strerror(errno));
> +		if (errno != EPIPE)
> +			die("write error (%s)", strerror(errno));
> +		exit(1);
>  	}
>  }

It's certainly less annoying, but I don't actually think it's *correct*.

I think the current behaviour is simply _superior_.

Think about what happens whenever somebody does

	git cmd | head

and then you want to see whether the end result was successful or not.

What would the error code from the "git cmd" thing mean?

I claim that exiting with SUCCESS is actually the rigt thing to do for the git command in question. It did what it was asked for.

And more importantly, considering that a pipe is of indeterminate size, what happens if it actuall yonly had 15 lines to be printed out. Guess what? If it writes them fast enough, they'll go into the pipe, and "head" will exit only after the write, and we'll never even *know* that the last five lines were ignored, ie there won't be a EPIPE at all.

In other words, even Jim's example of

	git log | sync

will actually *succeed* even with Jim's patch, if the output fit in the kernel pipe buffer, and the "git log" command ran quickly enough that "sync" took longer (which is not at all unlikely).

So EPIPE really _is_ special: because when you write to a pipe, there's no guarantee that you'll get it at all, so whenever you get an EPIPE you should ask yourself:

 - what would I have done if the data had fit in the 64kB kernel buffer?

 - should I really return a different error message or complain just 
   because I just happened to notice that the reader went away _this_ 
   time, even if I might not notice it next time?

In other words, the "exit(0)" is actually _more_ consistent than "exit(1)", because exiting with an error message or with an error return is going to depend on luck and timing.

For example, I just did a

	strace git show | sync

on my kernel archive, and strace shows me that I had:

	...
	write(1, "commit c420bc9f09a0926b708c3edb2"..., 736) = 736
	exit_group(0)

the first three times I tried it, and then

	write(1, "commit c420bc9f09a0926b708c3edb2"..., 736) = -1 EPIPE (Broken pipe)
	--- SIGPIPE (Broken pipe) @ 0 (0) ---
	+++ killed by SIGPIPE +++

the fourth time.

What should we learn from this?

 - a shell that talks about SIGPIPE is going to be universally *hated*, 
   because it's really a total crap-shoot. Even with something like 
   "sync", that doesn't read a single byte from the input, most of the 
   time I didn't actually get EPIPE/SIGPIPE at all!

 - by exactly the same token, considering "EPIPE" as anything else than a 
   "success" is always just going to lead you to random behaviour.

So what _should_ you do for EPIPE?

Here's what EPIPE _really_ means:

 - you might as well consider the write a success, but the reader isn't 
   actually interested, so rather than go on, you might as well stop 
   early.

Notice how I very carefull avoided the word "error" anywhere. Because it's really not an error. The reader already got everything it wanted. So EPIPE should generally be seen as an "early success" rather than as a "failure".

		Linus