# [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain 45 messages from 2010-02-08 to 2010-02-15. Participants: Larry D'Anna, Junio C Hamano, Jeff King, Shawn O. Pearce, Nicolas Pitre, Tay Ray Chuan. Thread: https://gitlist.dev/t/22574 ## Larry D'Anna, 2010-02-08 20:31 Subject: [PATCH v3 1/3] git-push: fix an error message so it goes to stderr Message-ID: <214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org> URL: https://gitlist.dev/e/214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry%40elder-gods.org In-Reply-To: <20100208201956.GA7015@cthulhu> ``` 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-08 20:31 Subject: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: URL: https://gitlist.dev/e/a1b71c9f6566549e6117f5c98c2f1e60754a7334.1265661033.git.larry%40elder-gods.org In-Reply-To: <214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org> ``` * 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 --- 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 ``` ## Larry D'Anna, 2010-02-08 20:31 Subject: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <032264a40d15cb9f4a86885947ffa23a603bfb0e.1265661033.git.larry@elder-gods.org> URL: https://gitlist.dev/e/032264a40d15cb9f4a86885947ffa23a603bfb0e.1265661033.git.larry%40elder-gods.org In-Reply-To: <214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org> ``` 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 | 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 ``` ## Junio C Hamano, 2010-02-08 20:45 Subject: Re: [PATCH v3 1/3] git-push: fix an error message so it goes to stderr Message-ID: <7vzl3jihhh.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzl3jihhh.fsf%40alter.siamese.dyndns.org In-Reply-To: <214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org> ``` Larry D'Anna 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. ``` ## Junio C Hamano, 2010-02-08 20:51 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7vtytrih7b.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtytrih7b.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Larry D'Anna 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? ``` ## Junio C Hamano, 2010-02-08 20:59 Subject: Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <7vpr4figv3.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpr4figv3.fsf%40alter.siamese.dyndns.org In-Reply-To: <032264a40d15cb9f4a86885947ffa23a603bfb0e.1265661033.git.larry@elder-gods.org> ``` Larry D'Anna 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? ``` ## Junio C Hamano, 2010-02-08 21:13 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7vvde7h1mn.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvde7h1mn.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vtytrih7b.fsf@alter.siamese.dyndns.org> ``` 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; ``` ## Jeff King, 2010-02-08 21:32 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100208213256.GA470@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100208213256.GA470%40coredump.intra.peff.net In-Reply-To: <7vvde7h1mn.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Larry D'Anna, 2010-02-08 21:49 Subject: Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <20100208214939.GA11355@cthulhu> URL: https://gitlist.dev/e/20100208214939.GA11355%40cthulhu In-Reply-To: <7vpr4figv3.fsf@alter.siamese.dyndns.org> ``` * Junio C Hamano (gitster@pobox.com) [100208 15:59]: > Larry D'Anna 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 ``` ## Larry D'Anna, 2010-02-08 22:15 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100208221558.GA21718@cthulhu> URL: https://gitlist.dev/e/20100208221558.GA21718%40cthulhu In-Reply-To: <20100208213256.GA470@coredump.intra.peff.net> ``` * 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 ``` ## Junio C Hamano, 2010-02-08 22:21 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7viqa7cqs9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7viqa7cqs9.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100208213256.GA470@coredump.intra.peff.net> ``` Jeff King 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?) ``` ## Larry D'Anna, 2010-02-08 22:31 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100208223107.GB21718@cthulhu> URL: https://gitlist.dev/e/20100208223107.GB21718%40cthulhu In-Reply-To: <7viqa7cqs9.fsf@alter.siamese.dyndns.org> ``` * Junio C Hamano (gitster@pobox.com) [100208 17:22]: > Jeff King 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 ``` ## Larry D'Anna, 2010-02-08 22:33 Subject: [PATCH] git-push: clean up some of the output from git push Message-ID: <1265668398-8635-1-git-send-email-larry@elder-gods.org> URL: https://gitlist.dev/e/1265668398-8635-1-git-send-email-larry%40elder-gods.org In-Reply-To: <20100208223107.GB21718@cthulhu> ``` * 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 --- 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 ``` ## Junio C Hamano, 2010-02-08 22:48 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7vpr4f9wey.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpr4f9wey.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100208223107.GB21718@cthulhu> ``` Larry D'Anna 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. ``` ## Junio C Hamano, 2010-02-08 22:59 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7vmxzj8hca.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmxzj8hca.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100208213256.GA470@coredump.intra.peff.net> ``` Jeff King 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. ``` ## Larry D'Anna, 2010-02-08 23:10 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100208231002.GA25729@cthulhu> URL: https://gitlist.dev/e/20100208231002.GA25729%40cthulhu In-Reply-To: <7vpr4f9wey.fsf@alter.siamese.dyndns.org> ``` * Junio C Hamano (gitster@pobox.com) [100208 17:48]: > Larry D'Anna 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 ``` ## Junio C Hamano, 2010-02-08 23:11 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7vmxzj726j.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmxzj726j.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100208231002.GA25729@cthulhu> ``` Larry D'Anna writes: >> Is this documented anywhere? > > aparnetly not. patch to follow. Thanks. ``` ## Larry D'Anna, 2010-02-08 23:44 Subject: [PATCH] git-push: fix the documentation to explain all the status flags Message-ID: <1265672654-26511-1-git-send-email-larry@elder-gods.org> URL: https://gitlist.dev/e/1265672654-26511-1-git-send-email-larry%40elder-gods.org In-Reply-To: <7vmxzj726j.fsf@alter.siamese.dyndns.org> ``` 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: \t : \t () ------------------------------- +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 ``` ## Junio C Hamano, 2010-02-09 00:23 Subject: Re: [PATCH] git-push: fix the documentation to explain all the status flags Message-ID: <7veikv2r50.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7veikv2r50.fsf%40alter.siamese.dyndns.org In-Reply-To: <1265672654-26511-1-git-send-email-larry@elder-gods.org> ``` Larry D'Anna 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: > \t : \t () > ------------------------------- > > +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 ``` ## Junio C Hamano, 2010-02-09 00:30 Subject: Re: [PATCH] git-push: fix the documentation to explain all the status flags Message-ID: <7vaavj2qu1.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vaavj2qu1.fsf%40alter.siamese.dyndns.org In-Reply-To: <7veikv2r50.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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. ``` ## Junio C Hamano, 2010-02-09 00:45 Subject: Re: [PATCH] git-push: fix the documentation to explain all the status flags Message-ID: <7vk4un1bku.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4un1bku.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vaavj2qu1.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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 --- 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: \t : \t () ------------------------------- +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 ``` ## Larry D'Anna, 2010-02-09 00:54 Subject: Re: [PATCH] git-push: fix the documentation to explain all the status flags Message-ID: <20100209005427.GA18023@cthulhu> URL: https://gitlist.dev/e/20100209005427.GA18023%40cthulhu In-Reply-To: <7veikv2r50.fsf@alter.siamese.dyndns.org> ``` * 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 ``` ## Larry D'Anna, 2010-02-09 00:56 Subject: Re: [PATCH] git-push: fix the documentation to explain all the status flags Message-ID: <20100209005622.GB18023@cthulhu> URL: https://gitlist.dev/e/20100209005622.GB18023%40cthulhu In-Reply-To: <7vk4un1bku.fsf@alter.siamese.dyndns.org> ``` * Junio C Hamano (gitster@pobox.com) [100208 19:45]: > Junio C Hamano 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 ``` ## Junio C Hamano, 2010-02-09 01:00 Subject: Re: [PATCH] git-push: fix the documentation to explain all the status flags Message-ID: <7vfx5bz0hk.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfx5bz0hk.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100209005622.GB18023@cthulhu> ``` Larry D'Anna 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. ``` ## Larry D'Anna, 2010-02-09 04:54 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100209045417.GA15210@cthulhu> URL: https://gitlist.dev/e/20100209045417.GA15210%40cthulhu In-Reply-To: <7vpr4f9wey.fsf@alter.siamese.dyndns.org> ``` * 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? ``` ## Larry D'Anna, 2010-02-09 05:48 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100209054820.GA30907@cthulhu> URL: https://gitlist.dev/e/20100209054820.GA30907%40cthulhu In-Reply-To: <7vpr4f9wey.fsf@alter.siamese.dyndns.org> ``` * 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 ``` ## Junio C Hamano, 2010-02-09 07:31 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7v4olqlva7.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4olqlva7.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100209045417.GA15210@cthulhu> ``` Larry D'Anna 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 URL: https://gitlist.dev/e/20100209162124.GA27790%40cthulhu In-Reply-To: <7v4olqlva7.fsf@alter.siamese.dyndns.org> ``` * Junio C Hamano (gitster@pobox.com) [100209 02:31]: > Larry D'Anna 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 ``` ## Shawn O. Pearce, 2010-02-09 17:51 Subject: Re: t5401-update-hooks test failure Message-ID: <20100209175139.GA28936@spearce.org> URL: https://gitlist.dev/e/20100209175139.GA28936%40spearce.org In-Reply-To: <7v4olqlva7.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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 > $ 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. ``` ## Nicolas Pitre, 2010-02-09 19:20 Subject: Re: t5401-update-hooks test failure Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1002091337421.1681%40xanadu.home In-Reply-To: <20100209175139.GA28936@spearce.org> ``` On Tue, 9 Feb 2010, Shawn O. Pearce wrote: > Junio C Hamano 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 > > > $ 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 ``` ## Shawn O. Pearce, 2010-02-09 19:26 Subject: Re: t5401-update-hooks test failure Message-ID: <20100209192628.GC28936@spearce.org> URL: https://gitlist.dev/e/20100209192628.GC28936%40spearce.org In-Reply-To: ``` Nicolas Pitre 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. ``` ## Junio C Hamano, 2010-02-09 22:25 Subject: Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <7vtytqyrlk.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtytqyrlk.fsf%40alter.siamese.dyndns.org In-Reply-To: <032264a40d15cb9f4a86885947ffa23a603bfb0e.1265661033.git.larry@elder-gods.org> ``` Larry D'Anna 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. ``` ## Junio C Hamano, 2010-02-09 22:44 Subject: Re: t5401-update-hooks test failure Message-ID: <7vpr4eyqok.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpr4eyqok.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100209192628.GC28936@spearce.org> ``` "Shawn O. Pearce" 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. ``` ## Junio C Hamano, 2010-02-09 23:16 Subject: Re: t5401-update-hooks test failure Message-ID: <7vljf2xane.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljf2xane.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vpr4eyqok.fsf@alter.siamese.dyndns.org> ``` 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' ' ``` ## Shawn O. Pearce, 2010-02-10 01:29 Subject: Re: t5401-update-hooks test failure Message-ID: <20100210012935.GD28936@spearce.org> URL: https://gitlist.dev/e/20100210012935.GD28936%40spearce.org In-Reply-To: <7vpr4eyqok.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Shawn O. Pearce" 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. ``` ## Larry D'Anna, 2010-02-10 04:13 Subject: Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <20100210041313.GA21516@cthulhu> URL: https://gitlist.dev/e/20100210041313.GA21516%40cthulhu In-Reply-To: <7vtytqyrlk.fsf@alter.siamese.dyndns.org> ``` * Junio C Hamano (gitster@pobox.com) [100209 17:25]: > Larry D'Anna 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 ``` ## Larry D'Anna, 2010-02-10 04:51 Subject: [PATCH 4/4] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <1265777470-11231-1-git-send-email-larry@elder-gods.org> URL: https://gitlist.dev/e/1265777470-11231-1-git-send-email-larry%40elder-gods.org In-Reply-To: <20100210041313.GA21516@cthulhu> ``` 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 | 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 ``` ## Jeff King, 2010-02-10 05:39 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100210053937.GF28526@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100210053937.GF28526%40coredump.intra.peff.net In-Reply-To: <7viqa7cqs9.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Jeff King, 2010-02-10 05:49 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100210054957.GG28526@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100210054957.GG28526%40coredump.intra.peff.net In-Reply-To: <7vmxzj8hca.fsf@alter.siamese.dyndns.org> ``` 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. ``` ## Larry D'Anna, 2010-02-10 05:55 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100210055529.GA1566@cthulhu> URL: https://gitlist.dev/e/20100210055529.GA1566%40cthulhu In-Reply-To: <20100210053937.GF28526@coredump.intra.peff.net> ``` * 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 ``` ## Tay Ray Chuan, 2010-02-10 10:43 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: URL: https://gitlist.dev/e/be6fef0d1002100243l52d1cbd4x5ab01283d54c5e9e%40mail.gmail.com In-Reply-To: <20100210055529.GA1566@cthulhu> ``` Hi, On Wed, Feb 10, 2010 at 1:55 PM, Larry D'Anna 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 ``` ## Junio C Hamano, 2010-02-11 23:23 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <7vwryjpdb6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwryjpdb6.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100210054957.GG28526@coredump.intra.peff.net> ``` Jeff King 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). ``` ## Jeff King, 2010-02-12 00:03 Subject: Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain Message-ID: <20100212000302.GB21930@coredump.intra.peff.net> URL: https://gitlist.dev/e/20100212000302.GB21930%40coredump.intra.peff.net In-Reply-To: <7vwryjpdb6.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Larry D'Anna, 2010-02-15 17:40 Subject: Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <20100215174013.GA12761@cthulhu> URL: https://gitlist.dev/e/20100215174013.GA12761%40cthulhu In-Reply-To: <7vtytqyrlk.fsf@alter.siamese.dyndns.org> ``` * Junio C Hamano (gitster@pobox.com) [100209 17:25]: > Larry D'Anna 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 ``` ## Junio C Hamano, 2010-02-15 20:42 Subject: Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected Message-ID: <7vmxzaxmc6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmxzaxmc6.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100215174013.GA12761@cthulhu> ``` Larry D'Anna 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. ```