# [PATCH] fix an error message in git-push so it goes to stderr 14 messages from 2010-02-05 to 2010-02-05. Participants: Larry D'Anna, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/22526 ## Larry D'Anna, 2010-02-05 00:41 Subject: [PATCH] fix an error message in git-push so it goes to stderr Message-ID: <20100205004140.GA2841@cthulhu> URL: https://gitlist.dev/e/20100205004140.GA2841%40cthulhu ``` 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 ``` ## Jeff King, 2010-02-05 15:06 Subject: Re: [PATCH] fix an error message in git-push so it goes to stderr Message-ID: <20100205150638.GB14116@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100205150638.GB14116%40coredump.intra.peff.net In-Reply-To: <20100205004140.GA2841@cthulhu> ``` 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 ``` ## Larry D'Anna, 2010-02-05 19:34 Subject: [PATCH 1/3] fix an error message in git-push so it goes to stderr Message-ID: <1265398462-17316-1-git-send-email-larry@elder-gods.org> URL: https://gitlist.dev/e/1265398462-17316-1-git-send-email-larry%40elder-gods.org In-Reply-To: <20100205150638.GB14116@coredump.intra.peff.net> ``` Having it go to standard output interferes with git-push --porcelain. Signed-off-by: Larry D'Anna --- 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 ``` ## Larry D'Anna, 2010-02-05 19:34 Subject: [PATCH 2/3] silence human readable info messages going to stderr from git push --porcelain Message-ID: <1265398462-17316-2-git-send-email-larry@elder-gods.org> URL: https://gitlist.dev/e/1265398462-17316-2-git-send-email-larry%40elder-gods.org In-Reply-To: <20100205150638.GB14116@coredump.intra.peff.net> ``` These messages are redundant information to a script that's calling git-push. Signed-off-by: Larry D'Anna --- 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 ``` ## Larry D'Anna, 2010-02-05 19:34 Subject: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <1265398462-17316-3-git-send-email-larry@elder-gods.org> URL: https://gitlist.dev/e/1265398462-17316-3-git-send-email-larry%40elder-gods.org In-Reply-To: <20100205150638.GB14116@coredump.intra.peff.net> ``` 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 --- 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 ``` ## Larry D'Anna, 2010-02-05 19:39 Subject: Re: [PATCH] fix an error message in git-push so it goes to stderr Message-ID: <20100205193950.GA18108@cthulhu> URL: https://gitlist.dev/e/20100205193950.GA18108%40cthulhu In-Reply-To: <20100205150638.GB14116@coredump.intra.peff.net> ``` * 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 ``` ## Jeff King, 2010-02-05 19:48 Subject: Re: [PATCH] fix an error message in git-push so it goes to stderr Message-ID: <20100205194824.GD24474@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100205194824.GD24474%40coredump.intra.peff.net In-Reply-To: <20100205193950.GA18108@cthulhu> ``` 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 ``` ## Larry D'Anna, 2010-02-05 19:50 Subject: Re: [PATCH] fix an error message in git-push so it goes to stderr Message-ID: <20100205195004.GA21772@cthulhu> URL: https://gitlist.dev/e/20100205195004.GA21772%40cthulhu In-Reply-To: <20100205194824.GD24474@coredump.intra.peff.net> ``` * 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 ``` ## Jeff King, 2010-02-05 19:50 Subject: Re: [PATCH] fix an error message in git-push so it goes to stderr Message-ID: <20100205195039.GA25201@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100205195039.GA25201%40coredump.intra.peff.net In-Reply-To: <20100205194824.GD24474@coredump.intra.peff.net> ``` On Fri, Feb 05, 2010 at 02:48:24PM -0500, Jeff King wrote: > From: Jeff King Argh, stupid email configuration failure. Please address any followups to my usual peff@peff.net. -Peff ``` ## Jeff King, 2010-02-05 19:56 Subject: Re: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <20100205195644.GE24474@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100205195644.GE24474%40coredump.intra.peff.net In-Reply-To: <1265398462-17316-3-git-send-email-larry@elder-gods.org> ``` 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 ``` ## Larry D'Anna, 2010-02-05 20:05 Subject: Re: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <20100205200524.GA24027@cthulhu> URL: https://gitlist.dev/e/20100205200524.GA24027%40cthulhu In-Reply-To: <20100205195644.GE24474@coredump.intra.peff.net> ``` * 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 ``` ## Jeff King, 2010-02-05 20:13 Subject: Re: [PATCH 3/3] make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <20100205201325.GA25697@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100205201325.GA25697%40coredump.intra.peff.net In-Reply-To: <20100205200524.GA24027@cthulhu> ``` 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 ``` ## Junio C Hamano, 2010-02-05 20:20 Subject: Re: [PATCH 2/3] silence human readable info messages going to stderr from git push --porcelain Message-ID: <7v1vgz5ta7.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1vgz5ta7.fsf%40alter.siamese.dyndns.org In-Reply-To: <1265398462-17316-2-git-send-email-larry@elder-gods.org> ``` Larry D'Anna 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. ``` ## Larry D'Anna, 2010-02-05 20:30 Subject: Re: [PATCH 2/3] silence human readable info messages going to stderr from git push --porcelain Message-ID: <20100205203045.GA1937@cthulhu> URL: https://gitlist.dev/e/20100205203045.GA1937%40cthulhu In-Reply-To: <7v1vgz5ta7.fsf@alter.siamese.dyndns.org> ``` * 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 ```