Bugreport: Git responds with stderr instead of stdout

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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

On Sun, Apr 25, 2010 at 11:06, Jack Desert [off-list ref] 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.

On Sun, Apr 25, 2010 at 18:10, Jacob Helwig [off-list ref] 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?

El Sun, 25 Apr 2010 11:10:47 -0700
Jacob Helwig [off-list ref] escribió:

On Sun, Apr 25, 2010 at 11:06, Jack Desert [off-list ref] wrote:

quoted

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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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 [off-list ref] wrote:

quoted

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

On Sun, Apr 25, 2010 at 19:22, Jeff King [off-list ref] wrote:

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

quoted

On Sun, Apr 25, 2010 at 18:10, Jacob Helwig [off-list ref] wrote:

quoted

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.

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

quoted

quoted

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

On Sun, Apr 25, 2010 at 19:32, Jeff King [off-list ref] wrote:

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

quoted

quoted

quoted

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), ...)

?

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 <redacted>
---

On Sat, Jun 12, 2010 at 16:52, Ævar Arnfjörð Bjarmason [off-list ref] 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

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 <redacted>
---

On Thu, Jun 24, 2010 at 22:34, Ævar Arnfjörð Bjarmason [off-list ref] 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