Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain

I realize that I more-or-less repeated what I already said for the second
round, so let's try a different approach.

How about replacing the three patches with something like this?

-- >8 --
Subject: push --porcelain: usability updates

"git push --porcelain" is meant for Porcelain scripts to read from; there
is no reason to give advice messages meant for the user under that mode.

When reporting the update status, the name of the destination and
"Everything up-to-date" message were shown to the standard error stream,
but that is unfriendly when a Porcelain script is reading from us.  Send
them to the standard output to make it easier for them.

---
 builtin-push.c |    5 +++++
 transport.c    |    5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5633f0a..f5082d8 100644
--- a/builtin-push.c
+++ b/builtin-push.c

@@ -226,6 +226,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
+	if (flags & TRANSPORT_PUSH_PORCELAIN) {
+		/* Do not give advice messages to Porcelain scripts */
+		advice_push_nonfastforward = 0;
+	}
+
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
 		die("--delete is incompatible with --all, --mirror and --tags");
 	if (deleterefs && argc < 2)

diff --git a/transport.c b/transport.c
index 3846aac..0492934 100644
--- a/transport.c
+++ b/transport.c

@@ -675,7 +675,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
 static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)
 {
 	if (!count)
-		fprintf(stderr, "To %s\n", dest);
+		fprintf(porcelain ? stdout : stderr, "To %s\n", dest);
 
 	switch(ref->status) {
 	case REF_STATUS_NONE:

@@ -1071,7 +1071,8 @@ int transport_push(struct transport *transport,
 		}
 
 		if (!quiet && !ret && !refs_pushed(remote_refs))
-			fprintf(stderr, "Everything up-to-date\n");
+			fprintf(porcelain ? stdout : stderr,
+				"Everything up-to-date\n");
 		return ret;
 	}
 	return 1;

On Mon, Feb 08, 2010 at 01:13:36PM -0800, Junio C Hamano wrote:

I think this is sane.

 {
 	if (!count)
-		fprintf(stderr, "To %s\n", dest);
+		fprintf(porcelain ? stdout : stderr, "To %s\n", dest);

But note here that you are changing the --porcelain format, as callers
which were keeping only the stdout (and letting stderr go to /dev/null,
or spew to the user) saw only the ref lines. So this may be breaking
such callers.

I think you argued elsewhere (and I agree) that with multiple push urls,
this information is useful. Which means that the original porcelain
format was perhaps not very well thought-out. :( So we have to choose
now whether to fix it and break compatibility, or leave it broken. If
the former, then we should make sure there are not other design issues
in need of fixing, so we can just break compatibility _once_.

This one, on the other hand, seems to me to be just noise. What does a
--porcelain caller learn by seeing "Everything up-to-date" that it did
not already know from seeing the list of refs?

-Peff

* Jeff King (peff@peff.net) [100208 16:32]:

quoted

@@ -1071,7 +1071,8 @@ int transport_push(struct transport *transport,
 		}
 
 		if (!quiet && !ret && !refs_pushed(remote_refs))
-			fprintf(stderr, "Everything up-to-date\n");
+			fprintf(porcelain ? stdout : stderr,
+				"Everything up-to-date\n");
 		return ret;
 	}

This one, on the other hand, seems to me to be just noise. What does a
--porcelain caller learn by seeing "Everything up-to-date" that it did
not already know from seeing the list of refs?

I agree.  I don't see how sending this message to stdout could possibly help the
--porcelain caller.


            --larry