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

Jeff King [off-list ref] writes:

... Which means that the original porcelain
format was perhaps not very well thought-out.
...
now whether to fix it and break compatibility, or leave it broken...

I think the purpose of the patches that started this thread was to admit
that 1965ff7 (add --porcelain option to git-push, 2009-06-22) was not well
thought out, and to break compatibility to fix it.

Having said that, I would say that what 1965ff7 specified was only these
two:

    = TAB refs/heads/master:refs/heads/master TAB [up to date]
    - TAB :refs/heads/foobar TAB [deleted]

so everything else that do not match this pattern is a fair game, most
importantly, the line that begins with "To" would not be mistaken with
this pattern, I think.

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 do not care too much about this hunk either way.  We could leave it as
is, as we will be giving some other stuff to the standard error stream
without squelching anyway, even with the three-patch series.  We could
squelch only this message, but it is dubious what it is buying us.  If you
forced me to decide, I would probably say "let's just drop this hunk and
keep the code as-is".

As to the exit status, do you have any thoughts, by the way?

I am not convinced that it would be necessary nor even a good idea to make
it behave inconsistently between the normal case and Porcelain case, only
to make it easier to special case the "remote side would reject due to
non-fast-forward" failure mode (iow, even if the calling script knows that
it would fail due to non-fast-forward but otherwise everything else would
be fine, what good would it do?)

* Junio C Hamano (gitster@pobox.com) [100208 17:22]:

Jeff King [off-list ref] writes:

quoted

... Which means that the original porcelain
format was perhaps not very well thought-out.
...
now whether to fix it and break compatibility, or leave it broken...

I think the purpose of the patches that started this thread was to admit
that 1965ff7 (add --porcelain option to git-push, 2009-06-22) was not well
thought out, and to break compatibility to fix it.

Having said that, I would say that what 1965ff7 specified was only these
two:

    = TAB refs/heads/master:refs/heads/master TAB [up to date]
    - TAB :refs/heads/foobar TAB [deleted]

also these

X TAB ...
! TAB ...
* TAB ...
+ TAB ...
SPACE TAB ...

I'll respond to this message with another iteration of the patch.

     --larry

* send the "To prevent you from losing history, non-fast-forward...." message to
  the standard error.  That's where these sort of advice messages typically go

* for git push --porcelain: squelch the above advice.

* for git push --porcelain: send "To dest" lines to the standard output so
  whoever is reading from the process which ref updates went to which remotes.

* for git push --porcelain: only send the "Everything up-to-date" line if
  verbose.

* for git push --porcelain --dry-run: exit with status 0 even if updates will be
  rejected.  Whoever is reading the output of git push--dry-run --porcelain can
  clearly see if updates will be rejected.  However, it will probably need to
  distinguish this condition from other unknown errors that it does not know how
  to handle.

Signed-off-by: Larry D'Anna <redacted>
---
 builtin-push.c      |   11 ++++++++---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    7 ++++---
 4 files changed, 17 insertions(+), 6 deletions(-)

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

@@ -124,9 +124,9 @@ static int push_with_options(struct transport *transport, int flags)
 		return 0;
 
 	if (nonfastforward && advice_push_nonfastforward) {
-		printf("To prevent you from losing history, non-fast-forward updates were rejected\n"
-		       "Merge the remote changes before pushing again.  See the 'Note about\n"
-		       "fast-forwards' section of 'git push --help' for details.\n");
+		fprintf(stderr, "To prevent you from losing history, non-fast-forward updates were rejected\n"
+				"Merge the remote changes before pushing again.  See the 'Note about\n"
+				"fast-forwards' section of 'git push --help' for details.\n");
 	}
 
 	return 1;

@@ -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/builtin-send-pack.c b/builtin-send-pack.c
index 76c7206..358f5e1 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c

@@ -476,6 +476,10 @@ int send_pack(struct send_pack_args *args,
 
 	if (ret < 0)
 		return ret;
+
+	if (args->porcelain && args->dry_run)
+		return 0;
+
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:

diff --git a/send-pack.h b/send-pack.h
index 28141ac..60b4ba6 100644
--- a/send-pack.h
+++ b/send-pack.h

@@ -4,6 +4,7 @@
 struct send_pack_args {
 	unsigned verbose:1,
 		quiet:1,
+		porcelain:1,
 		send_mirror:1,
 		force_update:1,
 		use_thin_pack:1,

diff --git a/transport.c b/transport.c
index 3846aac..2b9e4be 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:

@@ -791,6 +791,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	args.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);
 	args.quiet = !!(flags & TRANSPORT_PUSH_QUIET);
 	args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
+	args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
 
 	ret = send_pack(&args, data->fd, data->conn, remote_refs,
 			&data->extra_have);

@@ -1052,7 +1053,7 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_FORCE);
 
 		ret = transport->push_refs(transport, remote_refs, flags);
-		err = push_had_errors(remote_refs);
+		err = (pretend && porcelain) ? 0 : push_had_errors(remote_refs);
 
 		ret |= err;
 

@@ -1070,7 +1071,7 @@ int transport_push(struct transport *transport,
 				update_tracking_ref(transport->remote, ref, verbose);
 		}
 
-		if (!quiet && !ret && !refs_pushed(remote_refs))
+		if (!quiet && (!porcelain || verbose) && !ret && !refs_pushed(remote_refs))
 			fprintf(stderr, "Everything up-to-date\n");
 		return ret;
 	}

-- 
1.7.0.rc1.33.g07cf0f.dirty

On Mon, Feb 08, 2010 at 02:21:26PM -0800, Junio C Hamano wrote:

I think the purpose of the patches that started this thread was to admit
that 1965ff7 (add --porcelain option to git-push, 2009-06-22) was not well
thought out, and to break compatibility to fix it.

I thought it was simply to remove stdout cruft that had crept in, and at
the same time to remove some stderr cruft that was simply noise on the
terminal. That being said, I am in favor of fixing it even if it means a
slight compatibility breakage.

Having said that, I would say that what 1965ff7 specified was only these
two:

    = TAB refs/heads/master:refs/heads/master TAB [up to date]
    - TAB :refs/heads/foobar TAB [deleted]

so everything else that do not match this pattern is a fair game, most
importantly, the line that begins with "To" would not be mistaken with
this pattern, I think.

It depends on the parser. If the parser was something like:

  switch (line[0]) {
    case '=': ...; break;
    case '-': ...; break;
    default: die("wtf: %s", line);
  }

then we are not introducing any ambiguity, but we are causing a
breakage. The problem is that we did not specify the format anywhere, so
it is hard to say whether we are breaking any promises we made.

quoted

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 do not care too much about this hunk either way.  We could leave it as
is, as we will be giving some other stuff to the standard error stream
without squelching anyway, even with the three-patch series.  We could
squelch only this message, but it is dubious what it is buying us.  If you
forced me to decide, I would probably say "let's just drop this hunk and
keep the code as-is".

I have not actually been running these patches, just reading them, but
my impression was the goal _was_ to squelch all of the stderr cruft. But
if we are not even close, then probably we should just give up and
callers should "2>/dev/null".

As to the exit status, do you have any thoughts, by the way?

I am not convinced that it would be necessary nor even a good idea to make
it behave inconsistently between the normal case and Porcelain case, only
to make it easier to special case the "remote side would reject due to
non-fast-forward" failure mode (iow, even if the calling script knows that
it would fail due to non-fast-forward but otherwise everything else would
be fine, what good would it do?)

I had initially endorsed it, but now I am having second thoughts.
Especially if the "usual" calling convention is to redirect stderr as
above, then we are probably missing out on any useful error messages
that accompany a failure return, anyway.  So maybe the sane thing to do
is to leave the exit code alone, and include a --porcelain output line
that either says "Everything was OK, see individual ref status" or "We
couldn't even talk to the other side". Then the status code is
irrelevant, and stdout contains all of the useful information (and if
you don't get an error or OK message, you know there was some
serious error like a broken git installation).

-Peff

* Jeff King (peff@peff.net) [100210 00:41]:

I have not actually been running these patches, just reading them, but
my impression was the goal _was_ to squelch all of the stderr cruft. But
if we are not even close, then probably we should just give up and
callers should "2>/dev/null".

Personally, I don't really care about squelching stderr cruft.  All I really
want is for what goes to stdout be sane and the calling script to be able to
unambiguously figure out what happened, including

* which refs go to which remotes 

* whether or not some mysterious error occurred (beyond those mentioned in the
  ref status lines)

I had initially endorsed it, but now I am having second thoughts.
Especially if the "usual" calling convention is to redirect stderr as
above, then we are probably missing out on any useful error messages
that accompany a failure return, anyway.  So maybe the sane thing to do
is to leave the exit code alone, and include a --porcelain output line
that either says "Everything was OK, see individual ref status" or "We
couldn't even talk to the other side". Then the status code is
irrelevant, and stdout contains all of the useful information (and if
you don't get an error or OK message, you know there was some
serious error like a broken git installation).

That serves my purposes as well as the exit code would.  Is this the consensus?

     --larry

Hi,

On Wed, Feb 10, 2010 at 1:55 PM, Larry D'Anna [off-list ref] wrote:

* Jeff King (peff@peff.net) [100210 00:41]:

quoted

I had initially endorsed it, but now I am having second thoughts.
Especially if the "usual" calling convention is to redirect stderr as
above, then we are probably missing out on any useful error messages
that accompany a failure return, anyway.  So maybe the sane thing to do
is to leave the exit code alone, and include a --porcelain output line
that either says "Everything was OK, see individual ref status" or "We
couldn't even talk to the other side". Then the status code is
irrelevant, and stdout contains all of the useful information (and if
you don't get an error or OK message, you know there was some
serious error like a broken git installation).

That serves my purposes as well as the exit code would.  Is this the consensus?

does this mean the ugly return status mangling patch (git-push: make
git push --dry-run --porcelain exit with status 0 even if updates will
be rejected) would be dropped? If so, I'm all for it. =p

-- 
Cheers,
Ray Chuan