fatal output from git-show really wants a terminal

fatal output from git-show really wants a terminal

[Tim Olsen]

It appears that when outputting a fatal error, git-show will choose
stdout over stderr if stdout is a terminal and stderr is not.  How do I
redirect the error but still allow stdout to be displayed?

~/git$ mkdir test

~/git$ cd test

~/git/test$ git init

~/git/test$ git show 12345

fatal: ambiguous argument '12345': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions

~/git/test$ git show 12345 2> /dev/null

fatal: ambiguous argument '12345': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions

~/git/test$ git show 12345 > /dev/null

fatal: ambiguous argument '12345': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions

~/git/test$ git show 12345 > /dev/null 2> /dev/null

~/git/test$ git show 12345 > /tmp/out 2> /tmp/err

~/git/test$ cat /tmp/out

~/git/test$ cat /tmp/err

fatal: ambiguous argument '12345': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions

Re: fatal output from git-show really wants a terminal

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Wednesday 2008 December 10 10:01:49 you wrote:
>It appears that when outputting a fatal error, git-show will choose
>stdout over stderr if stdout is a terminal and stderr is not.  How do I
>redirect the error but still allow stdout to be displayed?

Gah, I think that's really bad behavior. Anyway, something like:
git show 12345 2>/dev/null | cat
should work.  Neither stdout nor stderr will be a terminal, but stdout will be 
displayed to your terminal.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss03@volumehost.net                      ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.org/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: fatal output from git-show really wants a terminal

[Johannes Sixt]

On Mittwoch, 10. Dezember 2008, Tim Olsen wrote:
> It appears that when outputting a fatal error, git-show will choose
> stdout over stderr if stdout is a terminal and stderr is not.

This is by design.

> How do I 
> redirect the error but still allow stdout to be displayed?

$ git show 12345 2> /dev/null | less

> ~/git$ mkdir test
> ~/git$ cd test
> ~/git/test$ git init
> ~/git/test$ git show 12345
> fatal: ambiguous argument '12345': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions

You see this through the pager.

> ~/git/test$ git show 12345 2> /dev/null
> fatal: ambiguous argument '12345': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions

And this went through the pager as well.

> ~/git/test$ git show 12345 > /dev/null
> fatal: ambiguous argument '12345': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions

This went straight to the terminal.

The pattern is that if stdout is a terminal, the pager is thrown up and both 
stdout and stderr of git show proper are redirected to the pager. If you 
redirect only stderr, then this redirection is actually ignored.

-- Hannes

Re: fatal output from git-show really wants a terminal

[Tim Olsen]

Johannes Sixt wrote:
> The pattern is that if stdout is a terminal, the pager is thrown up and both 
> stdout and stderr of git show proper are redirected to the pager. If you 
> redirect only stderr, then this redirection is actually ignored.

I see.  So I probably want to use --no-pager.

Thanks for the help.

-Tim

> 
> -- Hannes

Re: fatal output from git-show really wants a terminal

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On Wednesday 2008 December 10 13:46:50 you wrote:
>On Mittwoch, 10. Dezember 2008, Tim Olsen wrote:
>> It appears that when outputting a fatal error, git-show will choose
>> stdout over stderr if stdout is a terminal and stderr is not.
>
>This is by design.

Then it is poor design. :P j/k

Why not use the pager only if git-show is "interactive", using the same test 
for interactivity as SUSv3/POSIX shells use?  IIRC, a shell is interactive if 
both stdin and stderr are terminals.  That test for interactivity -- a 
associated difference in behavior -- predates git by a number of years.  Is 
there a reason for being different?  Is the porcelain consistent about that 
behavior?
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss03@volumehost.net                      ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.org/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: fatal output from git-show really wants a terminal

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 2411 bytes --]

On Thursday 2008 December 11 03:15:47 you wrote:
>On Wed, 10 Dec 2008, Boyd Stephen Smith Jr. wrote:
>> On Wednesday 2008 December 10 13:46:50 you wrote:
>> >On Mittwoch, 10. Dezember 2008, Tim Olsen wrote:
>> >> It appears that when outputting a fatal error, git-show will choose
>> >> stdout over stderr if stdout is a terminal and stderr is not.
>> >
>> >This is by design.
>>
>> Then it is poor design. :P j/k
>
>Read up on the reasoning before trolling, will ya?  It's all in the Git
>history.

Seeing how I'm new, and this message indicated I had screwed up, I starting 
going through the 'git log' looking for a commit message that either 
documented this behavior, or indicated the commit had documented this 
behavior.

Initially, I was looking for 'stdout' or 'stderr', and found many unrelated 
commits.  I then figured it was part of the PAGER support, and began 
searching for that.  I did find an indication of why stdout and stderr are 
both redirected to the PAGER's stdin -- but that makes sense to me; I wasn't 
questioning it.  At least not too much -- but when the user indicates stderr 
and stdout should go to different locations, shouldn't they?

I was mainly questioning using a pager AT ALL when the git command is used in 
a non-interactive environment, and how git detects an interactive invocation.  
I feel this should be done the same way a (POSIX standard) shell detects 
interactivity, and that in a non-interactive environment git should not 
default to using PAGER.

Now, I certainly could have missed the commit message / commit with 
rationale / documentation.  'git log' output is a long document, and I maybe 
using the wrong keywords for my search.  It also is not all the documentation 
that is out there.  I'm not afraid to RTFM; but I'm not having much luck 
finding the right parts to R.

Finally, I didn't mean to offend.  I was hoping the smiley (":P") and "j/k" 
would indicate that a was only half serious and know that I don't have the 
benefit of following the project closely for very long.  I'm appreciative of 
the hard work that goes into git and don't mean to belittle that effort.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss03@volumehost.net                      ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.org/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: fatal output from git-show really wants a terminal

[Jeff King]

On Thu, Dec 11, 2008 at 10:51:15AM -0600, Boyd Stephen Smith Jr. wrote:

> Initially, I was looking for 'stdout' or 'stderr', and found many unrelated 
> commits.  I then figured it was part of the PAGER support, and began 

Try looking for isatty, which takes the numeric fd. I think the behavior
you asked about would be this:

diff --git a/pager.c b/pager.c
index aa0966c..19f8856 100644
--- a/pager.c
+++ b/pager.c
@@ -42,7 +42,7 @@ void setup_pager(void)
 {
 	const char *pager = getenv("GIT_PAGER");
 
-	if (!isatty(1))
+	if (!isatty(0) || !isatty(2))
 		return;
 	if (!pager) {
 		if (!pager_program)

which is what is mentioned in POSIX:

  http://www.opengroup.org/onlinepubs/009695399/utilities/sh.html

But I don't think that makes sense here. We are not talking about
interactivity, but rather about where the output is going. So your test
would consider this interactive:

  $ git log >foo.out

and start a pager, which makes no sense.

Now if you proposed checking stderr and stdin _in addition_ to stdout,
that might make more sense, but I haven't thought too hard about any
implications.

And FWIW, I don't recall this ever being discussed before, but then I
have not been involved with git since the very beginning.

-Peff

Re: fatal output from git-show really wants a terminal

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

On Thursday 2008 December 11 15:55:55 Jeff King wrote:
>On Thu, Dec 11, 2008 at 10:51:15AM -0600, Boyd Stephen Smith Jr. wrote:
>> Initially, I was looking for 'stdout' or 'stderr', and found many
>> unrelated commits.  I then figured it was part of the PAGER support, and
>> began
>
>Try looking for isatty, which takes the numeric fd. I think the behavior
>you asked about would be this:

Thanks, this will be plenty of context for me to be able to crawl the archives 
and the actual history thinking about better behavior and considering old 
discussions.

>We are not talking about
>interactivity, but rather about where the output is going. So your test
>would consider this interactive:
>
>  $ git log >foo.out
>
>and start a pager, which makes no sense.

Good point, I'll try and consider that while I investgate the history of the 
issue.

>Now if you proposed checking stderr and stdin _in addition_ to stdout,
>that might make more sense, but I haven't thought too hard about any
>implications.

I did see a commit message mentioning some unusual settings for PAGER, but in 
general, pagers are interactive.  I'd think the default behavior would 
be "interactive <-> pager", with a config option to turn the pager always off 
or always on.  From there, I would reason the test for interactivity should 
be the POSIX test.

It looks like this test have have been attempting to follow the behavior 
of --color=auto to GNU less and GNU grep (and possibly others).  This 
certainly makes some sense as well, and may be less surprising.

>And FWIW, I don't recall this ever being discussed before, but then I
>have not been involved with git since the very beginning.

Google should be able to find it.  And worst-case, I can tell wget to spider 
the archives and then run some sort of find/html2txt/grep on them.

I'll go back to "the stacks" and read the discussions and commits.  If my 
well-informed self still thinks the behavior should change, I'll write a 
patch and open up the discussion again.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss03@volumehost.net                      ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.org/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: fatal output from git-show really wants a terminal

[Jeff King]

On Thu, Dec 11, 2008 at 04:45:05PM -0600, Boyd Stephen Smith Jr. wrote:

> I did see a commit message mentioning some unusual settings for PAGER, but in 
> general, pagers are interactive.  I'd think the default behavior would 
> be "interactive <-> pager", with a config option to turn the pager always off 
> or always on.  From there, I would reason the test for interactivity should 
> be the POSIX test.

Right, but then that leads to the case I mentioned before. I think you
want to say "this is interactive _and_ our stdout is going to the
interactive spot". Which by your definition would be isatty() on stdin,
stderr, and stdout.

And maybe that is a better test, but I think it would be helpful to
provide a concrete example where that behavior works and the current
behavior doesn't.

> It looks like this test have have been attempting to follow the behavior 
> of --color=auto to GNU less and GNU grep (and possibly others).  This 
> certainly makes some sense as well, and may be less surprising.

Yes. You'll see we use a similar test for git's "auto" color.

> >And FWIW, I don't recall this ever being discussed before, but then I
> >have not been involved with git since the very beginning.
> 
> Google should be able to find it.  And worst-case, I can tell wget to spider 
> the archives and then run some sort of find/html2txt/grep on them.

I have the complete archive, and I couldn't find anything useful. ;)

Let me know if you want a copy (or you can pull it straight from gmane,
but it is somewhat slow IIRC).

-Peff

Re: fatal output from git-show really wants a terminal

[Junio C Hamano]

"Boyd Stephen Smith Jr." <bss03@volumehost.net> writes:

>>  $ git log >foo.out
>>
>>and start a pager, which makes no sense.
>
> Good point, I'll try and consider that while I investgate the history of the 
> issue.

Isn't the issue about 61b8050 (sending errors to stdout under $PAGER,
2008-02-16)?  With that commit, we changed things so that when we send the
standard output to the $PAGER, we dup stderr to the $PAGER as well,
because otherwise any output to stderr will be wiped out by whatever the
pager does and the user will not notice the breakage.  E.g.

	$ git log

will just show reams of output, and you won't see any errors and warnings
even if there were any encountered during the process.

Unfortunately we did it unconditionally.  There is no reason to dup stderr
to the $PAGER if the command line was:

	$ git log 2>error.log

in which case you would want to view the normal output in your $PAGER and
you are keeping the log of the error output in a separate file.

Re: fatal output from git-show really wants a terminal

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Boyd Stephen Smith Jr." <bss03@volumehost.net> writes:
>
>>>  $ git log >foo.out
>>>
>>>and start a pager, which makes no sense.
>>
>> Good point, I'll try and consider that while I investgate the history of the 
>> issue.
>
> Isn't the issue about 61b8050 (sending errors to stdout under $PAGER,
> 2008-02-16)?  With that commit, we changed things so that when we send the
> standard output to the $PAGER, we dup stderr to the $PAGER as well,
> because otherwise any output to stderr will be wiped out by whatever the
> pager does and the user will not notice the breakage.  E.g.
>
> 	$ git log
>
> will just show reams of output, and you won't see any errors and warnings
> even if there were any encountered during the process.
>
> Unfortunately we did it unconditionally.  There is no reason to dup stderr
> to the $PAGER if the command line was:
>
> 	$ git log 2>error.log
>
> in which case you would want to view the normal output in your $PAGER and
> you are keeping the log of the error output in a separate file.

I haven't heard anything more about this, but if you were indeed
discussing the change made by 61b8050, I think the fix should just be like
this.

 pager.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git c/pager.c w/pager.c
index aa0966c..f19ddbc 100644
--- c/pager.c
+++ w/pager.c
@@ -70,7 +70,8 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	dup2(pager_process.in, 2);
+	if (isatty(2))
+		dup2(pager_process.in, 2);
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */

Re: fatal output from git-show really wants a terminal

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Isn't the issue about 61b8050 (sending errors to stdout under $PAGER,
>> 2008-02-16)?  With that commit, we changed things so that when we send the
>> standard output to the $PAGER, we dup stderr to the $PAGER as well,
>> because otherwise any output to stderr will be wiped out by whatever the
>> pager does and the user will not notice the breakage.  E.g.
>>
>> 	$ git log
>>
>> will just show reams of output, and you won't see any errors and warnings
>> even if there were any encountered during the process.
>>
>> Unfortunately we did it unconditionally.

By the by, I noticed that the example shown in the commit message of
61b8050 has been broken since 4f3dcc2 (Fix 'git show' on signed tag of
signed tag of commit, 2008-07-01).

Here is a fix.

 builtin-log.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git c/builtin-log.c w/builtin-log.c
index 840daf9..99d1137 100644
--- c/builtin-log.c
+++ w/builtin-log.c
@@ -340,7 +340,13 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 					t->tag,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			ret = show_object(o->sha1, 1, &rev);
-			objects[i].item = parse_object(t->tagged->sha1);
+			if (ret)
+				break;
+			o = parse_object(t->tagged->sha1);
+			if (!o)
+				ret = error("Could not read object %s",
+					    sha1_to_hex(t->tagged->sha1));
+			objects[i].item = o;
 			i--;
 			break;
 		}

Re: fatal output from git-show really wants a terminal

[Jeff King]

On Mon, Dec 15, 2008 at 12:15:38AM -0800, Junio C Hamano wrote:

> I haven't heard anything more about this, but if you were indeed
> discussing the change made by 61b8050, I think the fix should just be like
> this.
> [...]
> -	dup2(pager_process.in, 2);
> +	if (isatty(2))
> +		dup2(pager_process.in, 2);

I can't speak for the original poster, but I do think this behavior
should be less surprising to users who perform the one particular
redirection (and in the other 99% of cases, should behave exactly the
same). So I think it's a positive change.

-Peff