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

On Mon, 25 Jun 2007, Linus Torvalds wrote:

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.

Just for fun, I tried this out.

Doing

	time git log > logfile

on the kernel repo with and without the patch I just sent out, I get:

Without:

	real    0m1.361s
	user    0m1.312s
	sys     0m0.040s

With:

	real    0m1.687s
	user    0m1.392s
	sys     0m0.284s

so doing the extra flushing does actually cost us (it's just fundamentally 
more expensive to do disk IO on non-block-boundaries).

It would be much nicer if we only did it for sockets and pipes, which 
don't have the same block-boundary issues anyway (there's still the system 
call cost, but on a pipe/socket, the real costs tend to be elsewhere).

Again, this is something that a non-stdio-based buffering library would 
easily handle. You could just test the file descriptor _once_ at the 
beginning, to see if it's a regular file or not. And then you could have 
the error handling where it belongs (when the IO is actually done, and the 
error actually happens) rather than in the callers using a bad interface 
that sometimes loses 'errno'.

Btw, to balance the above performance comment: doing the flush_or_die() 
obviously *does* mean that you get better performance in the odd cases. 
For example, if you do

	[torvalds@woody linux]$ trap '' SIGPIPE
	[torvalds@woody linux]$ time git log | head

I get get 0.002s, while it used to be:

	real    0m1.382s
	user    0m1.340s
	sys     0m0.028s

just because it did the whole thing regardless of any EPIPE errors.

Of course, that case probably isn't very usual, but I could imagine that 
some users of "git blame -C --incremental" could actually cause situations 
like this (ie just stop listening when they got the part they're 
interested in, and maybe they'd have some strange reason to ignore 
SIGPIPE).

So I'm not opposed to the patch I sent out, I just wanted to point out 
that this is an area we *could* improve upon if we didn't do that stdio 
thing.

		Linus