[PATCH] fix an error message in git-push so it goes to stderr

[PATCH] fix an error message in git-push so it goes to stderr

[Larry D'Anna]

Having it go to standard output interferes with git-push --porcelain.
---
 builtin-push.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5633f0a..0a27072 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;
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH] fix an error message in git-push so it goes to stderr

[Jeff King]

On Thu, Feb 04, 2010 at 07:41:40PM -0500, Larry D'Anna wrote:

> Having it go to standard output interferes with git-push --porcelain.
> ---
>  builtin-push.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin-push.c b/builtin-push.c
> index 5633f0a..0a27072 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");

I agree that stderr is a more sensible place for such a message to go,
but shouldn't the porcelain output format just suppress it entirely? The
whole point of it is to be machine readable, and this text is

  a. formatted for humans

  b. totally redundant with the machine-readable information presented
     earlier

-Peff

[PATCH 1/3] fix an error message in git-push so it goes to stderr

[Larry D'Anna]

Having it go to standard output interferes with git-push --porcelain.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5633f0a..0a27072 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;
-- 
1.7.0.rc1.33.g07cf0f.dirty

[PATCH 2/3] silence human readable info messages going to stderr from git push --porcelain

[Larry D'Anna]

These messages are redundant information to a script that's calling git-push.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    4 ++--
 transport.c    |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 0a27072..3fa4516 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -111,7 +111,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (thin)
 		transport_set_option(transport, TRANS_OPT_THIN, "yes");
 
-	if (flags & TRANSPORT_PUSH_VERBOSE)
+	if (flags & TRANSPORT_PUSH_VERBOSE && !(flags & TRANSPORT_PUSH_PORCELAIN))
 		fprintf(stderr, "Pushing to %s\n", transport->url);
 	err = transport_push(transport, refspec_nr, refspec, flags,
 			     &nonfastforward);
@@ -123,7 +123,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (!err)
 		return 0;
 
-	if (nonfastforward && advice_push_nonfastforward) {
+	if (!(flags & TRANSPORT_PUSH_PORCELAIN) && nonfastforward && advice_push_nonfastforward) {
 		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");
diff --git a/transport.c b/transport.c
index 3846aac..f707c7b 100644
--- a/transport.c
+++ b/transport.c
@@ -674,7 +674,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)
+	if (!count && !porcelain)
 		fprintf(stderr, "To %s\n", dest);
 
 	switch(ref->status) {
@@ -1067,10 +1067,10 @@ int transport_push(struct transport *transport,
 		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
 			struct ref *ref;
 			for (ref = remote_refs; ref; ref = ref->next)
-				update_tracking_ref(transport->remote, ref, verbose);
+				update_tracking_ref(transport->remote, ref, verbose && !porcelain);
 		}
 
-		if (!quiet && !ret && !refs_pushed(remote_refs))
+		if (!quiet && !porcelain && !ret && !refs_pushed(remote_refs))
 			fprintf(stderr, "Everything up-to-date\n");
 		return ret;
 	}
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH 2/3] silence human readable info messages going to stderr from git push --porcelain

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> These messages are redundant information to a script that's calling git-push.

Redundant is not a reason for a change; unwanted would be.  And some of
the messages you are trying to squelch indeed look like unwanted ones, but
not all of them.

> -	if (flags & TRANSPORT_PUSH_VERBOSE)
> +	if (flags & TRANSPORT_PUSH_VERBOSE && !(flags & TRANSPORT_PUSH_PORCELAIN))
>  		fprintf(stderr, "Pushing to %s\n", transport->url);

Why should you be forbidden to expect "--porcelain -v" to give you this
message?

> @@ -123,7 +123,7 @@ static int push_with_options(struct transport *transport, int flags)
>  	if (!err)
>  		return 0;
>  
> -	if (nonfastforward && advice_push_nonfastforward) {
> +	if (!(flags & TRANSPORT_PUSH_PORCELAIN) && nonfastforward && advice_push_nonfastforward) {
>  		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");

This probably is a good change; the long lines are unsightly but that is a
separate topic.

> diff --git a/transport.c b/transport.c
> index 3846aac..f707c7b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -674,7 +674,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)
> +	if (!count && !porcelain)
>  		fprintf(stderr, "To %s\n", dest);

I don't think this is correct.

If you have more than one remote.there.pushURL, the calling Porcelain
script of "git push --porcelain there" should be able to tell which
destination the following report is about, and without this line you
cannot tell.

I would understand if this change were to make the message go to the
standard output when operating with --porcelain option, though.

> @@ -1067,10 +1067,10 @@ int transport_push(struct transport *transport,
>  		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
>  			struct ref *ref;
>  			for (ref = remote_refs; ref; ref = ref->next)
> -				update_tracking_ref(transport->remote, ref, verbose);
> +				update_tracking_ref(transport->remote, ref, verbose && !porcelain);

Again, why  should you be forbidden to expect "--porcelain -v" to work?

> -		if (!quiet && !ret && !refs_pushed(remote_refs))
> +		if (!quiet && !porcelain && !ret && !refs_pushed(remote_refs))
>  			fprintf(stderr, "Everything up-to-date\n");

This is a borderline.  If you are truly up-to-date, the calling script
won't get anything.  It may be easier for Porcelain scripts to see this
message on the standard output as an explicit succeses report instead.

Re: [PATCH 2/3] silence human readable info messages going to stderr from git push --porcelain

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100205 15:20]:
> > -		if (!quiet && !ret && !refs_pushed(remote_refs))
> > +		if (!quiet && !porcelain && !ret && !refs_pushed(remote_refs))
> >  			fprintf(stderr, "Everything up-to-date\n");
> 
> This is a borderline.  If you are truly up-to-date, the calling script
> won't get anything.  It may be easier for Porcelain scripts to see this
> message on the standard output as an explicit succeses report instead.

how about this?

if (!quiet && (!porcelain || verbose) && !ret && !refs_pushed(remote_refs))
   fprintf(stderr, "Everything up-to-date\n");     

   --larry



   

[PATCH v2 0/3]

[Larry D'Anna]

I've fixed his series according to Jeff' and Junio's comments.

Larry D'Anna (3):
  fix an error message in git-push so it goes to stderr
  clean up some of the output from git push --porcelain
  make git push --dry-run --porcelain exit with status 0 even if
    updates will be rejected

 builtin-push.c      |    8 ++++----
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |   11 +++++++----
 4 files changed, 16 insertions(+), 8 deletions(-)

[PATCH v2 1/3] fix an error message in git-push so it goes to stderr

[Larry D'Anna]

Having it go to standard output interferes with git-push --porcelain.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5633f0a..0a27072 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;
-- 
1.7.0.rc1.33.g07cf0f.dirty

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

[Larry D'Anna]

* don't emit long explanatory message about non-fast-forward updates.

* send "To dest" lines to standard out so whoever is reading standard out knows
  which ref updates went to which remotes.

* only send the "Everything up-to-date" line if verbose.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    2 +-
 transport.c    |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 0a27072..ff0b1c6 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -123,7 +123,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (!err)
 		return 0;
 
-	if (nonfastforward && advice_push_nonfastforward) {
+	if (!(flags & TRANSPORT_PUSH_PORCELAIN) && nonfastforward && advice_push_nonfastforward) {
 		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");
diff --git a/transport.c b/transport.c
index 3846aac..00d986c 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:
@@ -1070,7 +1070,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

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

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> diff --git a/builtin-push.c b/builtin-push.c
> index 0a27072..ff0b1c6 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -123,7 +123,7 @@ static int push_with_options(struct transport *transport, int flags)
>  	if (!err)
>  		return 0;
>  
> -	if (nonfastforward && advice_push_nonfastforward) {
> +	if (!(flags & TRANSPORT_PUSH_PORCELAIN) && nonfastforward && advice_push_nonfastforward) {

I suspect that it would be a much nicer solution if you turned advice_*
off in very early parts of the codepath after you read configuration and
command line options to see if --porcelain was asked for.  That way, we
don't have to add code check for PORCELAIN next time we add code for new
kinds of advice.  People who are adding new advice_* would grep for
existing one (e.g. "advice_push_nonfastforward") and will notice this
line, but if you had

	if (flags & TRANSPORT_PUSH_PORDCELAIN) {
        	advice_push_nonfastforward = 0;
	}

in early parts of the code, they will notice that as well, and then they
know where to add the corresponding change necessary.

[PATCH v2 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

The script calling git push --dry-run --porcelain can see clearly from the
output that the updates will be rejected.  However, it will probably need to
distinguish this condition from the push failing for other reasons, such as the
remote not being reachable.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    7 +++++--
 3 files changed, 10 insertions(+), 2 deletions(-)

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 00d986c..b41e1dc 100644
--- a/transport.c
+++ b/transport.c
@@ -558,8 +558,10 @@ static int fetch_refs_via_pack(struct transport *transport,
 	return (refs ? 0 : -1);
 }
 
-static int push_had_errors(struct ref *ref)
+static int push_had_errors(struct ref *ref, int flags)
 {
+	if (flags & TRANSPORT_PUSH_DRY_RUN && flags & TRANSPORT_PUSH_PORCELAIN)
+		return 0;
 	for (; ref; ref = ref->next) {
 		switch (ref->status) {
 		case REF_STATUS_NONE:
@@ -791,6 +793,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 +1055,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 = push_had_errors(remote_refs, flags);
 
 		ret |= err;
 
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH v2 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Tay Ray Chuan]

Hi,

instead of:

> diff --git a/transport.c b/transport.c
> index 00d986c..b41e1dc 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -558,8 +558,10 @@ static int fetch_refs_via_pack(struct transport *transport,
>  	return (refs ? 0 : -1);
>  }
>  
> -static int push_had_errors(struct ref *ref)
> +static int push_had_errors(struct ref *ref, int flags)
>  {
> +	if (flags & TRANSPORT_PUSH_DRY_RUN && flags & TRANSPORT_PUSH_PORCELAIN)
> +		return 0;
>  	for (; ref; ref = ref->next) {
>  		switch (ref->status) {
>  		case REF_STATUS_NONE:

and:

> @@ -1052,7 +1055,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 = push_had_errors(remote_refs, flags);
>  
>  		ret |= err;
>  

why not:

-->8--
@@ -1049,7 +1052,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;

-->8--

-- 
Cheers,
Ray Chuan

Re: [PATCH v2 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

good idea.  

     --larry

[PATCH v3 1/3] git-push: fix an error message so it goes to stderr

[Larry D'Anna]

Having it go to standard output interferes with git-push --porcelain.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5633f0a..0a27072 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;
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH v3 1/3] git-push: fix an error message so it goes to stderr

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> Having it go to standard output interferes with git-push --porcelain.

Could you at least update this rationale?

You will be squelching this in [2/3] when --porcelain is used, so the
above is no longer a good justification.  It won't interfere at all.

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

[Larry D'Anna]

* don't emit long explanatory message about non-fast-forward updates.

* send "To dest" lines to standard out so whoever is reading standard out knows
  which ref updates went to which remotes.

* only send the "Everything up-to-date" line if verbose.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    2 +-
 transport.c    |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 0a27072..ff0b1c6 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -123,7 +123,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (!err)
 		return 0;
 
-	if (nonfastforward && advice_push_nonfastforward) {
+	if (!(flags & TRANSPORT_PUSH_PORCELAIN) && nonfastforward && advice_push_nonfastforward) {
 		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");
diff --git a/transport.c b/transport.c
index 3846aac..00d986c 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:
@@ -1070,7 +1070,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

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

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> * don't emit long explanatory message about non-fast-forward updates.

This makes sense as a goal, but at the same time as an implementation it
would be cleaner to flip "advice" off under --porcelain, instead of doing
"s/if (advice_blah)/if (advice_blah && !porcelain)/;".

This is doubly important if you consider longer term maintainability.  I
do not want to see the next person who tries to add new advice messages to
copy and paste the long if() statement you added in this patch.

> * send "To dest" lines to standard out so whoever is reading standard
> out knows which ref updates went to which remotes.

Makes sense. s/standard out/the standard output/, and
s/reading .*knows/reading from the process knows/, perhaps.

> * only send the "Everything up-to-date" line if verbose.

Don't you want to send this also to stdout?

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

[Junio C Hamano]

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;

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

[Jeff King]

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

> 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;
> +	}

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

> @@ -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?

-Peff

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

[Larry D'Anna]

* Jeff King (peff@peff.net) [100208 16:32]:
> > @@ -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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

>> @@ -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?)

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

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100208 17:22]:
> Jeff King <peff@peff.net> 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]


also these

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

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

     --larry

[PATCH] git-push: clean up some of the output from git push

[Larry D'Anna]

* 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 <larry@elder-gods.org>
---
 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

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

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> also these
>
> X TAB ...
> ! TAB ...
> * TAB ...
> + TAB ...
> SPACE TAB ...

Is this documented anywhere?

Much of what your patches in this thread tried to do does not interest me
very much rignt now, i.e. during the pre-release freeze period [*1*].

If you are already showing '*' and '+' without telling the users in the
documentation, it is a much more urgent issue that needs fixing, and it
would be preferrable if we can do so before the release.

Documentation/git-push.txt seems to say that only ' ', '!' and '=' have
any defined meaning:

    flag::
            A single character indicating the status of the ref. This is
            blank for a successfully pushed ref, `!` for a ref that was
            rejected or failed to push, and '=' for a ref that was up to
            date and did not need pushing (note that the status of up to
            date refs is shown only when `git push` is running verbosely).


[Footnote]

*1* As I hinted repeatedly, I think many of them are mere churn, except
for "don't advice porcelain scripts" (good) and perhaps "exit with failure
status upon only this kind of failure" (I am undecided).  Especially I
want to see the "send advice to the standard error stream" one as a
separate patch, if you insist including that change in the series.

But they are something we can decide after 1.7.0 happens.

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

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100208 17:48]:
> Larry D'Anna <larry@elder-gods.org> writes:
> 
> > also these
> >
> > X TAB ...
> > ! TAB ...
> > * TAB ...
> > + TAB ...
> > SPACE TAB ...

Actually it won't to 'X'.  print_one_push_status has a case for 'X', but it
never gets called.  

> Is this documented anywhere?

aparnetly not.  patch to follow.

          --larry

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

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

>> Is this documented anywhere?
>
> aparnetly not.  patch to follow.

Thanks.

[PATCH] git-push: fix the documentation to explain all the status flags

[Larry D'Anna]

git-push prints a flag indicating the status of each ref that was pushed, or
attempted to be pushed.  The documentation did not correctly list all of the
possible flags.
---
 Documentation/git-push.txt |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 73a921c..61b6894 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -176,12 +176,17 @@ If --porcelain is used, then each line of the output is of the form:
  <flag> \t <from>:<to> \t <summary> (<reason>)
 -------------------------------
 
+The status of up to date refs is shown only if --porcelain or --verbose is used.
+
 flag::
 	A single character indicating the status of the ref. This is
-	blank for a successfully pushed ref, `!` for a ref that was
-	rejected or failed to push, and '=' for a ref that was up to
-	date and did not need pushing (note that the status of up to
-	date refs is shown only when `git push` is running verbosely).
+[horizontal]
+	" " (space)::: for a successfully pushed fast-forward
+	"+"::: for a successful forced update
+	"-"::: for a successfully deleted ref
+	"*"::: for a successfully pushed new ref
+	"!"::: for a ref that was rejected or failed to push
+	"="::: for a ref that was up to date and did not need pushing
 
 summary::
 	For a successfully pushed ref, the summary shows the old and new
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH] git-push: fix the documentation to explain all the status flags

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> git-push prints a flag indicating the status of each ref that was pushed, or
> attempted to be pushed.  The documentation did not correctly list all of the
> possible flags.
> ---

Thanks.

I assume it is Ok to copy your sign-off from your other messages ;-)

> @@ -176,12 +176,17 @@ If --porcelain is used, then each line of the output is of the form:
>   <flag> \t <from>:<to> \t <summary> (<reason>)
>  -------------------------------
>  
> +The status of up to date refs is shown only if --porcelain or --verbose is used.
> +
>  flag::
>  	A single character indicating the status of the ref. This is
> -	blank for a successfully pushed ref, `!` for a ref that was
> -	rejected or failed to push, and '=' for a ref that was up to
> -	date and did not need pushing (note that the status of up to
> -	date refs is shown only when `git push` is running verbosely).
> +[horizontal]
> +	" " (space)::: for a successfully pushed fast-forward

Both [horizontal] and three colons are something we never have used in the
existing documentation set.  How confident are you that various versions
of deployed AsciiDoc people would use all support this?

I am not _complaining_; I am just being cautious to see if I have to look
into the issue myself (if your answer is "not at all") or not (otherwise).

> +	"+"::: for a successful forced update
> +	"-"::: for a successfully deleted ref
> +	"*"::: for a successfully pushed new ref
> +	"!"::: for a ref that was rejected or failed to push
> +	"="::: for a ref that was up to date and did not need pushing

Re: [PATCH] git-push: fix the documentation to explain all the status flags

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Both [horizontal] and three colons are something we never have used in the
> existing documentation set.  How confident are you that various versions
> of deployed AsciiDoc people would use all support this?
>
> I am not _complaining_; I am just being cautious to see if I have to look
> into the issue myself (if your answer is "not at all") or not (otherwise).

Unfortunately, http://www.methods.co.nz/asciidoc/newlists.html labels the
syntax as "new in 8.3.0", and Debian 5.0.X and Fedora 11 both use AsciiDoc
that are older than that (8.2.7 and 8.2.5).

I'll flatten the description into prose for now.

Thanks.

Re: [PATCH] git-push: fix the documentation to explain all the status flags

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Unfortunately, http://www.methods.co.nz/asciidoc/newlists.html labels the
> syntax as "new in 8.3.0", and Debian 5.0.X and Fedora 11 both use AsciiDoc
> that are older than that (8.2.7 and 8.2.5).
>
> I'll flatten the description into prose for now.

Actually, we  already use another syntax for nested enumerations, namely
double-semicolon, in config.txt, so let's mimic that.

-- >8 --
Subject: git-push: document all the status flags used by --porcelain output 

We didn't talk about '-' (deletion), '*' (addition), nor '+' (forced).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-push.txt |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 73a921c..bd79119 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -176,12 +176,17 @@ If --porcelain is used, then each line of the output is of the form:
  <flag> \t <from>:<to> \t <summary> (<reason>)
 -------------------------------
 
+The status of up-to-date refs is shown only if --porcelain or --verbose
+option is used.
+
 flag::
-	A single character indicating the status of the ref. This is
-	blank for a successfully pushed ref, `!` for a ref that was
-	rejected or failed to push, and '=' for a ref that was up to
-	date and did not need pushing (note that the status of up to
-	date refs is shown only when `git push` is running verbosely).
+	A single character indicating the status of the ref:
+(space);; for a successfully pushed fast-forward;
+`{plus}`;; for a successful forced update;
+`-`;; for a successfully deleted ref;
+`*`;; for a successfully pushed new ref;
+`!`;; for a ref that was rejected or failed to push; and
+`=`;; for a ref that was up to date and did not need pushing.
 
 summary::
 	For a successfully pushed ref, the summary shows the old and new

Re: [PATCH] git-push: fix the documentation to explain all the status flags

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100208 19:45]:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Unfortunately, http://www.methods.co.nz/asciidoc/newlists.html labels the
> > syntax as "new in 8.3.0", and Debian 5.0.X and Fedora 11 both use AsciiDoc
> > that are older than that (8.2.7 and 8.2.5).
> >
> > I'll flatten the description into prose for now.
> 
> Actually, we  already use another syntax for nested enumerations, namely
> double-semicolon, in config.txt, so let's mimic that.
> 
> -- >8 --
> Subject: git-push: document all the status flags used by --porcelain output 

the flags are used by normal git-push as well

    --larry

Re: [PATCH] git-push: fix the documentation to explain all the status flags

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

>> Subject: git-push: document all the status flags used by --porcelain output 
>
> the flags are used by normal git-push as well

I noticed it after I sent it out and reworded "s/by.*/in the output/".
Thanks for being careful.

Re: [PATCH] git-push: fix the documentation to explain all the status flags

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100208 19:24]:
> Both [horizontal] and three colons are something we never have used in the
> existing documentation set.  How confident are you that various versions
> of deployed AsciiDoc people would use all support this?
> 
> I am not _complaining_; I am just being cautious to see if I have to look
> into the issue myself (if your answer is "not at all") or not (otherwise).

Apparently three colons is really new, its only in asciidoc 8.5.3, released this
January.  [horizontal] goes back to 8.3.0, from  2008.

          --arry

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

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100208 17:48]:
> *1* As I hinted repeatedly, I think many of them are mere churn, except
> for "don't advice porcelain scripts" (good) and perhaps "exit with failure
> status upon only this kind of failure" (I am undecided).  

What about the "To" lines?  It seems that they really should go to stdout (if
--porcelain is selected).  Otherwise, how does the reader of stdout know which
refs got pushed to which remote?

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

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> * Junio C Hamano (gitster@pobox.com) [100208 17:48]:
>> *1* As I hinted repeatedly, I think many of them are mere churn, except
>> for "don't advice porcelain scripts" (good) and perhaps "exit with failure
>> status upon only this kind of failure" (I am undecided).  
>
> What about the "To" lines?  It seems that they really should go to stdout (if
> --porcelain is selected).  Otherwise, how does the reader of stdout know which
> refs got pushed to which remote?

I think that probably is an improvement.

We would probably need documentation updates to cover the parsing of "To",
at least; it might be cleaner to have a separate section for porcelain
writers.  And tests.  But this series won't hit next until 1.7.0 ships so
we have enough time to polish the details.

I queued the series and once merged it to 'pu' but seeing some tests fail,
I reverted the merge.  The error is somewhat curious...

$ while sh t5401-*.sh -i; do :; done
... wait for a while ...
* FAIL 12: send-pack stderr contains hook messages

                grep ^remote: send.err | sed "s/ *\$//" >actual &&
                test_cmp - actual <expect

$ t/(364643e...); cat trash\ directory.t5401-update-hooks/actual 
remote: STDOUT pre-receive
remote: STDERR pre-receive
remote: STDOUT update refs/heads/master
remote: STDERR update refs/heads/master
remote: STDOUT update refs/heads/tofail
remote: STDOUT post-receive
remote: STDERR post-receive
remote: STDOUT post-update
remote: STDERR post-update
$ t/(364643e...); cat trash\ directory.t5401-update-hooks/expect 
remote: STDOUT pre-receive
remote: STDERR pre-receive
remote: STDOUT update refs/heads/master
remote: STDERR update refs/heads/master
remote: STDOUT update refs/heads/tofail
remote: STDERR update refs/heads/tofail
remote: STDOUT post-receive
remote: STDERR post-receive
remote: STDOUT post-update
remote: STDERR post-update

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

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100209 02:31]:
> Larry D'Anna <larry@elder-gods.org> writes:
> 
> > * Junio C Hamano (gitster@pobox.com) [100208 17:48]:
> >> *1* As I hinted repeatedly, I think many of them are mere churn, except
> >> for "don't advice porcelain scripts" (good) and perhaps "exit with failure
> >> status upon only this kind of failure" (I am undecided).  
> >
> > What about the "To" lines?  It seems that they really should go to stdout (if
> > --porcelain is selected).  Otherwise, how does the reader of stdout know which
> > refs got pushed to which remote?
> 
> I think that probably is an improvement.
> 
> We would probably need documentation updates to cover the parsing of "To",
> at least; it might be cleaner to have a separate section for porcelain
> writers.  And tests.  But this series won't hit next until 1.7.0 ships so
> we have enough time to polish the details.
> 
> I queued the series and once merged it to 'pu' but seeing some tests fail,
> I reverted the merge.  The error is somewhat curious...

I bisected, and this was introduced by  

6d525d3 receive-pack: Send hook output over side band #2

  --larry

Re: t5401-update-hooks test failure

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> $ while sh t5401-*.sh -i; do :; done
> ... wait for a while ...
> * FAIL 12: send-pack stderr contains hook messages
> 
>                 grep ^remote: send.err | sed "s/ *\$//" >actual &&
>                 test_cmp - actual <expect
> 
> $ t/(364643e...); cat trash\ directory.t5401-update-hooks/actual 
> remote: STDOUT pre-receive
> remote: STDERR pre-receive
> remote: STDOUT update refs/heads/master
> remote: STDERR update refs/heads/master
> remote: STDOUT update refs/heads/tofail
> remote: STDOUT post-receive
> remote: STDERR post-receive
> remote: STDOUT post-update
> remote: STDERR post-update
> $ t/(364643e...); cat trash\ directory.t5401-update-hooks/expect 
> remote: STDOUT pre-receive
> remote: STDERR pre-receive
> remote: STDOUT update refs/heads/master
> remote: STDERR update refs/heads/master
> remote: STDOUT update refs/heads/tofail
> remote: STDERR update refs/heads/tofail
> remote: STDOUT post-receive
> remote: STDERR post-receive
> remote: STDOUT post-update
> remote: STDERR post-update

A quick visual inspection shows that only the STDERR tofail message
is missing here.  That sounds to me like a race condition in the
recv_sideband decoder.  Or, a race condition in the hook code in
builtin-receive-pack.c.

I doubt its in receive-pack. run_update_hook() directly calls the
copy_to_sideband() function, and that reads until EOF on the hook's
stderr stream before it returns and waits for the hook's exit status.
So we should be pulling everything and dumping it into the sideband.

builtin-send-pack.c clearly isn't stopping early while processing
the stream, since we see later messages from the post-receive and
post-update hooks just fine.

So I think the only code that is in question is the case 2 arm of
recv_sideband().  But to be honest, I can't find any fault with it.


I've been running this test in a while loop for a while now, and
I can't make it trigger this failure.  Maybe its possible that
its the update hook itself, not flushing its stderr buffer before
it terminates?

-- 
Shawn.

Re: t5401-update-hooks test failure

[Nicolas Pitre]

On Tue, 9 Feb 2010, Shawn O. Pearce wrote:

> Junio C Hamano <gitster@pobox.com> wrote:
> > $ while sh t5401-*.sh -i; do :; done
> > ... wait for a while ...
> > * FAIL 12: send-pack stderr contains hook messages
> > 
> >                 grep ^remote: send.err | sed "s/ *\$//" >actual &&
> >                 test_cmp - actual <expect
> > 
> > $ t/(364643e...); cat trash\ directory.t5401-update-hooks/actual 
> > remote: STDOUT pre-receive
> > remote: STDERR pre-receive
> > remote: STDOUT update refs/heads/master
> > remote: STDERR update refs/heads/master
> > remote: STDOUT update refs/heads/tofail
> > remote: STDOUT post-receive
> > remote: STDERR post-receive
> > remote: STDOUT post-update
> > remote: STDERR post-update
> > $ t/(364643e...); cat trash\ directory.t5401-update-hooks/expect 
> > remote: STDOUT pre-receive
> > remote: STDERR pre-receive
> > remote: STDOUT update refs/heads/master
> > remote: STDERR update refs/heads/master
> > remote: STDOUT update refs/heads/tofail
> > remote: STDERR update refs/heads/tofail
> > remote: STDOUT post-receive
> > remote: STDERR post-receive
> > remote: STDOUT post-update
> > remote: STDERR post-update
> 
> A quick visual inspection shows that only the STDERR tofail message
> is missing here.  That sounds to me like a race condition in the
> recv_sideband decoder.  Or, a race condition in the hook code in
> builtin-receive-pack.c.
> 
> I doubt its in receive-pack. run_update_hook() directly calls the
> copy_to_sideband() function, and that reads until EOF on the hook's
> stderr stream before it returns and waits for the hook's exit status.
> So we should be pulling everything and dumping it into the sideband.
> 
> builtin-send-pack.c clearly isn't stopping early while processing
> the stream, since we see later messages from the post-receive and
> post-update hooks just fine.
> 
> So I think the only code that is in question is the case 2 arm of
> recv_sideband().  But to be honest, I can't find any fault with it.

Note that strict order of messages passed through the sideband can't be 
relied upon.  Often you have sideband 1 connected to stdin and sideband 
2 connected to stderr, and they are linked with pipes, and various 
factors such as stdio buffering or even printf implementation in the C 
lib, pipe buffers in the OS, random scheduling between the processes 
involved, and other factors such as disk or network speed vs CPU speed, 
are all adding to a certain degree of randomness affecting how the data 
end up in various channels wrt each other.

See my efforts to reduce that issue so far with those commits:

6b59f51b312f06d9420d34c09fa408c658aac6d2

6b9c42b4daabe3d0471d9f90d2ae472c9d994e1c

d048a96ee9bec968be0bdc9c43ffce61169545be

ebe8fa738dcf6911fe520adce0cfa0cb26dee5e2


Nicolas

Re: t5401-update-hooks test failure

[Shawn O. Pearce]

Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 9 Feb 2010, Shawn O. Pearce wrote:
> > 
> > builtin-send-pack.c clearly isn't stopping early while processing
> > the stream, since we see later messages from the post-receive and
> > post-update hooks just fine.
> > 
> > So I think the only code that is in question is the case 2 arm of
> > recv_sideband().  But to be honest, I can't find any fault with it.
> 
> Note that strict order of messages passed through the sideband can't be 
> relied upon.  Often you have sideband 1 connected to stdin and sideband 
> 2 connected to stderr,

Oh.  Sure.  But that isn't really the case here.

The messages are all coming down side band #2, before we write
anything down side band #1.  The missing message in question
should have appeared somewhere in the middle of that side band
#2 stream that we are seeing in the test output.  Given that its
all serialized down into a single stream by the parent process
receive-pack, we really shouldn't see the messages out of order.


> and they are linked with pipes, and various 
> factors such as stdio buffering or even printf implementation in the C 
> lib

The only way I can see this missing message happening is if the C
library isn't flushing the stdio buffer before the hook process
exits.  Given that the hook process is a /bin/sh shell script,
and its using echo to print its messages... I'm at a loss for how
to fix that in Git.

Unless its the recv_sideband() somehow skipping a line.  But I
can't see it doing that.

-- 
Shawn.

Re: t5401-update-hooks test failure

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> The only way I can see this missing message happening is if the C
> library isn't flushing the stdio buffer before the hook process
> exits.  Given that the hook process is a /bin/sh shell script,
> and its using echo to print its messages... I'm at a loss for how
> to fix that in Git.
>
> Unless its the recv_sideband() somehow skipping a line.  But I
> can't see it doing that.

The detection method of test is fooled by intermixed message.

This is what send.err has, and you grep for '^remote:' in it.

-- >8 --
warning: updating the current branch
warning: Updating the currently cheremote: STDERR pre-receive        
,
warning: as the index and work tree do not reflect changes that are in HEAD.
warning: As a result, you may see the changes you just pushed into it
warning: reverted when you run 'git diff' over there, and you may want
warning: to run 'git reset --hard' before starting to work to recover.
warning: 
warning: You can set 'receive.denyCurrentBranch' configuration variable to
warning: 'refuse' in the remote repository to forbid pushing into its
warning: current branch.
warning: To allow pushing into the current branch, you can set it to 'ignore';
warning: but this is not recommended unless you arranged to update its work
warning: tree to match what you pushed in some other way.
warning: 
warning: To squelch this message, you can set it to 'warn'.
warning: 
warning: Note that the default will change in a future version of git
warning: to refuse updating the current branch unless you have the
warning: configuration variable set to either 'ignore' or 'warn'.
remote: STDOUT update refs/heads/master        
remote: STDERR update refs/heads/master        
remote: STDOUT update refs/heads/tofail        
remote: STDERR update refs/heads/tofail        
error: hook declined to update refs/heads/tofail
remote: STDOUT post-receive        
remote: STDERR post-receive        
remote: STDOUT post-update        
remote: STDERR post-update        
To ./victim/.git
   d4f8e4a..b07e5b8  master -> master
 ! [remote rejected] tofail -> tofail (hook declined)
-- 8< --

But there indeed _is_ some skipping.  "Updating the currently che"
is interrupted by the output from the pre-receive hook, and I do not see
the remainder "cked out branch may cause confusion,\n" anywhere.

Re: t5401-update-hooks test failure

[Junio C Hamano]

Oh, I hate myself ;-)

This seems to fix the test.  Could somebody please explain why?

I am guessing it has something to do with O_APPEND but I cannot come up
with a commit log message that clearly describes the fix.

 t/t5401-update-hooks.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index c3cf397..804431a 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -61,8 +61,9 @@ EOF
 chmod u+x victim/.git/hooks/post-update
 
 test_expect_success push '
+	rm -f send.out send.err &&
 	test_must_fail git send-pack --force ./victim/.git \
-		master tofail >send.out 2>send.err
+		master tofail >>send.out 2>>send.err
 '
 
 test_expect_success 'updated as expected' '

Re: t5401-update-hooks test failure

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > The only way I can see this missing message happening is if the C
> > library isn't flushing the stdio buffer before the hook process
> > exits.  Given that the hook process is a /bin/sh shell script,
> > and its using echo to print its messages... I'm at a loss for how
> > to fix that in Git.
> >
> > Unless its the recv_sideband() somehow skipping a line.  But I
> > can't see it doing that.
> 
> The detection method of test is fooled by intermixed message.
> 
> This is what send.err has, and you grep for '^remote:' in it.
> 
> -- >8 --
> warning: updating the current branch
> warning: Updating the currently cheremote: STDERR pre-receive        
> ,
..
> But there indeed _is_ some skipping.  "Updating the currently che"
> is interrupted by the output from the pre-receive hook, and I do not see
> the remainder "cked out branch may cause confusion,\n" anywhere.

Uh.  I got the problem now, thanks.

What's going on is, other messages inside of builtin-receive-pack
are being sent to stderr, while hook output is going over the
multiplexed side-band through stdout, where its parsed and written
to stderr by send-pack.

What I missed in my patch was changing all of these other messages
inside of receive-pack to also go over the side-band #2 if we have
use_sideband enabled.

Patch coming in a few minutes.

-- 
Shawn.

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

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100208 17:48]:
> *1* As I hinted repeatedly, I think many of them are mere churn, except
> for "don't advice porcelain scripts" (good) and perhaps "exit with failure
> status upon only this kind of failure" (I am undecided).  Especially I
> want to see the "send advice to the standard error stream" one as a
> separate patch, if you insist including that change in the series.

For some reason I thought you wanted the series squashed into one patch.  Here
it is again (to follow) with each discrete change as a single patch, and without
messing with "Everything up-to-date"

        --larry

[PATCH 1/4] git-push: fix an error message so it goes to stderr

[Larry D'Anna]

These sort of messages typically go to the standard error.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 5633f0a..0a27072 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;
-- 
1.7.0.rc1.33.g07cf0f.dirty

[PATCH 2/4] git-push: squelch advice message if in --porcelain mode

[Larry D'Anna]

By default, git-push may give the user a verbose advice message if a ref is
rejected for not being a fast-forward.  This patch squelches that message for
--porcelain mode.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-push.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 0a27072..aacba45 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)
-- 
1.7.0.rc1.33.g07cf0f.dirty

[PATCH 3/4] git-push: send "To <remoteurl>" messages to the standard output in --porcelain mode

[Larry D'Anna]

git-push prints the line "To <remoteurl>" before above each of the ref status
lines.  In --porcelain mode, these "To <remoteurl>" lines go to the standard
error, but the ref status lines go to the standard output.  This makes it
difficult for the process reading standard output to know which ref status lines
correspond to which remote.  This patch sends the "To <remoteurl>" lines to the
the standard output instead.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index 3846aac..fb653c6 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:
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH 3/4] git-push: send "To <remoteurl>" messages to the standard output in --porcelain mode

[Tay Ray Chuan]

Hi,

On Tue, Feb 9, 2010 at 1:54 PM, Larry D'Anna <larry@elder-gods.org> wrote:
> diff --git a/transport.c b/transport.c
> index 3846aac..fb653c6 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:

just to check - are debug and user-specific messages (like advice and
info) to be sent, by convention/design/force-of-habit, to stderr? Are
those the "cruft" that has been referred to in other threads?

If that's the case, then perhaps the "To: <destination>" lines should
be sent to both stdout and stderr - stdout, for porcelain scripts as a
"header", and stderr, to help the user make sense of any errors that
occur ("oh, these errors were triggered when pushing to so-and-so
remote").

-- 
Cheers,
Ray Chuan

Re: [PATCH 3/4] git-push: send "To <remoteurl>" messages to the standard output in --porcelain mode

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> If that's the case, then perhaps the "To: <destination>" lines should
> be sent to both stdout and stderr - stdout, for porcelain scripts as a
> "header", and stderr, to help the user make sense of any errors that
> occur ("oh, these errors were triggered when pushing to so-and-so
> remote").

Under --porcelain, to show or not to show is under control of the calling
script.  Not sending it to standard error stream doesn't cause any harm,
as the calling script can choose to echo it out to its standard error (or
standard output).

[PATCH 4/4] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

The script calling git push --dry-run --porcelain can see clearly from the
output that the updates will be rejected.  However, it will probably need to
distinguish this condition from the push failing for other reasons, such as the
remote not being reachable.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    3 ++-
 3 files changed, 7 insertions(+), 1 deletions(-)

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 fb653c6..0edcc16 100644
--- a/transport.c
+++ b/transport.c
@@ -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;
 
-- 
1.7.0.rc1.33.g07cf0f.dirty

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

[Jeff King]

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.

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

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

[Larry D'Anna]

* 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

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

[Tay Ray Chuan]

Hi,

On Wed, Feb 10, 2010 at 1:55 PM, Larry D'Anna <larry@elder-gods.org> wrote:
> * Jeff King (peff@peff.net) [100210 00:41]:
>> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 08, 2010 at 01:13:36PM -0800, Junio C Hamano wrote:
>
>> 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;
>> +	}
>
> I think this is sane.

I am tempted to suggest adding "clear_advice(void)" in advice.[ch], so
that people adding new advices do not have to hunt for even the above
hunk.  It would be a good direction to go in general _if_ we will have
more like this --porcelain thing in other parts of the system.

I didn't do so because that "_if_" is still iffy.

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

[Jeff King]

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

> >> +	if (flags & TRANSPORT_PUSH_PORCELAIN) {
> >> +		/* Do not give advice messages to Porcelain scripts */
> >> +		advice_push_nonfastforward = 0;
> >> +	}
> >
> > I think this is sane.
> 
> I am tempted to suggest adding "clear_advice(void)" in advice.[ch], so
> that people adding new advices do not have to hunt for even the above
> hunk.  It would be a good direction to go in general _if_ we will have
> more like this --porcelain thing in other parts of the system.
> 
> I didn't do so because that "_if_" is still iffy.

Would it make sense to clear _all_ advice if we are just in porcelain
mode for git-push? That is, let's say I am in "push --porcelain" and I
try to write a reflog entry for a local tracking ref, but my identity is
implicitly picked up from the hostname[1]. Should it trigger
advice_implicit_identity and say "by the way, your identity is implicit"
on stderr or not?

I would say yes. The advice output should all be on stderr, and the
porcelain output should all be on stdout. So there is no parsing
conflict. And stderr either goes to /dev/null (because the porcelain is
not terminal-based, or doesn't want the terminal screwed up), in which
case the advice does nothing, or stderr goes to the terminal (because
the porcelain is some simple script), in which case the message is
probably something the user would want to see.

-Peff

[1] I am actually not sure if you can trigger the implicit identity
advice in this way. But I am arguing from the perspective of "assume you
have triggered some random advice through some sub-action", which may
include advice to be added in the future.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Would it make sense to clear _all_ advice if we are just in porcelain
> mode for git-push? That is, let's say I am in "push --porcelain" and I
> try to write a reflog entry for a local tracking ref, but my identity is
> implicitly picked up from the hostname[1]. Should it trigger
> advice_implicit_identity and say "by the way, your identity is implicit"
> on stderr or not?
>
> I would say yes. The advice output should all be on stderr, and the
> porcelain output should all be on stdout. So there is no parsing
> conflict. And stderr either goes to /dev/null (because the porcelain is
> not terminal-based, or doesn't want the terminal screwed up), in which
> case the advice does nothing, or stderr goes to the terminal (because
> the porcelain is some simple script), in which case the message is
> probably something the user would want to see.

Hmm, if we make it a rule to send all the advice messages to the standard
error stream, wouldn't the logical conclusion be that the hunk you are
commenting is unnecessary?  "push --porcelain" should give advice just as
usual (but we would simply send the advice to the standard error stream).

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

[Jeff King]

On Thu, Feb 11, 2010 at 03:23:09PM -0800, Junio C Hamano wrote:

> > I would say yes. The advice output should all be on stderr, and the
> > porcelain output should all be on stdout. So there is no parsing
> > conflict. And stderr either goes to /dev/null (because the porcelain is
> > not terminal-based, or doesn't want the terminal screwed up), in which
> > case the advice does nothing, or stderr goes to the terminal (because
> > the porcelain is some simple script), in which case the message is
> > probably something the user would want to see.
> 
> Hmm, if we make it a rule to send all the advice messages to the standard
> error stream, wouldn't the logical conclusion be that the hunk you are
> commenting is unnecessary?  "push --porcelain" should give advice just as
> usual (but we would simply send the advice to the standard error stream).

Yes and no. The advice it is giving is specifically related to things
you can't see, so it is not really as helpful as it could be. And in
that sense, squelching may be positive.

But it seems that the thread has come to the conclusion that we're still
going to have messages on stderr. There is not much point in suppressing
one particular message if there is a bunch of other cruft. Either the
user should see stuff on stderr, or they shouldn't, and the caller can
decide by redirecting stderr. So yes, squelching that advice probably
should just be dropped.

-Peff

[PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

The script calling git push --dry-run --porcelain can see clearly from the
output that the updates will be rejected.  However, it will probably need to
distinguish this condition from the push failing for other reasons, such as the
remote not being reachable.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    3 ++-
 3 files changed, 7 insertions(+), 1 deletions(-)

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 00d986c..2b9e4be 100644
--- a/transport.c
+++ b/transport.c
@@ -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;
 
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> The script calling git push --dry-run --porcelain can see clearly from the
> output that the updates will be rejected.  However, it will probably need to
> distinguish this condition from the push failing for other reasons, such as the
> remote not being reachable.

I am not sure about this reasoning.  If you are telling the script writers
to decide what happened by reading from the output, shouldn't the program
say "I fail because I cannot reach the other side" to its standard output
so that the script can read it as well?

Having said that, I don't think it matters either way.  If a script wants
to know if push would fully succeed or not, it will run without
--porcelain (perhaps while discarding the standard error) and check the
status.  Even without this patch, if a script runs with --porcelain and
gets non-zero status, it can inspect the output and if it got rejection,
that is a sure sign that it at least reached the other end to get enough
information to decide that it will be rejected, no?

Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100208 15:59]:
> Larry D'Anna <larry@elder-gods.org> writes:
> 
> > The script calling git push --dry-run --porcelain can see clearly from the
> > output that the updates will be rejected.  However, it will probably need to
> > distinguish this condition from the push failing for other reasons, such as the
> > remote not being reachable.
> 
> I am not sure about this reasoning.  If you are telling the script writers
> to decide what happened by reading from the output, shouldn't the program
> say "I fail because I cannot reach the other side" to its standard output
> so that the script can read it as well?

Wouldn't that just complicate life for the script writer?  If you send such
messages to the standard output, the script would have to include logic to
distinguish error messages from the rest of the output.  If you send them to the
standard error then the script knows exactly where to find them.

> Having said that, I don't think it matters either way.  If a script wants
> to know if push would fully succeed or not, it will run without
> --porcelain (perhaps while discarding the standard error) and check the
> status.  Even without this patch, if a script runs with --porcelain and
> gets non-zero status, it can inspect the output and if it got rejection,
> that is a sure sign that it at least reached the other end to get enough
> information to decide that it will be rejected, no?

Yes, it's a sure sign that it reached the other side, but how does the script
know nothing else went wrong?  What if a malloc failed?  OK that's a bit far
fetched, but it's really, really nice to be able to get an unambiguous status
bit out of a command so you know if the requested operation succeeded or not.
Without this patch, a script calling git push --porcelain --dry-run has no way
of distinguishing these two situations

1) "git push" would try to push a ref that would get rejected, but everything
else is fine.

2) "git push" would try to push a ref that would get rejected, and also some
unknown type of error occurred that this script has no idea how to handle.

        --larry
            

Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> @@ -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);

Hmph, you are doing (rewritten in an easier to follow format)

	if (--dry-run && --porcelain)
		err = 0;
	else
		err = push_add_errors(remote_refs);

here, which I think changes the semantics of what follows immediately
after this hunk, namely:

	if (!quiet || err)
		print_push_status(transport->url, remote_refs,
				verbose | porcelain, porcelain,
				nonfastforward);

Earlier, the logic said "even if you asked for --quiet, we would report if
there is an error" but now you are changing the rule to "under --dry-run
and --porcelain, --quiet means don't ever report the status, even when
there are errors".

I don't necessarily think it is a bad change, but in any case the semantic
change is worth documenting.

Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100209 17:25]:
> Larry D'Anna <larry@elder-gods.org> writes:
> 
> > @@ -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);
> 
> Hmph, you are doing (rewritten in an easier to follow format)
> 
> 	if (--dry-run && --porcelain)
> 		err = 0;
> 	else
> 		err = push_add_errors(remote_refs);
> 
> here, which I think changes the semantics of what follows immediately
> after this hunk, namely:
> 
> 	if (!quiet || err)
> 		print_push_status(transport->url, remote_refs,
> 				verbose | porcelain, porcelain,
> 				nonfastforward);
> 
> Earlier, the logic said "even if you asked for --quiet, we would report if
> there is an error" but now you are changing the rule to "under --dry-run
> and --porcelain, --quiet means don't ever report the status, even when
> there are errors".
> 
> I don't necessarily think it is a bad change, but in any case the semantic
> change is worth documenting.

Now that you point it out, I can't really think of any reason why this patch
ought to mess with the semantics of --quiet.  

      --larry

[PATCH 4/4] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

The script calling git push --dry-run --porcelain can see clearly from the
output that the updates will be rejected.  However, it will probably need to
distinguish this condition from the push failing for other reasons, such as the
remote not being reachable.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-send-pack.c |    4 ++++
 send-pack.h         |    1 +
 transport.c         |    4 +++-
 3 files changed, 8 insertions(+), 1 deletions(-)

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 fb653c6..31f2e84 100644
--- a/transport.c
+++ b/transport.c
@@ -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);
@@ -1054,7 +1055,8 @@ int transport_push(struct transport *transport,
 		ret = transport->push_refs(transport, remote_refs, flags);
 		err = push_had_errors(remote_refs);
 
-		ret |= err;
+		if ( !(pretend && porcelain) )
+			ret |= err;
 
 		if (!quiet || err)
 			print_push_status(transport->url, remote_refs,
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100209 17:25]:
> Larry D'Anna <larry@elder-gods.org> writes:
> 
> > @@ -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);
> 
> Hmph, you are doing (rewritten in an easier to follow format)
> 
> 	if (--dry-run && --porcelain)
> 		err = 0;
> 	else
> 		err = push_add_errors(remote_refs);
> 
> here, which I think changes the semantics of what follows immediately
> after this hunk, namely:
> 
> 	if (!quiet || err)
> 		print_push_status(transport->url, remote_refs,
> 				verbose | porcelain, porcelain,
> 				nonfastforward);
> 
> Earlier, the logic said "even if you asked for --quiet, we would report if
> there is an error" but now you are changing the rule to "under --dry-run
> and --porcelain, --quiet means don't ever report the status, even when
> there are errors".
> 
> I don't necessarily think it is a bad change, but in any case the semantic
> change is worth documenting.

Which version of the --quiet behavior do you want for the next version of this
series?  My feeling is that "even if you asked for --quiet, we would report if
there is an error" is the best policy, even under --dry-run --porcelain.

         --larry

Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> Which version of the --quiet behavior do you want for the next version
> of this series?  My feeling is that "even if you asked for --quiet, we
> would report if there is an error" is the best policy, even under
> --dry-run --porcelain.

I think that is probably a sane and the least-surprising thing to do from
the end user's point of view.

[PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

The script calling git push --dry-run --porcelain can see clearly from the
output that the updates will be rejected.  However, it will probably need to
distinguish this condition from the push failing for other reasons, such as the
remote not being reachable.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 builtin-send-pack.c |    5 +++++
 send-pack.h         |    1 +
 transport.c         |   11 +++++++++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 76c7206..dfd7470 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -478,6 +478,11 @@ int send_pack(struct send_pack_args *args,
 		return ret;
 	for (ref = remote_refs; ref; ref = ref->next) {
 		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_NODELETE:
+			if (args->porcelain && args->dry_run)
+				break;
+			return -1;
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
 		case REF_STATUS_OK:
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 f707c7b..e61d288 100644
--- a/transport.c
+++ b/transport.c
@@ -558,10 +558,16 @@ static int fetch_refs_via_pack(struct transport *transport,
 	return (refs ? 0 : -1);
 }
 
-static int push_had_errors(struct ref *ref)
+static int push_had_errors(struct ref *ref, int flags)
 {
 	for (; ref; ref = ref->next) {
 		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_NODELETE:
+			if (flags & TRANSPORT_PUSH_DRY_RUN && flags & TRANSPORT_PUSH_PORCELAIN)
+				break;
+			else
+				return 1;
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
 		case REF_STATUS_OK:
@@ -791,6 +797,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 +1059,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 = push_had_errors(remote_refs, flags);
 
 		ret |= err;
 
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Jeff King]

On Fri, Feb 05, 2010 at 02:34:22PM -0500, Larry D'Anna wrote:

> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 76c7206..dfd7470 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -478,6 +478,11 @@ int send_pack(struct send_pack_args *args,
>  		return ret;
>  	for (ref = remote_refs; ref; ref = ref->next) {
>  		switch (ref->status) {
> +		case REF_STATUS_REJECT_NONFASTFORWARD:
> +		case REF_STATUS_REJECT_NODELETE:
> +			if (args->porcelain && args->dry_run)
> +				break;
> +			return -1;
>  		case REF_STATUS_NONE:
>  		case REF_STATUS_UPTODATE:
>  		case REF_STATUS_OK:

Why just these two status flags? Based on your reasoning elsewhere, I
would assume the logic should be:

  - if we had some transport-related error, return failure

  - if not, then return success, as any ref's failure is already
    indicated in the porcelain output

So shouldn't it just be:

  if (args->porcelain && args->dry_run)
          return 0;

after we check for transport errors but before the loop that you are
modifying.

> -static int push_had_errors(struct ref *ref)
> +static int push_had_errors(struct ref *ref, int flags)
>  {
>  	for (; ref; ref = ref->next) {
>  		switch (ref->status) {
> +		case REF_STATUS_REJECT_NONFASTFORWARD:
> +		case REF_STATUS_REJECT_NODELETE:
> +			if (flags & TRANSPORT_PUSH_DRY_RUN && flags & TRANSPORT_PUSH_PORCELAIN)
> +				break;
> +			else
> +				return 1;

Ditto here.

-Peff

Re: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Larry D'Anna]

* Jeff King (peff@peff.net) [100205 14:56]:
> On Fri, Feb 05, 2010 at 02:34:22PM -0500, Larry D'Anna wrote:
> 
> > diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> > index 76c7206..dfd7470 100644
> > --- a/builtin-send-pack.c
> > +++ b/builtin-send-pack.c
> > @@ -478,6 +478,11 @@ int send_pack(struct send_pack_args *args,
> >  		return ret;
> >  	for (ref = remote_refs; ref; ref = ref->next) {
> >  		switch (ref->status) {
> > +		case REF_STATUS_REJECT_NONFASTFORWARD:
> > +		case REF_STATUS_REJECT_NODELETE:
> > +			if (args->porcelain && args->dry_run)
> > +				break;
> > +			return -1;
> >  		case REF_STATUS_NONE:
> >  		case REF_STATUS_UPTODATE:
> >  		case REF_STATUS_OK:
> 
> Why just these two status flags? Based on your reasoning elsewhere, I
> would assume the logic should be:
> 
>   - if we had some transport-related error, return failure
> 
>   - if not, then return success, as any ref's failure is already
>     indicated in the porcelain output
> 
> So shouldn't it just be:
> 
>   if (args->porcelain && args->dry_run)
>           return 0;
> 
> after we check for transport errors but before the loop that you are
> modifying.

I don't know what the deal is with REF_STATUS_EXPECTING_REPORT, so I didn't want
to modify the behavior in the case that ref->status was that.  What does
expecting report mean?

          --larry

Re: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected

[Jeff King]

On Fri, Feb 05, 2010 at 03:05:24PM -0500, Larry D'Anna wrote:

> > So shouldn't it just be:
> > 
> >   if (args->porcelain && args->dry_run)
> >           return 0;
> > 
> > after we check for transport errors but before the loop that you are
> > modifying.
> 
> I don't know what the deal is with REF_STATUS_EXPECTING_REPORT, so I
> didn't want to modify the behavior in the case that ref->status was
> that.  What does expecting report mean?

It means we told the other side we wanted to push that ref, and we
expect it to give us a status report. Most refs are in that state for a
short period, and then moved to their final state in
builtin-send-pack.c:receive_status. But if we never get a status for
that ref for some reason, then that could be the final state.

But more to the point, I don't think this bit of code should _have_ to
care what it means. If there is a per-ref error with "push --dry-run
--porcelain", it will be shown on that ref's output line. So I think
your proposal should simply be "if dry-run and porcelain, don't bother
looking at per-ref errors at all". You don't care what the per-ref
error is; they are all in the same class from the perspective of this
change.

-Peff

Re: [PATCH] fix an error message in git-push so it goes to stderr

[Larry D'Anna]

* Jeff King (peff@peff.net) [100205 10:06]:
> On Thu, Feb 04, 2010 at 07:41:40PM -0500, Larry D'Anna wrote:
> 
> > Having it go to standard output interferes with git-push --porcelain.
> > ---
> >  builtin-push.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/builtin-push.c b/builtin-push.c
> > index 5633f0a..0a27072 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");
> 
> I agree that stderr is a more sensible place for such a message to go,
> but shouldn't the porcelain output format just suppress it entirely? 

I think you're right.  There are some other messages that are similar that
should probably also be suppressed.

Also it seems to me that git push --dry-run --porcelain should exit successfully
even if it knows some refs will be rejected.  The calling script can see just
fine for itself that they will be rejected, and it probably still wants to know
whether or not the dry-run succeeded, which has nothing to do with whether or
not the same push would succeed as a not-dry-run.  

    --larry

Re: [PATCH] fix an error message in git-push so it goes to stderr

[Jeff King]

On Fri, Feb 05, 2010 at 02:39:50PM -0500, Larry D'Anna wrote:

> Also it seems to me that git push --dry-run --porcelain should exit successfully
> even if it knows some refs will be rejected.  The calling script can see just
> fine for itself that they will be rejected, and it probably still wants to know
> whether or not the dry-run succeeded, which has nothing to do with whether or
> not the same push would succeed as a not-dry-run.

I think that is OK, but only if "git push --dry-run" still exits with an
error case, since people may be using it for "will this push work?" and
not simply "did an error occur?".

-Peff

Re: [PATCH] fix an error message in git-push so it goes to stderr

[Larry D'Anna]

* Jeff King (jrk@wrek.org) [100205 14:48]:
> On Fri, Feb 05, 2010 at 02:39:50PM -0500, Larry D'Anna wrote:
> 
> > Also it seems to me that git push --dry-run --porcelain should exit successfully
> > even if it knows some refs will be rejected.  The calling script can see just
> > fine for itself that they will be rejected, and it probably still wants to know
> > whether or not the dry-run succeeded, which has nothing to do with whether or
> > not the same push would succeed as a not-dry-run.
> 
> I think that is OK, but only if "git push --dry-run" still exits with an
> error case, since people may be using it for "will this push work?" and
> not simply "did an error occur?".

Yup.  That's exactly what the patch I just posted does.

      --larry

Re: [PATCH] fix an error message in git-push so it goes to stderr

[Jeff King]

On Fri, Feb 05, 2010 at 02:48:24PM -0500, Jeff King wrote:

> From: Jeff King <jrk@wrek.org>

Argh, stupid email configuration failure. Please address any followups
to my usual peff@peff.net.

-Peff