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

On Sun, Apr 25, 2010 at 11:06, Jack Desert <jackdesert556@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.

On Sun, Apr 25, 2010 at 18:10, Jacob Helwig <jacob.helwig@gmail.com> wrote:

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?

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

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

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 <peff@peff.net> wrote:

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:

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 <peff@peff.net> wrote:

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 <avarab@gmail.com>
---

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

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(-)

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:

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(-)

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

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

On Fri, Jun 25, 2010 at 17:25, Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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

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

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

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

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.

Hi,

On Sat, Jun 26, 2010 at 8:14 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

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

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.

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

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

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

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

El Sun, 25 Apr 2010 11:10:47 -0700 Jacob Helwig <jacob.helwig@gmail.com> escribió:

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