Bugreport: Git responds with stderr instead of stdout

Bugreport: Git responds with stderr instead of stdout

[Jack Desert]

I think I found a bug in Git. When I run the command 

  git checkout -b new_branch

Git does exactly what I've asked, except that Git's response:
  
  Switched to a new branch 'new_branch'

comes through the stderr pipe instead of through the stdout pipe. Where do I file a bug report for this? 

I am using Git 1.6.3.3, Ubuntu 9.10

-Jack


-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Jack Desert     --    Writer, Entrepeneur
Author and Spokesman: www.LetsEATalready.com
Software Developer:   http://GrooveTask.org
Email: JackDesert556@gmail.com
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Re: Bugreport: Git responds with stderr instead of stdout

[Jacob Helwig]

On Sun, Apr 25, 2010 at 11:06, Jack Desert <jackdesert556@gmail.com> wrote:
> I think I found a bug in Git. When I run the command
>
>  git checkout -b new_branch
>
> Git does exactly what I've asked, except that Git's response:
>
>  Switched to a new branch 'new_branch'
>
> comes through the stderr pipe instead of through the stdout pipe. Where do I file a bug report for this?
>
> I am using Git 1.6.3.3, Ubuntu 9.10
>
> -Jack
>
>

I can't really say if it's actually a bug, or not, but as to your
question about where to file a bug report: You just did.  This mailing
list is the correct place.

Re: Bugreport: Git responds with stderr instead of stdout

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 25, 2010 at 18:10, Jacob Helwig <jacob.helwig@gmail.com> wrote:
> I can't really say if it's actually a bug, or not, but as to your
> question about where to file a bug report: You just did.  This mailing
> list is the correct place.

I've had some issues scripting `git fetch` because on error it'll
print to stdout and not stderr.

Are there some general guidelines for git's utilities that they follow
in this regard or does each tool just do its own thing?

Re: Bugreport: Git responds with stderr instead of stdout

[Jack Desert]

El Sun, 25 Apr 2010 11:10:47 -0700
Jacob Helwig <jacob.helwig@gmail.com> escribió:
> On Sun, Apr 25, 2010 at 11:06, Jack Desert <jackdesert556@gmail.com> wrote:
> > I think I found a bug in Git. When I run the command
> >
> >  git checkout -b new_branch
> >
> > Git does exactly what I've asked, except that Git's response:
> >
> >  Switched to a new branch 'new_branch'
> >
> > comes through the stderr pipe instead of through the stdout pipe. Where do I file a bug report for this?
> >
> > I am using Git 1.6.3.3, Ubuntu 9.10
> >
> > -Jack
> >
> >
> 
> I can't really say if it's actually a bug, or not, but as to your
> question about where to file a bug report: You just did.  This mailing
> list is the correct place.

I just finished testing with the latest development version and it has the same issue that 1.6.3.3 has in this regard. 


-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Jack Desert     --    Writer, Entrepeneur
Author and Spokesman: www.LetsEATalready.com
Software Developer:   http://GrooveTask.org
Email: JackDesert556@gmail.com
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Re: Bugreport: Git responds with stderr instead of stdout

[Jeff King]

On Sun, Apr 25, 2010 at 06:24:43PM +0000, Ævar Arnfjörð Bjarmason wrote:

> On Sun, Apr 25, 2010 at 18:10, Jacob Helwig <jacob.helwig@gmail.com> wrote:
> > I can't really say if it's actually a bug, or not, but as to your
> > question about where to file a bug report: You just did.  This mailing
> > list is the correct place.
> 
> I've had some issues scripting `git fetch` because on error it'll
> print to stdout and not stderr.

Errors should go to stderr, so I imagine patches would be welcome. Which
messages went to stdout?

> Are there some general guidelines for git's utilities that they follow
> in this regard or does each tool just do its own thing?

In practice, each tool does its own thing because they evolved
differently and from different authors. I think we are slowly converging
on similar behavior, though, as people fix warts.  As to exactly what
that behavior is, I don't know that anybody has ever enumerated it
exactly. Verbose status and progress reports, especially human readable
ones, should probably always go to stderr.

The "Switched to a new branch" message that started this thread is
correct to go to stderr.  If you want to silence the message but keep
stderr open for actual errors, the right way is to use "-q".

I tend to think the only thing that should go to stdout is the "main"
output of a command. For something like "ls-files", that is obviously
the list of files. For something like "checkout", which is about
changing the repository and not about querying it, I think there is
probably nothing that makes sense on stdout.

-Peff

Re: Bugreport: Git responds with stderr instead of stdout

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 25, 2010 at 19:22, Jeff King <peff@peff.net> wrote:
> On Sun, Apr 25, 2010 at 06:24:43PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> On Sun, Apr 25, 2010 at 18:10, Jacob Helwig <jacob.helwig@gmail.com> wrote:
>> > I can't really say if it's actually a bug, or not, but as to your
>> > question about where to file a bug report: You just did.  This mailing
>> > list is the correct place.
>>
>> I've had some issues scripting `git fetch` because on error it'll
>> print to stdout and not stderr.
>
> Errors should go to stderr, so I imagine patches would be welcome. Which
> messages went to stdout?

I can't recall exactly now. Looking at fetch.c I can't see anything
obvious, I'll report anything if I spot it in the future.

Re: Bugreport: Git responds with stderr instead of stdout

[Jeff King]

On Sun, Apr 25, 2010 at 07:32:00PM +0000, Ævar Arnfjörð Bjarmason wrote:

> >> I've had some issues scripting `git fetch` because on error it'll
> >> print to stdout and not stderr.
> >
> > Errors should go to stderr, so I imagine patches would be welcome. Which
> > messages went to stdout?
> 
> I can't recall exactly now. Looking at fetch.c I can't see anything
> obvious, I'll report anything if I spot it in the future.

Thanks. As I mentioned, we've been fixing little things like this as
time goes on, so it may well have been fixed already.

-Peff

Re: Bugreport: Git responds with stderr instead of stdout

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 25, 2010 at 19:32, Jeff King <peff@peff.net> wrote:
> On Sun, Apr 25, 2010 at 07:32:00PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> >> I've had some issues scripting `git fetch` because on error it'll
>> >> print to stdout and not stderr.
>> >
>> > Errors should go to stderr, so I imagine patches would be welcome. Which
>> > messages went to stdout?
>>
>> I can't recall exactly now. Looking at fetch.c I can't see anything
>> obvious, I'll report anything if I spot it in the future.
>
> Thanks. As I mentioned, we've been fixing little things like this as
> time goes on, so it may well have been fixed already.

Actually here's an example with Git 1.7.1:

    # time /etc/github-backup/github-backup
    remote: Counting objects: 76, done.
    remote: Compressing objects: 100% (43/43), done.
    remote: Total 47 (delta 26), reused 18 (delta 4)
    Unpacking objects: 100% (47/47), done.
    From github.com:avar/linode-etc
       75a27cf..09d5ff7  master     -> origin/master
    From github.com:avar/svn-dump-fast-export
     * [new branch]      gh-pages   -> origin/gh-pages
     * [new branch]      git-merge  -> origin/git-merge
     * [new branch]      master     -> origin/master
     * [new branch]      rollout    -> origin/rollout

The script I'm running is github-backup
(http://github.com/avar/github-backup) which just outputs `git fetch`
output as-is.

Looking at the source the problematic code is in builtin/fetch.c's
update_local_ref. That function takes a char *display which it writes
to things that are both errors and just status messages:

Error:

		sprintf(display, "! %-*s %-*s -> %s  (can't fetch in current branch)",
			TRANSPORT_SUMMARY_WIDTH, "[rejected]", REFCOL_WIDTH, remote,
			pretty_ref);

Just a status message (in my case):

		else {
			msg = "storing head";
			what = "[new branch]";
		}

		r = s_update_ref(msg, ref, 0);
		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '*',
			TRANSPORT_SUMMARY_WIDTH, what, REFCOL_WIDTH, remote, pretty_ref,
			r ? "  (unable to update local ref)" : "");

That function is then called as:

		if (ref) {
			rc |= update_local_ref(ref, what, note);
			free(ref);
		} else
			sprintf(note, "* %-*s %-*s -> FETCH_HEAD",
				TRANSPORT_SUMMARY_WIDTH, *kind ? kind : "branch",
				 REFCOL_WIDTH, *what ? what : "HEAD");
		if (*note) {
			if (verbosity >= 0 && !shown_url) {
				fprintf(stderr, "From %.*s\n",
						url_len, url);
				shown_url = 1;
			}
			if (verbosity >= 0)
				fprintf(stderr, " %s\n", note);
		}

Shouldn't that fprintf() be called as:

    fprintf((rc ? stderr : stdout), ...)

?

[PATCH] fetch: don't output non-errors on stderr

[Ævar Arnfjörð Bjarmason]

Change git-fetch to only print to stderr if it has encountered an
error.

A normal branch update (like "* branch HEAD -> FETCH_HEAD") is no
longer output to stderr but on stdout. Genuine errors (like
"[rejected]" messages) still go to stderr.

With this change I can run a cron script I've been developing
(http://github.com/avar/github-backup) without redirecting stderr to
/dev/null.

Before the change error messages were drowned out by git-fetch's
non-error update notices, which didn't need my attention.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Jun 12, 2010 at 16:52, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> Shouldn't that fprintf() be called as:
>
>    fprintf((rc ? stderr : stdout), ...)

To answer my own question: Yes it should. This patch fixes git-fetch
so that it doesn't taint stderr with non-error messages.

The small changes to the test suite that this requires is a testiment
to how bad our test coverage is in this area. As far as I can see the
error messages that update_local_ref can emit aren't being tested
for. Fixing that is outside the scope of this patch, however.

 builtin/fetch.c         |    5 +++--
 t/t5521-pull-options.sh |   12 ++++++------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5cb369c..116b322 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -397,13 +397,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				TRANSPORT_SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
+			FILE *fout = rc ? stderr : stdout;
 			if (verbosity >= 0 && !shown_url) {
-				fprintf(stderr, "From %.*s\n",
+				fprintf(fout, "From %.*s\n",
 						url_len, url);
 				shown_url = 1;
 			}
 			if (verbosity >= 0)
-				fprintf(stderr, " %s\n", note);
+				fprintf(fout, " %s\n", note);
 		}
 	}
 	free(url);
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 1b06691..7ec36e7 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -23,16 +23,16 @@ test_expect_success 'git pull' '
 	mkdir cloned &&
 	(cd cloned && git init &&
 	git pull "../parent" >out 2>err &&
-	test -s err &&
-	test ! -s out)
+	test ! -s err &&
+	test -s out)
 '
 
 test_expect_success 'git pull -v' '
 	mkdir clonedv &&
 	(cd clonedv && git init &&
 	git pull -v "../parent" >out 2>err &&
-	test -s err &&
-	test ! -s out)
+	test ! -s err &&
+	test -s out)
 '
 
 test_expect_success 'git pull -v -q' '
@@ -47,8 +47,8 @@ test_expect_success 'git pull -q -v' '
 	mkdir clonedqv &&
 	(cd clonedqv && git init &&
 	git pull -q -v "../parent" >out 2>err &&
-	test ! -s out &&
-	test -s err)
+	test ! -s err &&
+	test -s out)
 '
 
 test_expect_success 'git pull --force' '
-- 
1.7.1.251.g92a7

[PATCH v2] fetch: don't output non-errors on stderr

[Ævar Arnfjörð Bjarmason]

Change git-fetch to only print to stderr if it has encountered an
error. A normal branch update (like "* branch HEAD -> FETCH_HEAD") is
no longer output to stderr but on stdout. Genuine errors (like
"[rejected]" messages) still go to stderr.

With this change I can run a cron script I've been developing
(http://github.com/avar/github-backup) without redirecting stderr to
/dev/null.

Before the change error messages were drowned out by git-fetch's
non-error update notices, which didn't need my attention.

The changes in t/t5521-pull-options.sh invert the previously tested
for behavior of checking if normal messages are output on stderr. The
changes in t/t5510-fetch.sh however contain no behavioral changes,
just assertions that will break if git fetch's behavior is changed
again.

There still aren't tests for some of the errors output by
builtin/fetch.c's update_local_ref function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Thu, Jun 24, 2010 at 22:34, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> The small changes to the test suite that this requires is a testiment
> to how bad our test coverage is in this area. As far as I can see the
> error messages that update_local_ref can emit aren't being tested
> for. Fixing that is outside the scope of this patch, however.

Here's an updated patch that has a some of those supposedly out of
scope tests. The new tests have the same behavior, but will start
breaking if this behavior is changed again.

 builtin/fetch.c         |    5 ++-
 t/t5510-fetch.sh        |   57 +++++++++++++++++++++++++++++++++-------------
 t/t5521-pull-options.sh |   12 +++++-----
 3 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5cb369c..116b322 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -397,13 +397,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				TRANSPORT_SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
+			FILE *fout = rc ? stderr : stdout;
 			if (verbosity >= 0 && !shown_url) {
-				fprintf(stderr, "From %.*s\n",
+				fprintf(fout, "From %.*s\n",
 						url_len, url);
 				shown_url = 1;
 			}
 			if (verbosity >= 0)
-				fprintf(stderr, " %s\n", note);
+				fprintf(fout, " %s\n", note);
 		}
 	}
 	free(url);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 4eb10f6..808b256 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -51,7 +51,9 @@ test_expect_success "fetch test" '
 	echo >file updated by origin &&
 	git commit -a -m "updated by origin" &&
 	cd two &&
-	git fetch &&
+	git fetch >out 2>err &&
+	test ! -s err &&
+	test -s out &&
 	test -f .git/refs/heads/one &&
 	mine=`git rev-parse refs/heads/one` &&
 	his=`cd ../one && git rev-parse refs/heads/master` &&
@@ -61,7 +63,9 @@ test_expect_success "fetch test" '
 test_expect_success "fetch test for-merge" '
 	cd "$D" &&
 	cd three &&
-	git fetch &&
+	git fetch >out 2>err &&
+	test ! -s err &&
+	test -s out &&
 	test -f .git/refs/heads/two &&
 	test -f .git/refs/heads/one &&
 	master_in_two=`cd ../two && git rev-parse master` &&
@@ -81,7 +85,9 @@ test_expect_success 'fetch tags when there is no tags' '
     cd notags &&
     git init &&
 
-    git fetch -t ..
+    git fetch -t .. >out 2>err &&
+    test ! -s err &&
+    test ! -s out
 
 '
 
@@ -95,7 +101,10 @@ test_expect_success 'fetch following tags' '
 	cd four &&
 	git init &&
 
-	git fetch .. :track &&
+	git fetch .. :track >out 2>err &&
+	test ! -s err &&
+	test -s out &&
+
 	git show-ref --verify refs/tags/anno &&
 	git show-ref --verify refs/tags/light
 
@@ -109,8 +118,9 @@ test_expect_success 'fetch must not resolve short tag name' '
 	cd five &&
 	git init &&
 
-	test_must_fail git fetch .. anno:five
-
+	! git fetch .. anno:five >out 2>err &&
+	test -s err &&
+	test ! -s out
 '
 
 test_expect_success 'fetch must not resolve short remote name' '
@@ -122,7 +132,9 @@ test_expect_success 'fetch must not resolve short remote name' '
 	cd six &&
 	git init &&
 
-	test_must_fail git fetch .. six:six
+	! git fetch .. six:six >out 2>err &&
+	test -s err &&
+	test ! -s out
 
 '
 
@@ -148,7 +160,9 @@ test_expect_success 'create bundle 2' '
 test_expect_success 'unbundle 1' '
 	cd "$D/bundle" &&
 	git checkout -b some-branch &&
-	test_must_fail git fetch "$D/bundle1" master:master
+	! git fetch "$D/bundle1" master:master >out 2>err &&
+	test -s err &&
+	test ! -s out
 '
 
 
@@ -167,7 +181,9 @@ test_expect_success 'bundle 1 has only 3 files ' '
 
 test_expect_success 'unbundle 2' '
 	cd "$D/bundle" &&
-	git fetch ../bundle2 master:master &&
+	git fetch ../bundle2 master:master >out 2>err &&
+	test ! -s err &&
+	test -s out &&
 	test "tip" = "$(git log -1 --pretty=oneline master | cut -b42-)"
 '
 
@@ -203,7 +219,9 @@ test_expect_success 'fetch via rsync' '
 	mkdir rsynced &&
 	(cd rsynced &&
 	 git init --bare &&
-	 git fetch "rsync:$(pwd)/../.git" master:refs/heads/master &&
+	 git fetch "rsync:$(pwd)/../.git" master:refs/heads/master >out 2>err &&
+	 test ! -s err &&
+	 test -s out &&
 	 git gc --prune &&
 	 test $(git rev-parse master) = $(cd .. && git rev-parse master) &&
 	 git fsck --full)
@@ -237,14 +255,17 @@ test_expect_success 'fetch with a non-applying branch.<name>.merge' '
 	git config branch.master.merge refs/heads/bigfoot &&
 	git config remote.blub.url one &&
 	git config remote.blub.fetch "refs/heads/*:refs/remotes/one/*" &&
-	git fetch blub
+	git fetch blub >out 2>err &&
+	test ! -s err &&
+	test -s out
 '
 
 # the strange name is: a\!'b
 test_expect_success 'quoting of a strangely named repo' '
-	test_must_fail git fetch "a\\!'\''b" > result 2>&1 &&
-	cat result &&
-	grep "fatal: '\''a\\\\!'\''b'\''" result
+	test_must_fail git fetch "a\\!'\''b" >out 2>err &&
+	test -s err &&
+	test ! -s out &&
+	grep "fatal: '\''a\\\\!'\''b'\''" err
 '
 
 test_expect_success 'bundle should record HEAD correctly' '
@@ -267,7 +288,9 @@ test_expect_success 'explicit fetch should not update tracking' '
 	(
 		cd three &&
 		o=$(git rev-parse --verify refs/remotes/origin/master) &&
-		git fetch origin master &&
+		git fetch origin master >out 2>err &&
+		test ! -s err &&
+		test -s out &&
 		n=$(git rev-parse --verify refs/remotes/origin/master) &&
 		test "$o" = "$n" &&
 		test_must_fail git rev-parse --verify refs/remotes/origin/side
@@ -295,7 +318,9 @@ test_expect_success 'configured fetch updates tracking' '
 	(
 		cd three &&
 		o=$(git rev-parse --verify refs/remotes/origin/master) &&
-		git fetch origin &&
+		git fetch origin  >out 2>err &&
+		test ! -s err &&
+		test -s out &&
 		n=$(git rev-parse --verify refs/remotes/origin/master) &&
 		test "$o" != "$n" &&
 		git rev-parse --verify refs/remotes/origin/side
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 1b06691..7ec36e7 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -23,16 +23,16 @@ test_expect_success 'git pull' '
 	mkdir cloned &&
 	(cd cloned && git init &&
 	git pull "../parent" >out 2>err &&
-	test -s err &&
-	test ! -s out)
+	test ! -s err &&
+	test -s out)
 '
 
 test_expect_success 'git pull -v' '
 	mkdir clonedv &&
 	(cd clonedv && git init &&
 	git pull -v "../parent" >out 2>err &&
-	test -s err &&
-	test ! -s out)
+	test ! -s err &&
+	test -s out)
 '
 
 test_expect_success 'git pull -v -q' '
@@ -47,8 +47,8 @@ test_expect_success 'git pull -q -v' '
 	mkdir clonedqv &&
 	(cd clonedqv && git init &&
 	git pull -q -v "../parent" >out 2>err &&
-	test ! -s out &&
-	test -s err)
+	test ! -s err &&
+	test -s out)
 '
 
 test_expect_success 'git pull --force' '
-- 
1.7.1.251.g92a7

Re: [PATCH] fetch: don't output non-errors on stderr

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Before the change error messages were drowned out by git-fetch's
> non-error update notices, which didn't need my attention.

I don't understand this part; care to elaborate?

Re: [PATCH] fetch: don't output non-errors on stderr

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 25, 2010 at 17:25, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Before the change error messages were drowned out by git-fetch's
>> non-error update notices, which didn't need my attention.
>
> I don't understand this part; care to elaborate?

I have a cron job (github-backup) that calls git fetch. Without this
patch I have to run it as '> /dev/null 2>&1' and just rely on the exit
code, with it I can just do '> /dev/null' and not ignore stderr,
because non-error output isn't being sent there anymore.

In short, the current git-fetch breaks the conventional *nix
assumption that stderr only contains errors. With this patch errors go
to stderr and normal output to stdout.

Re: [PATCH] fetch: don't output non-errors on stderr

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Jun 25, 2010 at 17:25, Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> Before the change error messages were drowned out by git-fetch's
>>> non-error update notices, which didn't need my attention.
>>
>> I don't understand this part; care to elaborate?
>
> I have a cron job (github-backup) that calls git fetch. Without this
> patch I have to run it as '> /dev/null 2>&1' and just rely on the exit
> code,

Signaling failure with exit code is _the_ standard practice, no?

Some people seem to think unclean standard error means some error (most
notably tcl ;-), but I think they are mistaken.

Not that I care too much about this issue, though.  I might end up queuing
it, but we need to think about things like advice messages and such
(e.g. 011fe98 (git-push: fix an advice message so it goes to stderr,
2010-02-26)).

Re: [PATCH] fetch: don't output non-errors on stderr

[Jeff King]

On Fri, Jun 25, 2010 at 02:43:27PM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > On Fri, Jun 25, 2010 at 17:25, Junio C Hamano <gitster@pobox.com> wrote:
> >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >>
> >>> Before the change error messages were drowned out by git-fetch's
> >>> non-error update notices, which didn't need my attention.
> >>
> >> I don't understand this part; care to elaborate?
> >
> > I have a cron job (github-backup) that calls git fetch. Without this
> > patch I have to run it as '> /dev/null 2>&1' and just rely on the exit
> > code,
> 
> Signaling failure with exit code is _the_ standard practice, no?
> 
> Some people seem to think unclean standard error means some error (most
> notably tcl ;-), but I think they are mistaken.

Agreed. I thought it was intentional for any human-readable progress
messages go to stderr. It is true in the case of git-fetch that stdout
is not being used for anything, but:

  1. Git should be consistent about where output goes. And other
     programs may actually produce useful output on stdout, which should
     not be mixed with human-readable verbose messages. For the sake of
     those programs, we should be consistent about sending the output to
     stderr.

  2. Fetch may be combined with other operations in script. Consider
     this toy script to print the latest origin/master to stdout:

       #!/bin/sh
       git fetch && git rev-parse origin/master

     The user sees verbose cruft on stderr, but the interesting part is
     on stdout. With your patch, the script is now broken. Yes, it's
     obviously a toy, but I don't think it inconceivable that somebody
     would not want fetch unexpectedly polluting their stdout. Even if
     it would have been a better behavior in the first place (which I
     don't agree with), changing it now means breaking scripts.

The real problem is that git is very chatty compared to other unix
programs. Most produce no output at all unless there is an error, or
human-readable non-error output on stderr only if it is a tty. We do
this already with progress meters, and this output is just another form
of progress update. So I think a patch to quell the status table when
stderr is not a tty would be better (that still can break some scripts,
too, but I am less sympathetic to people trying to save and parse
human-readable stderr messages).

Or even easier: is there a reason that "git fetch -q" would not do what
you (Ævar) want?

-Peff

Re: [PATCH] fetch: don't output non-errors on stderr

[Ævar Arnfjörð Bjarmason]

On Sat, Jun 26, 2010 at 06:13, Jeff King <peff@peff.net> wrote:

> Or even easier: is there a reason that "git fetch -q" would not do what
> you (Ævar) want?

That'd reduce the verbosity level, which'd skip some messages that I
might want. E.g.:

		if (verbosity >= 0) {
			fprintf(stderr, " x %-*s %-*s -> %s\n",
				TRANSPORT_SUMMARY_WIDTH, "[deleted]",
				REFCOL_WIDTH, "(none)", prettify_refname(ref->name));

Anyway, it looks like the only correct way to do this with Git in
general is to:

    1. Capture stderr and stdout
    2. Check the exit code, and if it's non-zero print both

But it sounds like we need some general discussion on what stdout and
stderr should be used for in Git with regards to progress messages,
errors and other similar things.

Re: [PATCH] fetch: don't output non-errors on stderr

[Tay Ray Chuan]

Hi,

On Sat, Jun 26, 2010 at 8:14 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> But it sounds like we need some general discussion on what stdout and
> stderr should be used for in Git with regards to progress messages,
> errors and other similar things.

I'd say - submit a patch for the style guide with your proposed
guidelines on stdout and stderr usage.

-- 
Cheers,
Ray Chuan

Re: [PATCH] fetch: don't output non-errors on stderr

[Ævar Arnfjörð Bjarmason]

On Sat, Jun 26, 2010 at 13:40, Tay Ray Chuan <rctay89@gmail.com> wrote:

> On Sat, Jun 26, 2010 at 8:14 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> But it sounds like we need some general discussion on what stdout and
>> stderr should be used for in Git with regards to progress messages,
>> errors and other similar things.
>
> I'd say - submit a patch for the style guide with your proposed
> guidelines on stdout and stderr usage.

I'm not sure how such a guide should be like for the general case, but
at least making commands internally consistent (as this patch does) is
one step in that direction.

Re: [PATCH] fetch: don't output non-errors on stderr

[Jeff King]

On Sat, Jun 26, 2010 at 12:14:59PM +0000, Ævar Arnfjörð Bjarmason wrote:

> On Sat, Jun 26, 2010 at 06:13, Jeff King <peff@peff.net> wrote:
> 
> > Or even easier: is there a reason that "git fetch -q" would not do what
> > you (Ævar) want?
> 
> That'd reduce the verbosity level, which'd skip some messages that I
> might want. E.g.:
> 
> 		if (verbosity >= 0) {
> 			fprintf(stderr, " x %-*s %-*s -> %s\n",
> 				TRANSPORT_SUMMARY_WIDTH, "[deleted]",
> 				REFCOL_WIDTH, "(none)", prettify_refname(ref->name));

Wait, what? Isn't that line also part of the same human-readable status
table? What makes the status of a pruned ref any different than the
status of an updated ref? I don't see how that is an error message, but
the other lines are not.

-Peff

Re: [PATCH] fetch: don't output non-errors on stderr

[Ævar Arnfjörð Bjarmason]

On Sat, Jun 26, 2010 at 16:46, Jeff King <peff@peff.net> wrote:
> On Sat, Jun 26, 2010 at 12:14:59PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> On Sat, Jun 26, 2010 at 06:13, Jeff King <peff@peff.net> wrote:
>>
>> > Or even easier: is there a reason that "git fetch -q" would not do what
>> > you (Ævar) want?
>>
>> That'd reduce the verbosity level, which'd skip some messages that I
>> might want. E.g.:
>>
>>               if (verbosity >= 0) {
>>                       fprintf(stderr, " x %-*s %-*s -> %s\n",
>>                               TRANSPORT_SUMMARY_WIDTH, "[deleted]",
>>                               REFCOL_WIDTH, "(none)", prettify_refname(ref->name));
>
> Wait, what? Isn't that line also part of the same human-readable status
> table? What makes the status of a pruned ref any different than the
> status of an updated ref? I don't see how that is an error message, but
> the other lines are not.

I misread that and picked the wrong example, sorry for the noise.