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

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

[Jim Meyering]

Currently, when git-diff writes to a full device or gets an I/O error,
it fails to detect the write error:

    $ git-diff |wc -c
    3984
    $ git-diff > /dev/full && echo ignored write failure
    ignored write failure

git-log does the same thing:

    $ git-log -n1 > /dev/full && echo ignored write failure
    ignored write failure

Each git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.

    $ ./git-log -n1 > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]
    $ ./git-diff > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

You can demonstrate this with git's own --version output, too:
(but git --help detects the failure without this patch)

    $ ./git --version > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

Note that the fcntl test (for whether the fileno may be closed) is
required in order to avoid EBADF upon closing an already-closed stdout,
as would happen for each git command that already closes stdout; I think
update-index was the one I noticed in the failure of t5400, before I
added that test.

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 git.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 29b55a1..a7d6515 100644
--- a/git.c
+++ b/git.c
@@ -308,6 +308,7 @@ 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;
+		int status;
 		if (strcmp(p->cmd, cmd))
 			continue;

@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 			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));
+		status = p->fn(argc, argv, prefix);
+
+		/* Close stdout if necessary, and diagnose any failure.  */
+		if (0 <= fcntl(fileno (stdout), F_GETFD)
+		    && (ferror(stdout) || fclose(stdout)))
+			die("write failure on standard output: %s",
+			    strerror(errno));
+
+		exit(status);
 	}
 }

--
1.5.2.73.g18bece

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

[Linus Torvalds]

On Sat, 26 May 2007, Jim Meyering wrote:
>
> Each git command should report such a failure.
> Some already do, but with the patch below, they all do, and we
> won't have to rely on code in each command's implementation to
> perform the right incantation.

The patch is wrong.

Some write errors are expected and GOOD.

For example, EPIPE should not be reported. It's normal. The user got 
bored. It might be hidden by the SIGPIPE killing us, but regardless, 
reporting it for the normal log/diff thing is just not correct. EPIPE 
isn't an error, it's a "ok, nobody is listening any more".

Also, PLEASE don't do this:

> +		if (0 <= fcntl(fileno (stdout), F_GETFD)

That's totally unreadable to any normal human.

You don't say "if zero is smaller or equal to X". You say "if X is larger 
than or equal to zero". Stop messing with peoples minds, dammit!

Anybody who thinks that code like this causes fewer errors is just fooling 
himself. It causes *more* bugs, because people have a harder time reading 
it.

Maybe you and Junio have taught yourself bad manners, but you're a tiny 
tiny part of humanity or the development community. Junio can do it just 
because while he's just a single person, he's a big part of the git coding 
base, but anybody else who does it should just be shot.

			Linus

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Also, PLEASE don't do this:
>
>> +		if (0 <= fcntl(fileno (stdout), F_GETFD)
>
> That's totally unreadable to any normal human.
>
> You don't say "if zero is smaller or equal to X". You say "if X is larger 
> than or equal to zero". Stop messing with peoples minds, dammit!
>
> Anybody who thinks that code like this causes fewer errors is just fooling 
> himself. It causes *more* bugs, because people have a harder time reading 
> it.
>
> Maybe you and Junio have taught yourself bad manners, but you're a tiny 
> tiny part of humanity or the development community. Junio can do it just 
> because while he's just a single person, he's a big part of the git coding 
> base, but anybody else who does it should just be shot.

Whew, that is a blast from the past.

cf. http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3906

 (1) Maybe Jim was just being nice, trying to make the code look
     like surrounding code;

 (2) Maybe Jim and the person I learned the style from worked
     together for a long time and they picked it up from the
     same source;

 (3) Maybe I am not alone, and it is not native language -
     mother tongue issue as some suspected in the quoted thread.

In any case, I think my recent code have much less "textual
order should reflect actual order" convention than before,
because I have been forcing myself to say aloud "if X is larger"
or "if X is smaller" before writing my comparisons, in order to
match the "peoples minds" expectation you mentioned above.

This initially slowed me down and made my head hurt quite a bit,
and sometimes it still does.

Once you learn to _visualize_ the ordering relationship in "X op
Y" by relying on "op" being always < or <=, you will get the
"number line" pop in your head whenever you see a comparision
expression, without even having to think about it, and you "see"
X and Y on the number line:

        ... -2        -1         0         1         2  ...  
    ---------+---------+---------+---------+---------+---------
    true:                        0   <=  fcntl(...)


        ... -2        -1         0         1         2  ...  
    ---------+---------+---------+---------+---------+---------
    false:    (0 <= fcntl(...))

What the comparison is doing comes naturally to you, without
even having to translate it back to human language "X is larger
(or smaller) than this constant".  The ordering is right there,
in front of your eyes, before you vocalize it.

In a sense, just like it is hard to go back from git to CVS (or
it is hard to go back to not knowing the power of the index), it
is very hard to go back once you learn to do this.

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

[Nicolas Pitre]

On Sat, 26 May 2007, Junio C Hamano wrote:

> Once you learn to _visualize_ the ordering relationship in "X op
> Y" by relying on "op" being always < or <=, you will get the
> "number line" pop in your head whenever you see a comparision
> expression, without even having to think about it, and you "see"
> X and Y on the number line:
> 
>         ... -2        -1         0         1         2  ...  
>     ---------+---------+---------+---------+---------+---------
>     true:                        0   <=  fcntl(...)
> 
> 
>         ... -2        -1         0         1         2  ...  
>     ---------+---------+---------+---------+---------+---------
>     false:    (0 <= fcntl(...))
> 
> What the comparison is doing comes naturally to you, without
> even having to translate it back to human language "X is larger
> (or smaller) than this constant".  The ordering is right there,
> in front of your eyes, before you vocalize it.

Well... it probably depends on how your brain is wired up.

I completely agree with your reasoning.  It _should_ indeed be natural 
and more obvious to always put things in increasing order.

BUT it is not how my brain is connected, and after many attempts I just 
cannot work efficiently with your method.  It simply doesn't come out 
logical for me and I have to spend an unusual amount of time on every 
occasion I encounter this structure to really get it.  To me it always 
looks backward.

And I suspect the majority of people who just cannot train their brain 
with the arguably superior representation are many, probably the 
majority. It appears to be the case for Linus.  It is definitely the 
case for me.


Nicolas, who apologizes for his defective brain.

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

[Jim Meyering]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sat, 26 May 2007, Jim Meyering wrote:
>>
>> Each git command should report such a failure.
>> Some already do, but with the patch below, they all do, and we
>> won't have to rely on code in each command's implementation to
>> perform the right incantation.
>
> The patch is wrong.

What you should have said is that the patch is fine in principle, since
it does fix a pretty serious bug (important tools ignoring ENOSPC),
but you'd prefer that it continue to ignore EPIPE.

With a name like yours, being more positive would go a long way toward
encouraging (or rather *not discouraging*) contributions.

> Some write errors are expected and GOOD.
>
> For example, EPIPE should not be reported. It's normal. The user got
> bored. It might be hidden by the SIGPIPE killing us, but regardless,
> reporting it for the normal log/diff thing is just not correct. EPIPE
> isn't an error, it's a "ok, nobody is listening any more".

I have to disagree.  There may be precedent for hiding EPIPE errors,
but that is not the norm among command line tools wrt piped stdout.

First of all, one has to work just to get such an error.  These days,
most people use a shell that doesn't ignore, handle or block SIGPIPE, so
direct use of a program like git-log or git-diff gets the signal directly,
and there's no EPIPE error.  E.g., try "cat", and you see it gets SIGPIPE
(141=128+SIGPIPE(13)):

    $ seq 90000|(cat; echo $? >&2) | head -1 > /dev/null
    141

However, you can tweak your shell to handle/ignore SIGPIPE.  Also,
some porcelain scripts do ignore SIGPIPE.  Then, a program run in that
environment does see EPIPE.  Consider how a few other non-interactive
programs work when one of their stdout-writing syscalls fails with EPIPE:

Try GNU diff and sed:
[now, using shorter ":" in place of more realistic head -1]

    $ (trap '' PIPE; seq 90000|diff - /dev/null;echo $? >&2)| :
    diff: standard output: Broken pipe
    2
    $ (trap '' PIPE; seq 90000|sed s/a/b/; echo $? 1>&2)| :
    /bin/sed: couldn't write 5 items to stdout: Broken pipe
    seq: write error: Broken pipe
    4

Try tee (from GNU coreutils):

    $ (trap '' PIPE; seq 90000|tee /dev/null; echo $? >&2) | :
    tee: standard output: Broken pipe
    tee: write error
    1

sort, tac, cut, fold, od, head, tail, tr, uniq, etc. all work the same
way, if you're using the coreutils.  But perhaps that's not fair, since
I maintain the coreutils.  And there is some variance among how other-
vendor versions of those tools work.  E.g., Solaris 10's /bin/cat
diagnoses the error, but neither /bin/sort nor /bin/diff do.

As for version control tools, monotone does what I'd expect in
this situation: "mtn diff" reports the failure and exits nonzero.

svn and cvs also report the error, although they both exit successfully
in spite of that.  I tried both log and diff commands for each tool.
cvs catches the signal, svn doesn't.

mercurial and darcs totally ignore the write error and SIGPIPE,
so there is no way to determine from stderr or exit code whether
their writes complete normally.

For any tool whose output might be piped to another, the pipe-writing
tool should exit nonzero for any write error.  Otherwise, its exit code
ends up being a lie, pretending success but, in effect, covering up for
a failure.  In general, I've found that papering over syscall failures
makes higher-level problems harder to diagnose.

Do you really want git-log to continue to do this?

    $ (trap '' PIPE; git-log; echo $? >&2 ) | :
    0

With my patch, it does this:

    $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :
    fatal: write failure on standard output: Broken pipe
    128

> Also, PLEASE don't do this:
>
>> +		if (0 <= fcntl(fileno (stdout), F_GETFD)

Since Junio is making an effort to "conform",
I too will make the effort when contributing to git.

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

[Linus Torvalds]

On Sun, 27 May 2007, Jim Meyering wrote:
> 
> I have to disagree.  There may be precedent for hiding EPIPE errors,
> but that is not the norm among command line tools wrt piped stdout.

.. and this is a PROBLEM. Which is why I think your patch was really 
wrong.

I don't know how many people remember all the _stupid_ problems we had 
exactly because many versions of bash are crap, crap, crap, and people 
(including you) don't realize that EPIPE is _different_ from other write 
errors.

Just do a google search for

	"broken pipe" bash

and not only will you see a lot of complaints, but the #5 entry is a 
complaint for a git issue that we had tons of problems with. See for 
example

	http://www.gelato.unsw.edu.au/archives/git/0504/2602.html

The reason? Some _idiotic_ versions of bash don't have DONT_REPORT_SIGPIPE 
on by default. 

So I do get upset when people then make the same error with git.

> Do you really want git-log to continue to do this?
> 
>     $ (trap '' PIPE; git-log; echo $? >&2 ) | :
>     0
> 
> With my patch, it does this:
> 
>     $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :
>     fatal: write failure on standard output: Broken pipe
>     128

That error return is fine. The annoying error report, however, is NOT.

For _exactly_ the same reason that a bash that doesn't have 
DONT_REPORT_SIGPIPE enabled is a piece of crap.

And your arguments that "others do it wrong, so we can too" is so broken 
as to be really really sad. If you cannot see the serious problem with 
that argument, I don't know what to tell you.

Try this:

	trap '' PIPE; ./git-log | head

and dammit, if you get an error message from that, your program is BROKEN.

And if you cannot understand that, then I don't even know what to say.

But _exiting_ is fine. It's the bogus error reporting that isn't. The 
above command like should NOT cause the user to have to skip stderr - 
because no error happened!

(Whether the error code is 0 or some error, I dunno. I'd argue that if you 
ignore SIGPIPE, you'd probably also want to do "exit(0)" for EPIPE, but 
it's not nearly as annoying as writing bogus error messages to stderr.

			Linus

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

[Jim Meyering]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sun, 27 May 2007, Jim Meyering wrote:
>>
>> I have to disagree.  There may be precedent for hiding EPIPE errors,
>> but that is not the norm among command line tools wrt piped stdout.
>
> .. and this is a PROBLEM. Which is why I think your patch was really
> wrong.
>
> I don't know how many people remember all the _stupid_ problems we had
> exactly because many versions of bash are crap, crap, crap, and people
> (including you) don't realize that EPIPE is _different_ from other write
> errors.
>
> Just do a google search for
>
> 	"broken pipe" bash
>
> and not only will you see a lot of complaints, but the #5 entry is a
> complaint for a git issue that we had tons of problems with. See for
> example
>
> 	http://www.gelato.unsw.edu.au/archives/git/0504/2602.html

Whether bash should print a diagnostic when it kills a process with
SIGPIPE is _different_ from whether the writing process should diagnose
its own write failure arising from a handled SIGPIPE.

I suspect that git's special treatment of EPIPE was a shoot-the-messenger
reaction to the work-around (trap '' PIPE) required to avoid diagnostics
from porcelain being interpreted by what would now be a 2-year-old
version of bash.  It is time to remove that work-around, because it
can obscure real errors, and removing it will be largely unnoticed.

...
>> Do you really want git-log to continue to do this?
>>
>>     $ (trap '' PIPE; git-log; echo $? >&2 ) | :
>>     0
>>
>> With my patch, it does this:
>>
>>     $ (trap '' PIPE; ./git-log; echo $? >&2 ) | :
>>     fatal: write failure on standard output: Broken pipe
>>     128
>
> That error return is fine.  The annoying error report, however, is NOT.

That error message serves to indicate a REAL FLAW, some of the time.
In such cases, the diagnostic is likely to be welcome, not annoying.
Which is more important: avoiding annoyance in some now-very-unusual
circumstances, or allowing robust porcelain to diagnose real errors?

> And your arguments that "others do it wrong, so we can too" is so broken
> as to be really really sad. If you cannot see the serious problem with
> that argument, I don't know what to tell you.

Questionable debate tactic: misrepresent an opponent's argument with a
statement that is obviously foolish, then proceed to argue that anyone
who doesn't recognise the silliness of the restated argument leaves
you speechless.

My argument is "If so many other tools do it RIGHT,
why can't git do it right, too?".

> Try this:
>
> 	trap '' PIPE; ./git-log | head
>
> and dammit, if you get an error message from that, your program is BROKEN.
>
> And if you cannot understand that, then I don't even know what to say.

Of course error messages are annoying when your short-pipe-read is
_deliberate_ (tho, most real uses of git tools will actually get no
message to be annoyed about[*]), but what if there really *is* a mistake?
Try this:

    # You want to force git to ignore the error.
    $ trap '' PIPE; git-rev-list HEAD | sync
    $

    # I want to diagnose the error (the exit 128 is from zsh, bash gets 0):
    $ trap '' PIPE; git-rev-list HEAD | sync
    fatal: write failure on standard output: Broken pipe
    [Exit 128]

The use of "sync" above is obviously a mistake.
With the existing git tools, most write-to-stdout errors are ignored,
including EPIPE, so this erroneous command completes successfully, just as
it would when writing to a full or corrupt disk.  Even if you use bash's
"set -o pipefail" option, there is no indication of the failure.  With my
patch, write errors are reported, even EPIPE, so that in this case, the
user of the above will get an error message.  And with bash's pipefail
option, the git-rev-list write failure can propagate "out" to the script.

Since using "sync" is contrived, for a little more realism, imagine the
SHA1 strings are being written to a FIFO, and you don't have access to
the program running on the other side.  The sender should have a way to
detect when the other end closes the pipe prematurely.  Exempting EPIPE,
it CANNOT detect failure:

    $ mkfifo F && head -1 F > /dev/null & sleep 1
    $ trap '' PIPE; git-rev-list HEAD > F && echo bad: ignored write failure
    bad: ignored write failure

Handling EPIPE like other write errors, it CAN detect failure:

    $ mkfifo F && head -1 F > /dev/null & sleep 1
    $ trap '' PIPE; ./git-rev-list HEAD > F || echo ok
    fatal: write failure on standard output: Broken pipe
    ok

> But _exiting_ is fine. It's the bogus error reporting that isn't. The
> above command like should NOT cause the user to have to skip stderr -
> because no error happened!

Don't worry about the diagnostic.  It probably won't show up much at all
[*] these days, since most shells now let SIGPIPE kill the writer (silently).
And if the message does appear once in a while, there are cleaner ways
to suppress it than hamstringing all of the git plumbing for everyone.

In fact, in this vein, the existing EPIPE exceptions in write_or_die.c
and merge-recursive.c should be removed.  Here's a revised patch to do
that, too.  Junio, if you see fit to accept any part of this, I'll be
happy to write test case additions demonstrating before/after improvements.

========================================================================
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:

    $ git-diff |wc -c
    3984
    $ git-diff > /dev/full && echo ignored write failure
    ignored write failure

git-log does the same thing:

    $ git-log -n1 > /dev/full && echo ignored write failure
    ignored write failure

Each and every git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.

    $ ./git-log -n1 > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]
    $ ./git-diff > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

You can demonstrate this with git's own --version output, too:
(but git --help detects the failure without this patch)

    $ ./git --version > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

Note that the fcntl test (for whether the fileno may be closed) is
required in order to avoid EBADF upon closing an already-closed stdout,
as would happen for each git command that already closes stdout; I think
update-index was the one I noticed in the failure of t5400, before I
added that test.

Also, to be consistent, don't ignore EPIPE write failures.

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 git.c             |   11 ++++++++++-
 merge-recursive.c |    3 ---
 write_or_die.c    |    4 ----
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/git.c b/git.c
index 29b55a1..e176ab4 100644
--- a/git.c
+++ b/git.c
@@ -308,6 +308,7 @@ 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;
+		int status;
 		if (strcmp(p->cmd, cmd))
 			continue;

@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 			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));
+		status = p->fn(argc, argv, prefix);
+
+		/* Close stdout if necessary, and diagnose any failure.  */
+		if (fcntl(fileno (stdout), F_GETFD) >= 0)
+		    && (ferror(stdout) || fclose(stdout)))
+			die("write failure on standard output: %s",
+			    strerror(errno));
+
+		exit(status);
 	}
 }

diff --git a/merge-recursive.c b/merge-recursive.c
index 8f72b2c..bfa4335 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -523,9 +523,6 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
 	while (size > 0) {
 		long ret = write_in_full(fd, buf, size);
 		if (ret < 0) {
-			/* Ignore epipe */
-			if (errno == EPIPE)
-				break;
 			die("merge-recursive: %s", strerror(errno));
 		} else if (!ret) {
 			die("merge-recursive: disk full?");
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,6 @@ 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));
 	}
 }
@@ -50,8 +48,6 @@ void write_or_die(int fd, const void *buf, size_t count)
 int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
 {
 	if (write_in_full(fd, buf, count) < 0) {
-		if (errno == EPIPE)
-			exit(0);
 		fprintf(stderr, "%s: write error (%s)\n",
 			msg, strerror(errno));
 		return 0;

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

[Marco Roeland]

On monday May 28th 2007 at 15:14 Jim Meyering wrote:

> > I don't know how many people remember all the _stupid_ problems we had
> > exactly because many versions of bash are crap, crap, crap, and people
> > (including you) don't realize that EPIPE is _different_ from other write
> > errors.
> >
> > Just do a google search for
> >
> > 	"broken pipe" bash
> >
> > and not only will you see a lot of complaints, but the #5 entry is a
> > complaint for a git issue that we had tons of problems with. See for
> > example
> >
> > 	http://www.gelato.unsw.edu.au/archives/git/0504/2602.html
> 
> Whether bash should print a diagnostic when it kills a process with
> SIGPIPE is _different_ from whether the writing process should diagnose
> its own write failure arising from a handled SIGPIPE.
> 
> I suspect that git's special treatment of EPIPE was a shoot-the-messenger
> reaction to the work-around (trap '' PIPE) required to avoid diagnostics
> from porcelain being interpreted by what would now be a 2-year-old
> version of bash.  It is time to remove that work-around, because it
> can obscure real errors, and removing it will be largely unnoticed.

Good point. But also notice that when you are stuck with a shell that
does complain about SIGPIPE it is _really_ annoying!

On current Debian 'sid' with your patch this does not seem to be the
case.

The second chunk of your patch (to git.c) contains a small copy-paste
accident methinks:

> @@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
>  			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));
> +		status = p->fn(argc, argv, prefix);
> +
> +		/* Close stdout if necessary, and diagnose any failure.  */
> +		if (fcntl(fileno (stdout), F_GETFD) >= 0)
> +		    && (ferror(stdout) || fclose(stdout)))
> +			die("write failure on standard output: %s",
> +			    strerror(errno));
> +
> +		exit(status);

The if statement with 'fcntl' is missing a brace, it should be:

+		if ((fcntl(fileno (stdout), F_GETFD) >= 0)
+		    && (ferror(stdout) || fclose(stdout)))

-- 
Marco Roeland

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

[Linus Torvalds]

On Mon, 28 May 2007, Jim Meyering wrote:
> 
> I suspect that git's special treatment of EPIPE was a shoot-the-messenger
> reaction to the work-around (trap '' PIPE) required to avoid diagnostics
> from porcelain being interpreted by what would now be a 2-year-old
> version of bash.

No. You don't seem to realize. That was the *default* behaviour of bash.

For all I know, it might _still_ be the default behaviour.

The only reason not everybody ever even noticed, was that most 
distributions were clueful enough to have figured out that it was broken, 
and configured bash separately. But "most" does not mean "all", and I had 
this problem on powerpc, and others had it on Debian, I htink  (might have 
been slackware). I think RH and SuSE had both fixed it explicitly.

> 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,6 @@ 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));

Nack. Nack. NACK.

I think this patch is fundamentally WRONG. This fragment is just a prime 
example of why the whole patch is crap. The old code was correct, and you 
broke it.

		Linus

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

[Jim Meyering]

Marco Roeland <marco.roeland@xs4all.nl> wrote:
> The if statement with 'fcntl' is missing a brace, it should be:
>
> +		if ((fcntl(fileno (stdout), F_GETFD) >= 0)
> +		    && (ferror(stdout) || fclose(stdout)))

Thank you.
That was a stale patch (from before I compiled/tested, obviously).
I intended this:

		if (fcntl(fileno (stdout), F_GETFD) >= 0
		    && (ferror(stdout) || fclose(stdout)))

Here's the correct one:

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:

    $ git-diff |wc -c
    3984
    $ git-diff > /dev/full && echo ignored write failure
    ignored write failure

git-log does the same thing:

    $ git-log -n1 > /dev/full && echo ignored write failure
    ignored write failure

Each and every git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.

    $ ./git-log -n1 > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]
    $ ./git-diff > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

You can demonstrate this with git's own --version output, too:
(but git --help detects the failure without this patch)

    $ ./git --version > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

Note that the fcntl test (for whether the fileno may be closed) is
required in order to avoid EBADF upon closing an already-closed stdout,
as would happen for each git command that already closes stdout; I think
update-index was the one I noticed in the failure of t5400, before I
added that test.

Also, to be consistent, don't ignore EPIPE write failures.

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 git.c             |   11 ++++++++++-
 merge-recursive.c |    3 ---
 write_or_die.c    |    4 ----
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/git.c b/git.c
index 29b55a1..7b45a73 100644
--- a/git.c
+++ b/git.c
@@ -308,6 +308,7 @@ 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;
+		int status;
 		if (strcmp(p->cmd, cmd))
 			continue;

@@ -321,7 +322,15 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 			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));
+		status = p->fn(argc, argv, prefix);
+
+		/* Close stdout if necessary, and diagnose any failure.  */
+		if (fcntl(fileno (stdout), F_GETFD) >= 0
+		    && (ferror(stdout) || fclose(stdout)))
+			die("write failure on standard output: %s",
+			    strerror(errno));
+
+		exit(status);
 	}
 }

diff --git a/merge-recursive.c b/merge-recursive.c
index 8f72b2c..bfa4335 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -523,9 +523,6 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
 	while (size > 0) {
 		long ret = write_in_full(fd, buf, size);
 		if (ret < 0) {
-			/* Ignore epipe */
-			if (errno == EPIPE)
-				break;
 			die("merge-recursive: %s", strerror(errno));
 		} else if (!ret) {
 			die("merge-recursive: disk full?");
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,6 @@ 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));
 	}
 }
@@ -50,8 +48,6 @@ void write_or_die(int fd, const void *buf, size_t count)
 int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
 {
 	if (write_in_full(fd, buf, count) < 0) {
-		if (errno == EPIPE)
-			exit(0);
 		fprintf(stderr, "%s: write error (%s)\n",
 			msg, strerror(errno));
 		return 0;

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

[Marco Roeland]

On monday May 28th 2007 at 20:19 Jim Meyering wrote:

> ...
> 
> Also, to be consistent, don't ignore EPIPE write failures.

In practice I agree with someone else on this thread that EPIPE _is_
different. In a way the responsibility doesn't lie with the writer but
with the reader.

But just out of curiosity is there an easy way to test the EPIPE
behaviour? I cite a piece of the "changelog.Debian" file from the
Debian version of the bash shell. In Debian, as earlier in many other
distributions, the annoying EPIPE error was "fixed" in version 2.0.3-3
from 19 dec 1999.

========================================================================
* Define DONT_REPORT_SIGPIPE: We don't want to see `Broken pipe' messages
  when a job like `cat jobs.c | exit 1' is executed. Fixes part of #7047,
  #10259, #10433 and #10494. Comment from the upstream author: "The default
  bash behavior with respect to the exit status of a pipeline will not
  change.  Changing it as suggested in the discussion of #10494 would render
  bash incompatible with every other shell out there.". Closed these reports.

  -- Matthias Klose <doko@debian.org>  Sun, 19 Dec 1999 15:58:43 +0100
========================================================================

The mentioned "test-case" as used in "git log -n1 | exit 1" doesn't
produce an error in my Debian 'sid' bash, either with or without your
patch, so it doesn't seem to have any effect there? Whereas probably in
a "default" bash (don't know if upstream has changed it's mind already!)
with your patch (i.e. the EPIPE special casing removal) it will again
probably introduce these annoying (for interactive use) errors.

Thanks for your patch anyway, the "fcntl" diagnosis is a really useful
technique to know, and IMVHO also useful for git; although perhaps not
very portable for all platforms.
-- 
Marco Roeland

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

[Jim Meyering]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Mon, 28 May 2007, Jim Meyering wrote:
>>
>> I suspect that git's special treatment of EPIPE was a shoot-the-messenger
>> reaction to the work-around (trap '' PIPE) required to avoid diagnostics
>> from porcelain being interpreted by what would now be a 2-year-old
>> version of bash.

Hi Linus!

Thanks for all the encouragement.

> No. You don't seem to realize. That was the *default* behaviour of bash.

Of course it was the default.  Because it was the default, it
provoked contortions like using `trap '' PIPE' in shell scripts,
which in turn provoked EPIPE diagnostics from git, which
prompted the EPIPE-ignoring changes in git plumbing.
And those changes can now OBSCURE REAL ERRORS, as I've shown.

Note: it was the SIGPIPE-ignoring work-around that caused the trouble.
The bash bug didn't cause trouble with git _directly_.

If anyone can find a mainstream distro (I didn't) on which my
patch causes trouble, please let us all know.

Bash changed its default first in bash-3.1-alpha1.
The next stable release was bash-3.1, in Dec 2005:

  r.  By default, the shell no longer reports processes dying from SIGPIPE.

It looks like most major distros had fixed it long before.
The latest stable release is bash-3.2 from October, 2006.

> For all I know, it might _still_ be the default behaviour.

It's not.  See above.
Easy to test: run this: seq 99999|head -1
if all you see is a single line with "1" on it, and an exit status of 0,
then there's no problem.

However, the version of bash you use is IRRELEVANT to the question of
EPIPE.  SIGPIPE has always been delivered by default.  The only difference
lay in whether bash _reported_ the delivery.  It's only the work-around
ignoring of SIGPIPE that used to provoke EPIPE "broken pipe" errors.
Now, all of the git porcelain shell code that did that appears to be gone,
probably converted to perl or C.

You can get an EPIPE diagnostic with my patch any time the affected
plumbing is invoked from an environment in which SIGPIPE is ignored.
That environment could be your shell (if you put "trap '' PIPE" in a
start-up file -- though no one does *that*), or porcelain that does that,
or the perlish $SIG{PIPE} = 'IGNORE'.  The following are the only parts
of git I've found that ignore SIGPIPE:

  git-archimport.perl
  git-cvsimport.perl
  git-svn.perl
  git-svnimport.perl

And nothing in cogito does.
So, now, I *really* don't see why there's any fuss about EPIPE.

> The only reason not everybody ever even noticed, was that most
> distributions were clueful enough to have figured out that it was broken,
> and configured bash separately. But "most" does not mean "all", and I had
> this problem on powerpc, and others had it on Debian, I htink  (might have
> been slackware). I think RH and SuSE had both fixed it explicitly.

Precisely.  That behavior in bash was so annoying that people were
motivated to fix it quickly.  But all of that was resolved long ago.

...
> Nack. Nack. NACK.
>
> I think this patch is fundamentally WRONG. This fragment is just a prime
> example of why the whole patch is crap. The old code was correct, and you
> broke it.

Um... maybe you've forgotten that this patch fixes a hole in the
"old code" (git.c).  Many git tools ignore write (ENOSPC) failures.
Compared to that aspect of the fix, I would have thought EPIPE-
handling would be a minor detail.  But now, the whole patch has
become "crap"?

Consider the EPIPE-related risks/choice:

  1) Continue to ignore EPIPE write failure: can obscure real errors.
       BTW, Linus, don't you agree?  You never commented on this point.

  2) Remove the EPIPE exclusion: *might* make git give a "broken pipe"
       diagnostic, if you run git in a SIGPIPE-ignoring environment.

#2 seems to be the lesser of two evils, considering that we can fix or
work around the occasional "broken pipe" error, but we can't work around
an unconditionally-ignored EPIPE.

Jim

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

[Jim Meyering]

Marco Roeland <marco.roeland@xs4all.nl> wrote:
> On monday May 28th 2007 at 20:19 Jim Meyering wrote:
>> Also, to be consistent, don't ignore EPIPE write failures.
>
> In practice I agree with someone else on this thread that EPIPE _is_
> different. In a way the responsibility doesn't lie with the writer but
> with the reader.

Do you think it's ok for git-rev-list _not_ to diagnose an erroneous
command like this (i.e., to exit(0)):

    git-rev-list HEAD | sync

where "sync" could be any command that exits successfully
without reading any input?

Is it ok that it is currently *impossible* to diagnose that
failure by looking at exit codes?

> But just out of curiosity is there an easy way to test the EPIPE
> behaviour? I cite a piece of the "changelog.Debian" file from the

There are some examples here:
http://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48617

...
> The mentioned "test-case" as used in "git log -n1 | exit 1" doesn't
> produce an error in my Debian 'sid' bash, either with or without your
> patch, so it doesn't seem to have any effect there? Whereas probably in
> a "default" bash (don't know if upstream has changed it's mind already!)
> with your patch (i.e. the EPIPE special casing removal) it will again
> probably introduce these annoying (for interactive use) errors.

As I just said in reply to Linus, the EPIPE handling difference
is independent of what version of bash you use.

> Thanks for your patch anyway, the "fcntl" diagnosis is a really useful
> technique to know, and IMVHO also useful for git; although perhaps not
> very portable for all platforms.

It appears to be portable enough.  fcntl/F_GETFD support is required
by POSIX, and has been around for ages.  FWIW, it's also used in git's
daemon.c and sha1_file.c.

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

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
> ...
> You can demonstrate this with git's own --version output, too:
> (but git --help detects the failure without this patch)
>
>     $ ./git --version > /dev/full
>     fatal: write failure on standard output: No space left on device
>     [Exit 128]
>
> Note that the fcntl test (for whether the fileno may be closed) is
> required in order to avoid EBADF upon closing an already-closed stdout,
> as would happen for each git command that already closes stdout; I think
> update-index was the one I noticed in the failure of t5400, before I
> added that test.
>
> Also, to be consistent, don't ignore EPIPE write failures.
>
> Signed-off-by: Jim Meyering <jim@meyering.net>

I do not think anybody has much objection about the change to
handle_internal_command() in git.c you made.  Earlier we relied
on exit(3) to close still open filehandles (while ignoring
errors), and you made the close explicit in order to detect
errors.

But "to be consistent" above is not a very good justification.

In the presense of shells that give "annoying" behaviour (we
have to remember that not everybody have enough privilege,
expertise, or motivation to update /bin/sh or install their own
copy in $HOME/bin/sh), "EPIPE is different from other errors" is
a more practical point of view, I'd have to say.  IOW, it is not
clear if it is a good thing "to be consistent" to begin with.

I would have appreciated if this were two separate patches.  I
think the EPIPE one is an independent issue.  We could even make
it a configuration option to ignore EPIPE ;-)

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

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> Of course error messages are annoying when your short-pipe-read is
> _deliberate_ (tho, most real uses of git tools will actually get no
> message to be annoyed about[*]), but what if there really *is* a mistake?
> Try this:
>
>     # You want to force git to ignore the error.
>     $ trap '' PIPE; git-rev-list HEAD | sync
>     $

It is perfectly valid (although it is stupid) for a Porcelain
script to do this:

    latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)
    case "$latest_by_jim" in
    '') echo "No commit by Jim" ;;
    *)  # do something interesting on the commit
        ;;;
    esac

In such a case, it is a bit too much for my taste to force the
script to redirect what comes out of fd 2 of the upstream of the
pipe, so that it can filter out only the "write error" message
but still show other kinds of error messages.  You could do so
by elaborate shell magic, perhaps like this:

        filter_pipe_error () {
                exec 3>&1
                (eval "$1" 2>&1 1>&3 | grep >&2 -v 'Broken pipe')
        }

	latest_by_jim=$(filter_pipe_error \
        	'git log --pretty=oneline --author='\''Jim'\'' | head -n 1'
	)

but what's the point?

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);
 	}
 }

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

[Petr Baudis]

  (I think that funnily enough, Linus is to a degree to the Git
community something like Al Viro and Chris Hellwig are to the Linux
kernel community. Don't get too derailed by his blunt^Whonest criticism,
which is however usually quite valid. ;-)

On Mon, May 28, 2007 at 10:23:20PM CEST, Jim Meyering wrote:
> Marco Roeland <marco.roeland@xs4all.nl> wrote:
> > On monday May 28th 2007 at 20:19 Jim Meyering wrote:
> >> Also, to be consistent, don't ignore EPIPE write failures.
> >
> > In practice I agree with someone else on this thread that EPIPE _is_
> > different. In a way the responsibility doesn't lie with the writer but
> > with the reader.
> 
> Do you think it's ok for git-rev-list _not_ to diagnose an erroneous
> command like this (i.e., to exit(0)):
> 
>     git-rev-list HEAD | sync
> 
> where "sync" could be any command that exits successfully
> without reading any input?
> 
> Is it ok that it is currently *impossible* to diagnose that
> failure by looking at exit codes?

  Actually, yes!

  Because there's no "failure" per se. The command we piped the output
into just decided that he isn't actually interested in any (for whatever
reason; it might decide dynamically based on some parameters etc.). I
can't think of why it could be considered a failure for git-rev-list if
its customer doesn't happily eat all the output it generates. It's the
customer's job to report any real trouble that happenned and might be
cause of the premature end (or maybe the premature end was totally
valid).

  Maybe it could expose some (IMHO contrived) error scenarios, but in
most cases I think it will end up just spitting out bogus error
messages. And what will people do? They won't bother to filter out this
particular one (which isn't even that easy if the strerror() is
localized, furthermore). They will just 2>/dev/null it. And cause the
*real* error messages go to the land of void as well. There's enough of
impossible-to-diagnose-error-conditions-because-stderr-goes-to-null
scripts in the land of UNIX already and this patch, while actually
well-meant to do the opposite, might well actually increase their number
because of this.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
		-- Samuel Beckett

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

[Linus Torvalds]

On Mon, 28 May 2007, Jim Meyering wrote:
> 
> Um... maybe you've forgotten that this patch fixes a hole in the
> "old code" (git.c).  Many git tools ignore write (ENOSPC) failures.

Maybe you have not noticed, but my argument has ben about EPIPE.

> Compared to that aspect of the fix, I would have thought EPIPE-
> handling would be a minor detail.  But now, the whole patch has
> become "crap"?

About half the patch was _removing_ EPIPE stuff - not at all about the 
ENOSPC stuff you claim.

And the ENOSPC code could have added the same *correct* code that does the 
right thing for EPIPE.

>   1) Continue to ignore EPIPE write failure: can obscure real errors.
>        BTW, Linus, don't you agree?  You never commented on this point.

THAT'S THE ONLY THING I'VE BEEN COMMENTING ON!

They aren't "obscure real errors". EPIPE is neither obscure _nor_ an 
error.

The code-paths where you removed EPIPE handlign have two cases:
 - SIGPIPE happens: you made no change
 - SIGPIPE diesn't happen: you broke the code.

So remind me again, why the hell do you think your patch is so great and 
so important, considering that it broke real code, and made things worse?

And why don't you just admit that EPIPE is special, isn't an error, and 
shouldn't be complained about? If you get EPIPE on the write, it means 
"the other end didn't care". It does NOT mean "I should now do a really 
annoying message".

It's that simple. You seem to admit that SIGPIPE handling in bash should 
have been fixed, and that it was annoying to complain about it there. Why 
can't you just admit that it's annoyign and wrong to complain about the 
same thing when it's EPIPE?

			Linus

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

[Linus Torvalds]

On Mon, 28 May 2007, Junio C Hamano wrote:
> 
> 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

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

[Jim Meyering]

Junio C Hamano <junkio@cox.net> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.
>> ...
...
> I do not think anybody has much objection about the change to
> handle_internal_command() in git.c you made.  Earlier we relied
> on exit(3) to close still open filehandles (while ignoring
> errors), and you made the close explicit in order to detect
> errors.

Hi Junio,

> But "to be consistent" above is not a very good justification.

I know that well, now, especially after all of my fruitless
justification on this list.

> In the presense of shells that give "annoying" behaviour (we
> have to remember that not everybody have enough privilege,
> expertise, or motivation to update /bin/sh or install their own
> copy in $HOME/bin/sh), "EPIPE is different from other errors" is
> a more practical point of view, I'd have to say.  IOW, it is not
> clear if it is a good thing "to be consistent" to begin with.

In case you haven't seen it in the rest of this thread, the version of
bash you use does not change git's EPIPE-handling behavior.  Using stock
bash-3.0 or bash-2.05b, you will get "Broken pipe" messages from some
scripts, but those are from bash, when it delivers the SIGPIPE signal,
and not from any application like git.  The EPIPE-handling behavior can
come into play only when SIGPIPE is *ignored*.

> I would have appreciated if this were two separate patches.  I
> think the EPIPE one is an independent issue.  We could even make
> it a configuration option to ignore EPIPE ;-)

Ok.  Even though I'm still convinced that ignoring EPIPE is no longer
justified, I've hamstrung my patch to do what people here want.

Note that I've also changed it not to print strerror(errno) when the
ferror(stdout) test is triggered.  In that case, errno may well be
irrelevant.  The ugliness of this addition is pretty striking, compared
to what I'm used to.  FWIW, I would have liked to handle closing stdout
here with the same one-liner I use in coreutils: atexit (close_stdout);
but that requires autoconf/automake/gnulib infrastructure.

-----------------------------
From 42e3a6f676e9ae4e9640bc2ff36b7ab0b061a60e Mon Sep 17 00:00:00 2001
From: Jim Meyering <jim@meyering.net>
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:

    $ git-diff |wc -c
    3984
    $ git-diff > /dev/full && echo ignored write failure
    ignored write failure

git-log does the same thing:

    $ git-log -n1 > /dev/full && echo ignored write failure
    ignored write failure

Each and every git command should report such a failure.
Some already do, but with the patch below, they all do, and we
won't have to rely on code in each command's implementation to
perform the right incantation.

    $ ./git-log -n1 > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]
    $ ./git-diff > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

You can demonstrate this with git's own --version output, too:
(but git --help detects the failure without this patch)

    $ ./git --version > /dev/full
    fatal: write failure on standard output: No space left on device
    [Exit 128]

Note that the fcntl test (for whether the fileno may be closed) is
required in order to avoid EBADF upon closing an already-closed stdout,
as would happen for each git command that already closes stdout; I think
update-index was the one I noticed in the failure of t5400, before I
added that test.

Also, to be consistent with e.g., write_or_die, do not
diagnose EPIPE write failures.

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 git.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 29b55a1..8258885 100644
--- a/git.c
+++ b/git.c
@@ -308,6 +308,7 @@ 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;
+		int status;
 		if (strcmp(p->cmd, cmd))
 			continue;

@@ -321,7 +322,23 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 			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));
+		status = p->fn(argc, argv, prefix);
+
+		/* Close stdout if necessary, and diagnose any failure
+		   other than EPIPE.  */
+		if (fcntl(fileno (stdout), F_GETFD) >= 0) {
+			errno = 0;
+			if ((ferror(stdout) || fclose(stdout))
+			    && errno != EPIPE) {
+				if (errno == 0)
+					die("write failure on standard output");
+				else
+					die("write failure on standard output"
+					    ": %s", strerror(errno));
+			}
+		}
+
+		exit(status);
 	}
 }

--
1.5.2.73.g18bece

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

[Jim Meyering]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 28 May 2007, Jim Meyering wrote:
>>
>> Um... maybe you've forgotten that this patch fixes a hole in the
>> "old code" (git.c).  Many git tools ignore write (ENOSPC) failures.
>
> Maybe you have not noticed, but my argument has ben about EPIPE.

Ha ha.  That's a good one.
The point was that even you must see that your
"[Jim's] WHOLE patch is crap" statement was wrong.

>>   1) Continue to ignore EPIPE write failure: can obscure real errors.
>>        BTW, Linus, don't you agree?  You never commented on this point.
>
> THAT'S THE ONLY THING I'VE BEEN COMMENTING ON!
>
> They aren't "obscure real errors". EPIPE is neither obscure _nor_ an
> error.

Reread #1, above.
"ignore EPIPE write failure: can obscure real errors" (using "obscure"
as a verb, not an adjective) means that ignoring EPIPE failure can cause
git commands to hide/mask/paper-over a real, conceptual error like this:

    # This is obviously bogus shell code:
    # "cat" with an argument like this doesn't read stdin
    git-rev-list HEAD | cat foo | ...

We're really on the head of a pin here, since the EPIPE test
makes a difference only when SIGPIPE is being ignored (unusual).
Note that ignoring SIGPIPE can cause gross inefficiencies or
even expose bugs.  E.g., On Solaris 10, this infloops:

  (trap '' PIPE; /bin/yes) |head -1

so there are good reasons *not* to ignore SIGPIPE.  Add to that the fact
that the condition provoking an EPIPE is not *that* common, and you
begin to see that even if EPIPE is a little different, it doesn't matter
enough to justify polluting every application file-close test with an
EPIPE exemption.

> The code-paths where you removed EPIPE handlign have two cases:
>  - SIGPIPE happens: you made no change
>  - SIGPIPE diesn't happen: you broke the code.
>
> So remind me again, why the hell do you think your patch is so great and
> so important,

Whoa!  I guess you had a bad day, yesterday.

I try to be humble, and certainly have not been crowing that this
tiny patch is "so great and so important."  However, I did defend it
when you claimed that the whole thing was crap.

> considering that it broke real code, and made things worse?

I believe it is an IMPROVEMENT to make such mistakes detectable (exit
nonzero), and that the risk of annoying users with EPIPE diagnostics is
minimal.  You seem to think there would be some outpouring of "broken
pipe" errors, but since so few Porcelains ignore SIGPIPE, I disagree.

However, it's an improvement only in the unusual event that SIGPIPE is
being ignored, which is also when an application may output the "broken
pipe" error.  IMHO, these conditions are rare enough (now) that there's
no point in making an exception for EPIPE everywhere.

> And why don't you just admit that EPIPE is special, isn't an error, and
> shouldn't be complained about? If you get EPIPE on the write, it means
> "the other end didn't care". It does NOT mean "I should now do a really
> annoying message".

Sure, EPIPE is special, but it is so unusual now that it's not
worth even the small added complexity to treat it specially in all
application code.

> It's that simple. You seem to admit that SIGPIPE handling in bash should
> have been fixed, and that it was annoying to complain about it there. Why

Yes. When bash-3.0 announced each process-killing-via-SIGPIPE with e.g.,
    /some/script: line 2: 31994 Broken pipe     seq 99999
that was a big deal because it affected many scripts.

> can't you just admit that it's annoyign and wrong to complain about the
> same thing when it's EPIPE?

If it happened a lot, it *would* be annoying, but that's just it:
it doesn't happen much at all anymore.

Also, no one is complaining about EPIPE diagnostics from any of
the GNU coreutils, and I take that as a good indication that there
is no problem.

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

[Linus Torvalds]

On Tue, 29 May 2007, Jim Meyering wrote:
> >
> > Maybe you have not noticed, but my argument has ben about EPIPE.
> 
> Ha ha.  That's a good one.
> The point was that even you must see that your
> "[Jim's] WHOLE patch is crap" statement was wrong.

Ehh. That's a rather edited version of what I said, isn't it?

That's after I explicitly _quoted_ the part where you actively removed the 
code that said "EPIPE is right", and also after I had told you several 
times that you should consider EPIPE as a special case in your other part.

In other words, yes, EVERY SINGLE HUNK of your patch was wrong, and I had 
told you exactly why.

How wrong does a patch have to be to be "crap"? Maybe I have higher 
standards than you do (apparently so), but "every single hunk was wrong" 
should certainly be a damn good reason to consider _any_ patch crap, 
wouldn't you say?

And now you have trouble accepting that, even after you have sent out a 
fixed patch without the crap. Thanks for finally bothering to get the 
patch right, but I don't see why you have to try to make-believe that it 
was ever about anything but EPIPE.

So go back and read my emails. You'll see that in every single one I made 
it very clear that EPIPE was special. From the very first one (where I 
didn't call your patch crap, btw: I said it was wrong, and that some 
errors are expected and good, and I explicitly told you about EPIPE).

So what did you do? Instead of acknowledging that EPIPE was different, you 
actually *expanded* on that original patch, and made the other places 
where we _did_ handle EPIPE correctly, and made those places handle it 
_incorrectly_.

And then you expect me to be _polite_ about it? Grow up. I was polite 
before you started explicitly doing the reverse of what I told you you 
should do. At that point, your patch went from "meant well, but the patch 
was wrong" to "That's just obviously crap".

		Linus

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

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> ...
> Also, to be consistent with e.g., write_or_die, do not
> diagnose EPIPE write failures.
>
> Signed-off-by: Jim Meyering <jim@meyering.net>
> ---
>  git.c |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/git.c b/git.c
> index 29b55a1..8258885 100644
> --- a/git.c
> +++ b/git.c
> @@ -308,6 +308,7 @@ 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;
> +		int status;
>  		if (strcmp(p->cmd, cmd))
>  			continue;
>
> @@ -321,7 +322,23 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
>  			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));
> +		status = p->fn(argc, argv, prefix);
> +
> +		/* Close stdout if necessary, and diagnose any failure
> +		   other than EPIPE.  */
> +		if (fcntl(fileno (stdout), F_GETFD) >= 0) {
> +			errno = 0;
> +			if ((ferror(stdout) || fclose(stdout))
> +			    && errno != EPIPE) {
> +				if (errno == 0)
> +					die("write failure on standard output");
> +				else
> +					die("write failure on standard output"
> +					    ": %s", strerror(errno));
> +			}

This makes the final write failure trump the breakage p->fn()
already diagnosed, doesn't it?  Maybe if (fcntrl(...) >=0 )
should read if (!status && fcntrl(...) >= 0).

> +		}
> +
> +		exit(status);
>  	}
>  }
>
> --
> 1.5.2.73.g18bece

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

[Jim Meyering]

Junio C Hamano <junkio@cox.net> wrote:
>> -		exit(p->fn(argc, argv, prefix));
>> +		status = p->fn(argc, argv, prefix);
>> +
>> +		/* Close stdout if necessary, and diagnose any failure
>> +		   other than EPIPE.  */
>> +		if (fcntl(fileno (stdout), F_GETFD) >= 0) {
>> +			errno = 0;
>> +			if ((ferror(stdout) || fclose(stdout))
>> +			    && errno != EPIPE) {
>> +				if (errno == 0)
>> +					die("write failure on standard output");
>> +				else
>> +					die("write failure on standard output"
>> +					    ": %s", strerror(errno));
>> +			}
>
> This makes the final write failure trump the breakage p->fn()
> already diagnosed, doesn't it?

Yes.  Are there circumstances in which a nonzero status from
some cmd_* function would mean something so grave that you
wouldn't also want to know that standard output is incomplete
or corrupt (and possibly use a different exit status)?
So far, after a quick and incomplete survey, I haven't found any.

However, if some git command is documented to exit with
status N for some listed values of N, e.g.,
    1 A happened
    2 B happened
    3 any other failure
then the above choice of dying with "die" would be wrong.

E.g. git-diff's --exit-code comes close:

       --exit-code
           Make the program exit with codes similar to diff(1). That is, it
           exits with 1 if there were differences and 0 means no differences.

but doesn't say how it handles errors.

[ OT: Perhaps that documentation should be changed to look more like diff's,
  so that it says there is a different exit code for the third
  case (some failure):

    $ diff --help|tail -3|head -1
    Exit status is 0 if inputs are the same, 1 if different, 2 if trouble.
]

> Maybe if (fcntrl(...) >=0 )
> should read if (!status && fcntrl(...) >= 0).

No, because then something like git-diff's --exit-code could hide
a write error.

If you want to preserve the exit status, then it should be enough
to call set_die_routine with a function that will work just like
"die" but exit with a specified (status) value.

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

[Jim Meyering]

Junio C Hamano <junkio@cox.net> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>> Of course error messages are annoying when your short-pipe-read is
>> _deliberate_ (tho, most real uses of git tools will actually get no
>> message to be annoyed about[*]), but what if there really *is* a mistake?
>> Try this:
>>
>>     # You want to force git to ignore the error.
>>     $ trap '' PIPE; git-rev-list HEAD | sync
>>     $
>
> It is perfectly valid (although it is stupid) for a Porcelain
> script to do this:
>
>     latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)
>     case "$latest_by_jim" in
>     '') echo "No commit by Jim" ;;
>     *)  # do something interesting on the commit
>         ;;;
>     esac

Hi Junio,

The above snippet (prepending a single #!/bin/bash line) doesn't provoke
an EPIPE diagnostic from my patched git.  In fact, even if you're using
an old, unpatched version of bash, it provokes *no* diagnostic at all.

To provoke a diagnostic (from bash, not git), using old unpatched bash,
you need a script doing output from a subshell, e.g.:

    #!/tmp/bash-3.0/bash
    for x in 1; do
      git-log
    done | head -1

With unpatched bash-3.0, it does this:

    commit 42e3a6f676e9ae4e9640bc2ff36b7ab0b061a60e
    /tmp/bp-demo: line 2: 24864 Broken pipe             git-log

It's only if you try to avoid the above and change your script to
ignore SIGPIPE do you finally get an offending EPIPE diagnostic:

    #!/tmp/bash-3.0/bash
    trap '' PIPE
    for x in 1; do
      ./git-log; echo $? 1>&2
    done | head -1

Here's its output, using my patch:

    commit 42e3a6f676e9ae4e9640bc2ff36b7ab0b061a60e
    fatal: write failure on standard output
    128

That trap has the nasty side effect of making the poorly written script
wait until "git-log" has completed (before, it was interrupted right
away), so it can take a lot longer.  With my patch, it also gives a
diagnostic, which might serve to inform someone that they should not
ignore SIGPIPE.

No porcelain (modulo [*]) in git proper or cogito ignores SIGPIPE,
so I don't see how EPIPE error diagnostics can be a problem.

[*] These scripts do ignore SIGPIPE, but either don't need to,
or can/should be fixed not to:

   git-archimport.perl
   git-cvsimport.perl
   git-svnimport.perl

And, yes, I'd be happy to fix them, if anyone is interested.

> In such a case, it is a bit too much for my taste to force the
> script to redirect what comes out of fd 2 of the upstream of the
> pipe, so that it can filter out only the "write error" message
> but still show other kinds of error messages.  You could do so
> by elaborate shell magic, perhaps like this:
>
>         filter_pipe_error () {
>                 exec 3>&1
>                 (eval "$1" 2>&1 1>&3 | grep >&2 -v 'Broken pipe')
>         }
>
> 	latest_by_jim=$(filter_pipe_error \
>         	'git log --pretty=oneline --author='\''Jim'\'' | head -n 1'
> 	)

I agree that would be extreme.  But it's not necessary, since the
'Broken pipe' diagnostic appears now only in contrived circumstances.

> but what's the point?
>
> I think something like this instead might be more palatable.

...[patch to make git/EPIPE exit nonzero, but with no diagnostic]

Thank you for taking the time to reply and to come up with a compromise.
At first I thought this would be a step in the right direction, but,
now that I understand how infrequently EPIPE actually comes into play,
I think it'd be better to avoid a half-measure fix, since that would
just perpetuate the idea that EPIPE is worth handling specially.

Jim

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

[Jim Meyering]

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, 29 May 2007, Jim Meyering wrote:
>> >
>> > Maybe you have not noticed, but my argument has ben about EPIPE.
>>
>> Ha ha.  That's a good one.
>> The point was that even you must see that your
>> "[Jim's] WHOLE patch is crap" statement was wrong.
>
> Ehh. That's a rather edited version of what I said, isn't it?

No.  I'm glad to see that perhaps even you are surprised by your words.
The only editing was to capitalize WHOLE.  Here's what you wrote:

    > I think this patch is fundamentally WRONG. This fragment is just a prime
    > example of why the whole patch is crap. The old code was correct, and you
    > broke it.

Umm... are the above three lines the only part of my message you're
prepared to talk about?  You haven't addressed any of the interesting
(technical) parts.

> That's after I explicitly _quoted_ the part where you actively removed the
> code that said "EPIPE is right", and also after I had told you several
> times that you should consider EPIPE as a special case in your other part.
>
> In other words, yes, EVERY SINGLE HUNK of your patch was wrong, and I had
> told you exactly why.

And I told you why I think my patch was on the right track.
i.e., why it now appears to be fine to treat EPIPE like any other error.

It is interesting to see that you've elided all of my arguments rather
than make an attempt to rebut them.  I'm trying to keep my side of this
discussion professional, so I'll ignore parts of what you've written
below.

> How wrong does a patch have to be to be "crap"? Maybe I have higher
> standards than you do...

> And now you have trouble accepting that, even after you have sent out a
> fixed patch without the crap. Thanks for finally bothering to get the
> patch right, but I don't see why you have to try to make-believe that it
> was ever about anything but EPIPE.

My original patch was about ENOSPC and EIO.  EPIPE was mostly incidental.
I don't care about EPIPE, and think it deserves no special treatment.
You appear to be obsessed with it now, perhaps because SIGPIPE-ignoring
porcelain (now long gone) once caused trouble.

> So go back and read my emails.

Did you read mine?  I explained why EPIPE is no longer a problem for
git, even if you're using stock bash-2.05b or bash-3.0.

> You'll see that in every single one I made
> it very clear that EPIPE was special.

No.  You merely *said* it was special.
You haven't demonstrated that it's special enough (and still common
enough) to pollute all code that tests for file-close failure.  I hear
that it *used to be* common enough to merit such treatment.  Now, it is
a much harder case to make.  But from what I've seen, you haven't even
tried to do that much.  Hmm... or maybe you did try to make the case,
and came up short.  Is that why you are resorting to hyperbole, and ad
hominem arguments?

> From the very first one (where I
> didn't call your patch crap, btw: I said it was wrong, and that some
> errors are expected and good, and I explicitly told you about EPIPE).
>
> So what did you do? Instead of acknowledging that EPIPE was different, you
> actually *expanded* on that original patch, and made the other places
> where we _did_ handle EPIPE correctly, and made those places handle it
> _incorrectly_.
>
> And then you expect me to be _polite_ about it? Grow up. I was polite
> before you started explicitly doing the reverse of what I told you you
> should do. At that point, your patch went from "meant well, but the patch
> was wrong" to "That's just obviously crap".

Let's see...  I dared to post code contrary to your unsubstantiated claim,
and therefore you describe that code as "obviously crap".

Just because you are Linus doesn't mean you can decree that
"EPIPE must be ignored" and make everyone take it on faith.

Can you substantiate your claim that my proposed changes cause trouble
*in practice*?  So far, all I've heard is FUD, and all of my explanations
of why EPIPE no longer matters seem to have been ignored.

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

[Linus Torvalds]

On Wed, 30 May 2007, Jim Meyering wrote:
> 
> No.  I'm glad to see that perhaps even you are surprised by your words.

If you thought I was a polite person and am surpised by calling things 
crap after the fact, I'm afraid you have a few shocking moments coming to 
you. My reply was fairly polite by my standards. My tag-line is often "On 
the internet, nobody can hear you being subtle".

So let me rephrase my message to you:

	I think your INTERMEDIATE PATCH WAS TOTAL AND UTTER CRAP.

	EVERY SINGLE HUNK WAS SH*T. 

	You expressly IGNORED my point that some errors aren't errors, and 
	MADE GIT WORSE.

Are we on the same page now? 

In contrast, your final patch was fine. The one where you finally fixed 
the issue that I complained about FROM THE VERY BEGINNING.

Comprende?

> The only editing was to capitalize WHOLE.  Here's what you wrote:
> 
>     > I think this patch is fundamentally WRONG. This fragment is just a prime
>     > example of why the whole patch is crap. The old code was correct, and you
>     > broke it.
> 
> Umm... are the above three lines the only part of my message you're
> prepared to talk about?  You haven't addressed any of the interesting
> (technical) parts.

Umm. Your final patch was a few trivial lines. I addressed all interesting 
technical parts IN MY ORIGINAL REPLY WHEN YOU FIRST POSTED IT.

Which you ignored (or rather, explicitly chose to disagree with, and 
added MORE crap to the patch).

Go away. I'm not interested in flaming you any more. The patch wasn't that 
interesting to begin with, and you have shown that you're more interested 
in being contrary than to actually fix the problems that were pointed out 
to you _immediately_ and without any flames. 

			Linus

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

[Jim Meyering]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
[...big flame!...]

Wow!  This is like the scene from the Wizard of Oz:
I've just raised the curtain and seen who's behind it.

Jim

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

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> Junio C Hamano <junkio@cox.net> wrote:
>> Jim Meyering <jim@meyering.net> writes:
>>
>>> Of course error messages are annoying when your short-pipe-read is
>>> _deliberate_ (tho, most real uses of git tools will actually get no
>>> message to be annoyed about[*]), but what if there really *is* a mistake?
>>> Try this:
>>>
>>>     # You want to force git to ignore the error.
>>>     $ trap '' PIPE; git-rev-list HEAD | sync
>>>     $
>>
>> It is perfectly valid (although it is stupid) for a Porcelain
>> script to do this:
>>
>>     latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)
>>     case "$latest_by_jim" in
>>     '') echo "No commit by Jim" ;;
>>     *)  # do something interesting on the commit
>>         ;;;
>>     esac
>
> Hi Junio,
>
> The above snippet (prepending a single #!/bin/bash line) doesn't provoke
> an EPIPE diagnostic from my patched git.  In fact, even if you're using
> an old, unpatched version of bash, it provokes *no* diagnostic at all.

> To provoke a diagnostic (from bash, not git), using old unpatched bash,
> you need a script doing output from a subshell, e.g.:
>
>     #!/tmp/bash-3.0/bash
>     for x in 1; do
>       git-log
>     done | head -1

I haven't thought it through, but isn't the above example only
talking about the "Broken pipe" message?  Surely, you would get
that message from older Bash if you have a shell loop on the
upstream side of the pipe no matter what we (the command that is
run by the shell loop) do, and trap is needed to squelch it.

But I do not see how this pipeline, where git-rev-list produces
more than what fits in the in-kernel pipe buffer:

	"git-rev-list a lot of data | head -n 1"

would not catch EPIPE and say "Broken Pipe" with your patch.
Especially if the downstream is sufficiently slow (say, replace
it with "(sleep 10 && head -n 1)", perhaps), wouldn't the
upstream produce enough without being read, gets stuck on write,
and when the downstream exits, it would notice its write(2)
failed with EPIPE, wouldn't it?

Maybe you are talking about your updated patch?

> ...[patch to make git/EPIPE exit nonzero, but with no diagnostic]
>
> Thank you for taking the time to reply and to come up with a compromise.
> At first I thought this would be a step in the right direction, but,
> now that I understand how infrequently EPIPE actually comes into play,
> I think it'd be better to avoid a half-measure fix, since that would
> just perpetuate the idea that EPIPE is worth handling specially.

After having read what Linus said about how the "fixed" one
would behave differently, depending on the amount of data we
produce before the consumer says "I've seen enough" and
depending on the amount of data that would fit in the in-kernel
pipe buffer, I no longer think the compromise patch you mention
above is improvement anymore.

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

[Jim Meyering]

Junio C Hamano <junkio@cox.net> wrote:
> Jim Meyering <jim@meyering.net> writes:
>> Junio C Hamano <junkio@cox.net> wrote:
>>> Jim Meyering <jim@meyering.net> writes:
>>>
>>>> Of course error messages are annoying when your short-pipe-read is
>>>> _deliberate_ (tho, most real uses of git tools will actually get no
>>>> message to be annoyed about[*]), but what if there really *is* a mistake?
>>>> Try this:
>>>>
>>>>     # You want to force git to ignore the error.
>>>>     $ trap '' PIPE; git-rev-list HEAD | sync
>>>>     $
>>>
>>> It is perfectly valid (although it is stupid) for a Porcelain
>>> script to do this:
>>>
>>>     latest_by_jim=$(git log --pretty=oneline --author='Jim' | head -n 1)
>>>     case "$latest_by_jim" in
>>>     '') echo "No commit by Jim" ;;
>>>     *)  # do something interesting on the commit
>>>         ;;;
>>>     esac
>>
>> Hi Junio,
>>
>> The above snippet (prepending a single #!/bin/bash line) doesn't provoke
>> an EPIPE diagnostic from my patched git.  In fact, even if you're using
>> an old, unpatched version of bash, it provokes *no* diagnostic at all.
>
>> To provoke a diagnostic (from bash, not git), using old unpatched bash,
>> you need a script doing output from a subshell, e.g.:
>>
>>     #!/tmp/bash-3.0/bash
>>     for x in 1; do
>>       git-log
>>     done | head -1
>
> I haven't thought it through, but isn't the above example only
> talking about the "Broken pipe" message?  Surely, you would get

I'm not sure which "Broken pipe" message you mean.

There are two types of "Broken pipe" messages.
There's the old, verbose one from bash that includes script line-number,
pid, and killed command name.  Old, unpatched versions of bash
print that message whenever bash kills a process with SIGPIPE.

Then there's the application (EPIPE) one, that can be printed
by a writing application like git,cat,seq,etc. only when SIGPIPE
stops a write but doesn't kill the writer.
In that case, the write syscall fails with errno==EPIPE,
and if it's diagnosed by the application, you get e.g.,

  seq: write error: Broken pipe

Since the script above is not ignoring SIGPIPE, the git-log process
is killed by bash-3.0, and that old version of bash announces the killing
with the verbose message:

  /t/bp-demo: line 2: 14474 Broken pipe             git-log

Any other patched or newer version of bash will print the
single requested line on stdout and nothing on stderr.

> that message from older Bash if you have a shell loop on the
> upstream side of the pipe no matter what we (the command that is
> run by the shell loop) do, and trap is needed to squelch it.

Right.
That's why no one is using such broken shells anymore.
And why no porcelain tools incur the penalty of ignoring
SIGQUIT anymore either.

> But I do not see how this pipeline, where git-rev-list produces
> more than what fits in the in-kernel pipe buffer:
>
> 	"git-rev-list a lot of data | head -n 1"
>
> would not catch EPIPE and say "Broken Pipe" with your patch.

I haven't debugged the old bash to see why that first one
fails to provoke a broken pipe message (from bash).
Unless you add a line like "trap '' PIPE" before it, bash kills
the writer with SIGPIPE, and so my patch is irrelevant,
because the failing write syscall never returns.

> Especially if the downstream is sufficiently slow (say, replace
> it with "(sleep 10 && head -n 1)", perhaps), wouldn't the
> upstream produce enough without being read, gets stuck on write,
> and when the downstream exits, it would notice its write(2)
> failed with EPIPE, wouldn't it?

Are you presuming SIGPIPE is ignored?

> Maybe you are talking about your updated patch?

I was talking about the initial patch, or the one that
also removed the errno == EPIPE tests.