[RFC] sending errors to stdout under $PAGER

If you do this (and you are not an Emacs user who uses PAGER=cat
in your *shell* buffer):

        $ git init
        Initialized empty Git repository in .git/
        $ echo hello world >foo
        $ H=$(git hash-object -w foo)
        $ git tag -a foo-tag -m "Tags $H" $H
        $ echo $H
        3b18e512dba79e4c8300dd08aeb37f8e728b8dad
        $ rm -f .git/objects/3b/18e5*
        $ git show foo-tag
        tag foo-tag
        Tagger: Junio C Hamano [off-list ref]
        Date:   Sat Feb 16 10:43:23 2008 -0800

        Tags 3b18e512dba79e4c8300dd08aeb37f8e728b8dad

you do not get any indication of error.  If you are careful, you
would notice that no contents from the tagged object is
displayed, but that is about it.  If you run the "show" command
without pager, however, you will see the error:

        $ git --no-pager show foo-tag
        tag foo-tag
        Tagger: Junio C Hamano [off-list ref]
        Date:   Sat Feb 16 10:43:23 2008 -0800

        Tags 3b18e512dba79e4c8300dd08aeb37f8e728b8dad
        error: Could not read object 3b18e512dba79e4c8300dd08aeb37f8e728b8dad

Because we spawn the pager as the foreground process and feed
its input via pipe from the real command, we cannot affect the
exit status the shell sees from git command when the pager is in
use (I think there is not much gain we can have by working it
around, though).  But at least it may make sense to show the
error message to the user sitting in front of the pager, perhaps
like this.

What do people think?  Have I overlooked any downsides?

---
 usage.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/usage.c b/usage.c
index a5fc4ec..681b84a 100644
--- a/usage.c
+++ b/usage.c

@@ -4,12 +4,15 @@
  * Copyright (C) Linus Torvalds, 2005
  */
 #include "git-compat-util.h"
+#include "cache.h"
 
 static void report(const char *prefix, const char *err, va_list params)
 {
 	char msg[256];
+	FILE *outto = (pager_in_use() ? stdout : stderr);
+
 	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s%s\n", prefix, msg);
+	fprintf(outto, "%s%s\n", prefix, msg);
 }
 
 static NORETURN void usage_builtin(const char *err)

Junio C Hamano [off-list ref] wrote:

Because we spawn the pager as the foreground process and feed
its input via pipe from the real command, we cannot affect the
exit status the shell sees from git command when the pager is in
use (I think there is not much gain we can have by working it
around, though).  But at least it may make sense to show the
error message to the user sitting in front of the pager, perhaps
like this.

What do people think?  Have I overlooked any downsides?

I think this is a good idea.

If you are using an interactive pager, you have asked for the content
to come to you through that.  Not unlike how I have chosen to have
the content come to me through a virtual pty and not a printer with
green-and-white bar paper.  :)

I've been bitten by this in the past a few times, but I have also
been knowledgable enough about git, the command I ran, and the
project I ran it on to realize something wasn't right with the
output I am seeing in the pager and retry without the pager to see
the real error(s).

-- 
Shawn.

On Sat, Feb 16, 2008 at 11:15:41AM -0800, Junio C Hamano wrote:

Because we spawn the pager as the foreground process and feed
its input via pipe from the real command, we cannot affect the
exit status the shell sees from git command when the pager is in
use (I think there is not much gain we can have by working it
around, though).  But at least it may make sense to show the
error message to the user sitting in front of the pager, perhaps
like this.

What do people think?  Have I overlooked any downsides?

I think this makes sense. It could be annoying if chatty stderr output
got mixed in with the actual output, making things harder to read. But
git is not very chatty in general, and the point is that things sent to
stderr _should_ grab the user's attention.

The only downside I see is that it disrupts the parsing of the output.
In most cases, this doesn't matter, since anything parsing the output
will disable the pager. The notable exception is something like 'tig',
which I believe can act as a git pager which understands the output; it
can potentially be confused by the extra lines on stdout.

-Peff

Junio C Hamano wrote:

+	FILE *outto = (pager_in_use() ? stdout : stderr);
+
 	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s%s\n", prefix, msg);
+	fprintf(outto, "%s%s\n", prefix, msg);

What do people think?  Have I overlooked any downsides?

Wouldn't it be better/safer to redirect stderr to the pager
in the first place?

So, instead of the current

	foo | less
use
	foo 2>&1 | less

or, in pager.c:

         /* return in the child */
        if (!pid) {
                dup2(fd[1], 1);
+               dup2(fd[1], 2);
                close(fd[0]);
                close(fd[1]);
                return;
        }

Ciao, ET.

Hi,

On Sun, 17 Feb 2008, Edgar Toernig wrote:

Junio C Hamano wrote:

quoted

+	FILE *outto = (pager_in_use() ? stdout : stderr);
+
 	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s%s\n", prefix, msg);
+	fprintf(outto, "%s%s\n", prefix, msg);

What do people think?  Have I overlooked any downsides?

Wouldn't it be better/safer to redirect stderr to the pager
in the first place?

[...]

         /* return in the child */
        if (!pid) {
                dup2(fd[1], 1);
+               dup2(fd[1], 2);
                close(fd[0]);
                close(fd[1]);
                return;
        }

I like it.

Ciao,
Dscho