{"thread":{"id":"22574","subject":"[PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","startedAt":"2010-02-08T20:31:22Z","lastAt":"2010-02-15T20:42:17Z","messageCount":45,"participants":["Larry D'Anna","Junio C Hamano","Jeff King","Shawn O. Pearce","Nicolas Pitre","Tay Ray Chuan"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"133947","messageId":"214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org","threadId":"22574","inReplyTo":"20100208201956.GA7015@cthulhu","subject":"[PATCH v3 1/3] git-push: fix an error message so it goes to stderr","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T20:31:22Z","receivedAt":"2010-02-08T20:31:22Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"Having it go to standard output interferes with git-push --porcelain.\n\nSigned-off-by: Larry D'Anna \n---\n builtin-push.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 5633f0a..0a27072 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -124,9 +124,9 @@ static int push_with_options(struct transport *transport, int flags)\n \t\treturn 0;\n \n \tif (nonfastforward && advice_push_nonfastforward) {\n-\t\tprintf(\"To prevent you from losing history, non-fast-forward updates were rejected\\n\"\n-\t\t \"Merge the remote changes before pushing again. See the 'Note about\\n\"\n-\t\t \"fast-forwards' section of 'git push --help' for details.\\n\");\n+\t\tfprintf(stderr, \"To prevent you from losing history, non-fast-forward updates were rejected\\n\"\n+\t\t\t\t\"Merge the remote changes before pushing again. See the 'Note about\\n\"\n+\t\t\t\t\"fast-forwards' section of 'git push --help' for details.\\n\");\n \t}\n \n \treturn 1;\n-- \n1.7.0.rc1.33.g07cf0f.dirty\n"},{"id":"133945","messageId":"a1b71c9f6566549e6117f5c98c2f1e60754a7334.1265661033.git.larry@elder-gods.org","threadId":"22574","inReplyTo":"214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org","subject":"[PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T20:31:23Z","receivedAt":"2010-02-08T20:31:23Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* don't emit long explanatory message about non-fast-forward updates.\n\n* send \"To dest\" lines to standard out so whoever is reading standard out knows\n which ref updates went to which remotes.\n\n* only send the \"Everything up-to-date\" line if verbose.\n\nSigned-off-by: Larry D'Anna \n---\n builtin-push.c | 2 +-\n transport.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 0a27072..ff0b1c6 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -123,7 +123,7 @@ static int push_with_options(struct transport *transport, int flags)\n \tif (!err)\n \t\treturn 0;\n \n-\tif (nonfastforward && advice_push_nonfastforward) {\n+\tif (!(flags & TRANSPORT_PUSH_PORCELAIN) && nonfastforward && advice_push_nonfastforward) {\n \t\tfprintf(stderr, \"To prevent you from losing history, non-fast-forward updates were rejected\\n\"\n \t\t\t\t\"Merge the remote changes before pushing again. See the 'Note about\\n\"\n \t\t\t\t\"fast-forwards' section of 'git push --help' for details.\\n\");\ndiff --git a/transport.c b/transport.c\nindex 3846aac..00d986c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -675,7 +675,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)\n static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)\n {\n \tif (!count)\n-\t\tfprintf(stderr, \"To %s\\n\", dest);\n+\t\tfprintf(porcelain ? stdout : stderr, \"To %s\\n\", dest);\n \n \tswitch(ref->status) {\n \tcase REF_STATUS_NONE:\n@@ -1070,7 +1070,7 @@ int transport_push(struct transport *transport,\n \t\t\t\tupdate_tracking_ref(transport->remote, ref, verbose);\n \t\t}\n \n-\t\tif (!quiet && !ret && !refs_pushed(remote_refs))\n+\t\tif (!quiet && (!porcelain || verbose) && !ret && !refs_pushed(remote_refs))\n \t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \t\treturn ret;\n \t}\n-- \n1.7.0.rc1.33.g07cf0f.dirty\n"},{"id":"133946","messageId":"032264a40d15cb9f4a86885947ffa23a603bfb0e.1265661033.git.larry@elder-gods.org","threadId":"22574","inReplyTo":"214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org","subject":"[PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T20:31:24Z","receivedAt":"2010-02-08T20:31:24Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"The script calling git push --dry-run --porcelain can see clearly from the\noutput that the updates will be rejected. However, it will probably need to\ndistinguish this condition from the push failing for other reasons, such as the\nremote not being reachable.\n\nSigned-off-by: Larry D'Anna \n---\n builtin-send-pack.c | 4 ++++\n send-pack.h | 1 +\n transport.c | 3 ++-\n 3 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 76c7206..358f5e1 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -476,6 +476,10 @@ int send_pack(struct send_pack_args *args,\n \n \tif (ret < 0)\n \t\treturn ret;\n+\n+\tif (args->porcelain && args->dry_run)\n+\t\treturn 0;\n+\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\ndiff --git a/send-pack.h b/send-pack.h\nindex 28141ac..60b4ba6 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -4,6 +4,7 @@\n struct send_pack_args {\n \tunsigned verbose:1,\n \t\tquiet:1,\n+\t\tporcelain:1,\n \t\tsend_mirror:1,\n \t\tforce_update:1,\n \t\tuse_thin_pack:1,\ndiff --git a/transport.c b/transport.c\nindex 00d986c..2b9e4be 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -791,6 +791,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);\n \targs.quiet = !!(flags & TRANSPORT_PUSH_QUIET);\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n+\targs.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n \n \tret = send_pack(&args, data->fd, data->conn, remote_refs,\n \t\t\t&data->extra_have);\n@@ -1052,7 +1053,7 @@ int transport_push(struct transport *transport,\n \t\t\tflags & TRANSPORT_PUSH_FORCE);\n \n \t\tret = transport->push_refs(transport, remote_refs, flags);\n-\t\terr = push_had_errors(remote_refs);\n+\t\terr = (pretend && porcelain) ? 0 : push_had_errors(remote_refs);\n \n \t\tret |= err;\n \n-- \n1.7.0.rc1.33.g07cf0f.dirty\n"},{"id":"133949","messageId":"7vzl3jihhh.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"214a0317f2e4707a866b2f5d10509296bc1479c1.1265661033.git.larry@elder-gods.org","subject":"Re: [PATCH v3 1/3] git-push: fix an error message so it goes to stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T20:45:46Z","receivedAt":"2010-02-08T20:45:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> Having it go to standard output interferes with git-push --porcelain.\n\nCould you at least update this rationale?\n\nYou will be squelching this in [2/3] when --porcelain is used, so the\nabove is no longer a good justification. It won't interfere at all.\n"},{"id":"133950","messageId":"7vtytrih7b.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"a1b71c9f6566549e6117f5c98c2f1e60754a7334.1265661033.git.larry@elder-gods.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T20:51:52Z","receivedAt":"2010-02-08T20:51:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> * don't emit long explanatory message about non-fast-forward updates.\n\nThis makes sense as a goal, but at the same time as an implementation it\nwould be cleaner to flip \"advice\" off under --porcelain, instead of doing\n\"s/if (advice_blah)/if (advice_blah && !porcelain)/;\".\n\nThis is doubly important if you consider longer term maintainability. I\ndo not want to see the next person who tries to add new advice messages to\ncopy and paste the long if() statement you added in this patch.\n\n> * send \"To dest\" lines to standard out so whoever is reading standard\n> out knows which ref updates went to which remotes.\n\nMakes sense. s/standard out/the standard output/, and\ns/reading .*knows/reading from the process knows/, perhaps.\n\n> * only send the \"Everything up-to-date\" line if verbose.\n\nDon't you want to send this also to stdout?\n"},{"id":"133952","messageId":"7vpr4figv3.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"032264a40d15cb9f4a86885947ffa23a603bfb0e.1265661033.git.larry@elder-gods.org","subject":"Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T20:59:12Z","receivedAt":"2010-02-08T20:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> The script calling git push --dry-run --porcelain can see clearly from the\n> output that the updates will be rejected. However, it will probably need to\n> distinguish this condition from the push failing for other reasons, such as the\n> remote not being reachable.\n\nI am not sure about this reasoning. If you are telling the script writers\nto decide what happened by reading from the output, shouldn't the program\nsay \"I fail because I cannot reach the other side\" to its standard output\nso that the script can read it as well?\n\nHaving said that, I don't think it matters either way. If a script wants\nto know if push would fully succeed or not, it will run without\n--porcelain (perhaps while discarding the standard error) and check the\nstatus. Even without this patch, if a script runs with --porcelain and\ngets non-zero status, it can inspect the output and if it got rejection,\nthat is a sure sign that it at least reached the other end to get enough\ninformation to decide that it will be rejected, no?\n"},{"id":"133953","messageId":"7vvde7h1mn.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"7vtytrih7b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T21:13:36Z","receivedAt":"2010-02-08T21:13:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I realize that I more-or-less repeated what I already said for the second\nround, so let's try a different approach.\n\nHow about replacing the three patches with something like this?\n\n-- >8 --\nSubject: push --porcelain: usability updates\n\n\"git push --porcelain\" is meant for Porcelain scripts to read from; there\nis no reason to give advice messages meant for the user under that mode.\n\nWhen reporting the update status, the name of the destination and\n\"Everything up-to-date\" message were shown to the standard error stream,\nbut that is unfriendly when a Porcelain script is reading from us. Send\nthem to the standard output to make it easier for them.\n\n---\n builtin-push.c | 5 +++++\n transport.c | 5 +++--\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 5633f0a..f5082d8 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -226,6 +226,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \n+\tif (flags & TRANSPORT_PUSH_PORCELAIN) {\n+\t\t/* Do not give advice messages to Porcelain scripts */\n+\t\tadvice_push_nonfastforward = 0;\n+\t}\n+\n \tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n \t\tdie(\"--delete is incompatible with --all, --mirror and --tags\");\n \tif (deleterefs && argc < 2)\ndiff --git a/transport.c b/transport.c\nindex 3846aac..0492934 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -675,7 +675,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)\n static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)\n {\n \tif (!count)\n-\t\tfprintf(stderr, \"To %s\\n\", dest);\n+\t\tfprintf(porcelain ? stdout : stderr, \"To %s\\n\", dest);\n \n \tswitch(ref->status) {\n \tcase REF_STATUS_NONE:\n@@ -1071,7 +1071,8 @@ int transport_push(struct transport *transport,\n \t\t}\n \n \t\tif (!quiet && !ret && !refs_pushed(remote_refs))\n-\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n+\t\t\tfprintf(porcelain ? stdout : stderr,\n+\t\t\t\t\"Everything up-to-date\\n\");\n \t\treturn ret;\n \t}\n \treturn 1;\n"},{"id":"133955","messageId":"20100208213256.GA470@coredump.intra.peff.net","threadId":"22574","inReplyTo":"7vvde7h1mn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-08T21:32:57Z","receivedAt":"2010-02-08T21:32:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 01:13:36PM -0800, Junio C Hamano wrote:\n\n> diff --git a/builtin-push.c b/builtin-push.c\n> index 5633f0a..f5082d8 100644\n> --- a/builtin-push.c\n> +++ b/builtin-push.c\n> @@ -226,6 +226,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n> \tgit_config(git_default_config, NULL);\n> \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n> \n> +\tif (flags & TRANSPORT_PUSH_PORCELAIN) {\n> +\t\t/* Do not give advice messages to Porcelain scripts */\n> +\t\tadvice_push_nonfastforward = 0;\n> +\t}\n\nI think this is sane.\n\n> {\n> \tif (!count)\n> -\t\tfprintf(stderr, \"To %s\\n\", dest);\n> +\t\tfprintf(porcelain ? stdout : stderr, \"To %s\\n\", dest);\n\nBut note here that you are changing the --porcelain format, as callers\nwhich were keeping only the stdout (and letting stderr go to /dev/null,\nor spew to the user) saw only the ref lines. So this may be breaking\nsuch callers.\n\nI think you argued elsewhere (and I agree) that with multiple push urls,\nthis information is useful. Which means that the original porcelain\nformat was perhaps not very well thought-out. :( So we have to choose\nnow whether to fix it and break compatibility, or leave it broken. If\nthe former, then we should make sure there are not other design issues\nin need of fixing, so we can just break compatibility _once_.\n\n> @@ -1071,7 +1071,8 @@ int transport_push(struct transport *transport,\n> \t\t}\n> \n> \t\tif (!quiet && !ret && !refs_pushed(remote_refs))\n> -\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n> +\t\t\tfprintf(porcelain ? stdout : stderr,\n> +\t\t\t\t\"Everything up-to-date\\n\");\n> \t\treturn ret;\n> \t}\n\nThis one, on the other hand, seems to me to be just noise. What does a\n--porcelain caller learn by seeing \"Everything up-to-date\" that it did\nnot already know from seeing the list of refs?\n\n-Peff\n"},{"id":"133956","messageId":"20100208214939.GA11355@cthulhu","threadId":"22574","inReplyTo":"7vpr4figv3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T21:49:39Z","receivedAt":"2010-02-08T21:49:39Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100208 15:59]:\n> Larry D'Anna writes:\n> \n> > The script calling git push --dry-run --porcelain can see clearly from the\n> > output that the updates will be rejected. However, it will probably need to\n> > distinguish this condition from the push failing for other reasons, such as the\n> > remote not being reachable.\n> \n> I am not sure about this reasoning. If you are telling the script writers\n> to decide what happened by reading from the output, shouldn't the program\n> say \"I fail because I cannot reach the other side\" to its standard output\n> so that the script can read it as well?\n\nWouldn't that just complicate life for the script writer? If you send such\nmessages to the standard output, the script would have to include logic to\ndistinguish error messages from the rest of the output. If you send them to the\nstandard error then the script knows exactly where to find them.\n\n> Having said that, I don't think it matters either way. If a script wants\n> to know if push would fully succeed or not, it will run without\n> --porcelain (perhaps while discarding the standard error) and check the\n> status. Even without this patch, if a script runs with --porcelain and\n> gets non-zero status, it can inspect the output and if it got rejection,\n> that is a sure sign that it at least reached the other end to get enough\n> information to decide that it will be rejected, no?\n\nYes, it's a sure sign that it reached the other side, but how does the script\nknow nothing else went wrong? What if a malloc failed? OK that's a bit far\nfetched, but it's really, really nice to be able to get an unambiguous status\nbit out of a command so you know if the requested operation succeeded or not.\nWithout this patch, a script calling git push --porcelain --dry-run has no way\nof distinguishing these two situations\n\n1) \"git push\" would try to push a ref that would get rejected, but everything\nelse is fine.\n\n2) \"git push\" would try to push a ref that would get rejected, and also some\nunknown type of error occurred that this script has no idea how to handle.\n\n --larry\n \n"},{"id":"133958","messageId":"20100208221558.GA21718@cthulhu","threadId":"22574","inReplyTo":"20100208213256.GA470@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T22:15:58Z","receivedAt":"2010-02-08T22:15:58Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Jeff King (peff@peff.net) [100208 16:32]:\n> > @@ -1071,7 +1071,8 @@ int transport_push(struct transport *transport,\n> > \t\t}\n> > \n> > \t\tif (!quiet && !ret && !refs_pushed(remote_refs))\n> > -\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n> > +\t\t\tfprintf(porcelain ? stdout : stderr,\n> > +\t\t\t\t\"Everything up-to-date\\n\");\n> > \t\treturn ret;\n> > \t}\n> \n> This one, on the other hand, seems to me to be just noise. What does a\n> --porcelain caller learn by seeing \"Everything up-to-date\" that it did\n> not already know from seeing the list of refs?\n\nI agree. I don't see how sending this message to stdout could possibly help the\n--porcelain caller.\n\n\n --larry\n"},{"id":"133959","messageId":"7viqa7cqs9.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100208213256.GA470@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T22:21:26Z","receivedAt":"2010-02-08T22:21:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... Which means that the original porcelain\n> format was perhaps not very well thought-out.\n> ...\n> now whether to fix it and break compatibility, or leave it broken...\n\nI think the purpose of the patches that started this thread was to admit\nthat 1965ff7 (add --porcelain option to git-push, 2009-06-22) was not well\nthought out, and to break compatibility to fix it.\n\nHaving said that, I would say that what 1965ff7 specified was only these\ntwo:\n\n = TAB refs/heads/master:refs/heads/master TAB [up to date]\n - TAB :refs/heads/foobar TAB [deleted]\n\nso everything else that do not match this pattern is a fair game, most\nimportantly, the line that begins with \"To\" would not be mistaken with\nthis pattern, I think.\n\n>> @@ -1071,7 +1071,8 @@ int transport_push(struct transport *transport,\n>> \t\t}\n>> \n>> \t\tif (!quiet && !ret && !refs_pushed(remote_refs))\n>> -\t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n>> +\t\t\tfprintf(porcelain ? stdout : stderr,\n>> +\t\t\t\t\"Everything up-to-date\\n\");\n>> \t\treturn ret;\n>> \t}\n>\n> This one, on the other hand, seems to me to be just noise. What does a\n> --porcelain caller learn by seeing \"Everything up-to-date\" that it did\n> not already know from seeing the list of refs?\n\nI do not care too much about this hunk either way. We could leave it as\nis, as we will be giving some other stuff to the standard error stream\nwithout squelching anyway, even with the three-patch series. We could\nsquelch only this message, but it is dubious what it is buying us. If you\nforced me to decide, I would probably say \"let's just drop this hunk and\nkeep the code as-is\".\n\nAs to the exit status, do you have any thoughts, by the way?\n\nI am not convinced that it would be necessary nor even a good idea to make\nit behave inconsistently between the normal case and Porcelain case, only\nto make it easier to special case the \"remote side would reject due to\nnon-fast-forward\" failure mode (iow, even if the calling script knows that\nit would fail due to non-fast-forward but otherwise everything else would\nbe fine, what good would it do?)\n"},{"id":"133960","messageId":"20100208223107.GB21718@cthulhu","threadId":"22574","inReplyTo":"7viqa7cqs9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T22:31:07Z","receivedAt":"2010-02-08T22:31:07Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100208 17:22]:\n> Jeff King writes:\n> \n> > ... Which means that the original porcelain\n> > format was perhaps not very well thought-out.\n> > ...\n> > now whether to fix it and break compatibility, or leave it broken...\n> \n> I think the purpose of the patches that started this thread was to admit\n> that 1965ff7 (add --porcelain option to git-push, 2009-06-22) was not well\n> thought out, and to break compatibility to fix it.\n> \n> Having said that, I would say that what 1965ff7 specified was only these\n> two:\n> \n> = TAB refs/heads/master:refs/heads/master TAB [up to date]\n> - TAB :refs/heads/foobar TAB [deleted]\n\n\nalso these\n\nX TAB ...\n! TAB ...\n* TAB ...\n+ TAB ...\nSPACE TAB ...\n\nI'll respond to this message with another iteration of the patch.\n\n --larry\n"},{"id":"133961","messageId":"1265668398-8635-1-git-send-email-larry@elder-gods.org","threadId":"22574","inReplyTo":"20100208223107.GB21718@cthulhu","subject":"[PATCH] git-push: clean up some of the output from git push","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T22:33:18Z","receivedAt":"2010-02-08T22:33:18Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* send the \"To prevent you from losing history, non-fast-forward....\" message to\n the standard error. That's where these sort of advice messages typically go\n\n* for git push --porcelain: squelch the above advice.\n\n* for git push --porcelain: send \"To dest\" lines to the standard output so\n whoever is reading from the process which ref updates went to which remotes.\n\n* for git push --porcelain: only send the \"Everything up-to-date\" line if\n verbose.\n\n* for git push --porcelain --dry-run: exit with status 0 even if updates will be\n rejected. Whoever is reading the output of git push--dry-run --porcelain can\n clearly see if updates will be rejected. However, it will probably need to\n distinguish this condition from other unknown errors that it does not know how\n to handle.\n\nSigned-off-by: Larry D'Anna \n---\n builtin-push.c | 11 ++++++++---\n builtin-send-pack.c | 4 ++++\n send-pack.h | 1 +\n transport.c | 7 ++++---\n 4 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 5633f0a..aacba45 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -124,9 +124,9 @@ static int push_with_options(struct transport *transport, int flags)\n \t\treturn 0;\n \n \tif (nonfastforward && advice_push_nonfastforward) {\n-\t\tprintf(\"To prevent you from losing history, non-fast-forward updates were rejected\\n\"\n-\t\t \"Merge the remote changes before pushing again. See the 'Note about\\n\"\n-\t\t \"fast-forwards' section of 'git push --help' for details.\\n\");\n+\t\tfprintf(stderr, \"To prevent you from losing history, non-fast-forward updates were rejected\\n\"\n+\t\t\t\t\"Merge the remote changes before pushing again. See the 'Note about\\n\"\n+\t\t\t\t\"fast-forwards' section of 'git push --help' for details.\\n\");\n \t}\n \n \treturn 1;\n@@ -226,6 +226,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \n+\tif (flags & TRANSPORT_PUSH_PORCELAIN) {\n+\t\t/* Do not give advice messages to Porcelain scripts */\n+\t\tadvice_push_nonfastforward = 0;\n+\t}\n+\n \tif (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))\n \t\tdie(\"--delete is incompatible with --all, --mirror and --tags\");\n \tif (deleterefs && argc < 2)\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 76c7206..358f5e1 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -476,6 +476,10 @@ int send_pack(struct send_pack_args *args,\n \n \tif (ret < 0)\n \t\treturn ret;\n+\n+\tif (args->porcelain && args->dry_run)\n+\t\treturn 0;\n+\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\ndiff --git a/send-pack.h b/send-pack.h\nindex 28141ac..60b4ba6 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -4,6 +4,7 @@\n struct send_pack_args {\n \tunsigned verbose:1,\n \t\tquiet:1,\n+\t\tporcelain:1,\n \t\tsend_mirror:1,\n \t\tforce_update:1,\n \t\tuse_thin_pack:1,\ndiff --git a/transport.c b/transport.c\nindex 3846aac..2b9e4be 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -675,7 +675,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)\n static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)\n {\n \tif (!count)\n-\t\tfprintf(stderr, \"To %s\\n\", dest);\n+\t\tfprintf(porcelain ? stdout : stderr, \"To %s\\n\", dest);\n \n \tswitch(ref->status) {\n \tcase REF_STATUS_NONE:\n@@ -791,6 +791,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);\n \targs.quiet = !!(flags & TRANSPORT_PUSH_QUIET);\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n+\targs.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n \n \tret = send_pack(&args, data->fd, data->conn, remote_refs,\n \t\t\t&data->extra_have);\n@@ -1052,7 +1053,7 @@ int transport_push(struct transport *transport,\n \t\t\tflags & TRANSPORT_PUSH_FORCE);\n \n \t\tret = transport->push_refs(transport, remote_refs, flags);\n-\t\terr = push_had_errors(remote_refs);\n+\t\terr = (pretend && porcelain) ? 0 : push_had_errors(remote_refs);\n \n \t\tret |= err;\n \n@@ -1070,7 +1071,7 @@ int transport_push(struct transport *transport,\n \t\t\t\tupdate_tracking_ref(transport->remote, ref, verbose);\n \t\t}\n \n-\t\tif (!quiet && !ret && !refs_pushed(remote_refs))\n+\t\tif (!quiet && (!porcelain || verbose) && !ret && !refs_pushed(remote_refs))\n \t\t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \t\treturn ret;\n \t}\n-- \n1.7.0.rc1.33.g07cf0f.dirty\n"},{"id":"133962","messageId":"7vpr4f9wey.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100208223107.GB21718@cthulhu","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T22:48:05Z","receivedAt":"2010-02-08T22:48:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> also these\n>\n> X TAB ...\n> ! TAB ...\n> * TAB ...\n> + TAB ...\n> SPACE TAB ...\n\nIs this documented anywhere?\n\nMuch of what your patches in this thread tried to do does not interest me\nvery much rignt now, i.e. during the pre-release freeze period [*1*].\n\nIf you are already showing '*' and '+' without telling the users in the\ndocumentation, it is a much more urgent issue that needs fixing, and it\nwould be preferrable if we can do so before the release.\n\nDocumentation/git-push.txt seems to say that only ' ', '!' and '=' have\nany defined meaning:\n\n flag::\n A single character indicating the status of the ref. This is\n blank for a successfully pushed ref, `!` for a ref that was\n rejected or failed to push, and '=' for a ref that was up to\n date and did not need pushing (note that the status of up to\n date refs is shown only when `git push` is running verbosely).\n\n\n[Footnote]\n\n*1* As I hinted repeatedly, I think many of them are mere churn, except\nfor \"don't advice porcelain scripts\" (good) and perhaps \"exit with failure\nstatus upon only this kind of failure\" (I am undecided). Especially I\nwant to see the \"send advice to the standard error stream\" one as a\nseparate patch, if you insist including that change in the series.\n\nBut they are something we can decide after 1.7.0 happens.\n"},{"id":"133965","messageId":"7vmxzj8hca.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100208213256.GA470@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T22:59:01Z","receivedAt":"2010-02-08T22:59:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 08, 2010 at 01:13:36PM -0800, Junio C Hamano wrote:\n>\n>> diff --git a/builtin-push.c b/builtin-push.c\n>> index 5633f0a..f5082d8 100644\n>> --- a/builtin-push.c\n>> +++ b/builtin-push.c\n>> @@ -226,6 +226,11 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>> \tgit_config(git_default_config, NULL);\n>> \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n>> \n>> +\tif (flags & TRANSPORT_PUSH_PORCELAIN) {\n>> +\t\t/* Do not give advice messages to Porcelain scripts */\n>> +\t\tadvice_push_nonfastforward = 0;\n>> +\t}\n>\n> I think this is sane.\n\nI am tempted to suggest adding \"clear_advice(void)\" in advice.[ch], so\nthat people adding new advices do not have to hunt for even the above\nhunk. It would be a good direction to go in general _if_ we will have\nmore like this --porcelain thing in other parts of the system.\n\nI didn't do so because that \"_if_\" is still iffy.\n"},{"id":"133967","messageId":"20100208231002.GA25729@cthulhu","threadId":"22574","inReplyTo":"7vpr4f9wey.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T23:10:02Z","receivedAt":"2010-02-08T23:10:02Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100208 17:48]:\n> Larry D'Anna writes:\n> \n> > also these\n> >\n> > X TAB ...\n> > ! TAB ...\n> > * TAB ...\n> > + TAB ...\n> > SPACE TAB ...\n\nActually it won't to 'X'. print_one_push_status has a case for 'X', but it\nnever gets called. \n\n> Is this documented anywhere?\n\naparnetly not. patch to follow.\n\n --larry\n"},{"id":"133968","messageId":"7vmxzj726j.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100208231002.GA25729@cthulhu","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-08T23:11:48Z","receivedAt":"2010-02-08T23:11:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n>> Is this documented anywhere?\n>\n> aparnetly not. patch to follow.\n\nThanks.\n"},{"id":"133969","messageId":"1265672654-26511-1-git-send-email-larry@elder-gods.org","threadId":"22574","inReplyTo":"7vmxzj726j.fsf@alter.siamese.dyndns.org","subject":"[PATCH] git-push: fix the documentation to explain all the status flags","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-08T23:44:14Z","receivedAt":"2010-02-08T23:44:14Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"git-push prints a flag indicating the status of each ref that was pushed, or\nattempted to be pushed. The documentation did not correctly list all of the\npossible flags.\n---\n Documentation/git-push.txt | 13 +++++++++----\n 1 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 73a921c..61b6894 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -176,12 +176,17 @@ If --porcelain is used, then each line of the output is of the form:\n \\t : \\t ()\n -------------------------------\n \n+The status of up to date refs is shown only if --porcelain or --verbose is used.\n+\n flag::\n \tA single character indicating the status of the ref. This is\n-\tblank for a successfully pushed ref, `!` for a ref that was\n-\trejected or failed to push, and '=' for a ref that was up to\n-\tdate and did not need pushing (note that the status of up to\n-\tdate refs is shown only when `git push` is running verbosely).\n+[horizontal]\n+\t\" \" (space)::: for a successfully pushed fast-forward\n+\t\"+\"::: for a successful forced update\n+\t\"-\"::: for a successfully deleted ref\n+\t\"*\"::: for a successfully pushed new ref\n+\t\"!\"::: for a ref that was rejected or failed to push\n+\t\"=\"::: for a ref that was up to date and did not need pushing\n \n summary::\n \tFor a successfully pushed ref, the summary shows the old and new\n-- \n1.7.0.rc1.33.g07cf0f.dirty\n"},{"id":"133972","messageId":"7veikv2r50.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"1265672654-26511-1-git-send-email-larry@elder-gods.org","subject":"Re: [PATCH] git-push: fix the documentation to explain all the status flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T00:23:55Z","receivedAt":"2010-02-09T00:23:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> git-push prints a flag indicating the status of each ref that was pushed, or\n> attempted to be pushed. The documentation did not correctly list all of the\n> possible flags.\n> ---\n\nThanks.\n\nI assume it is Ok to copy your sign-off from your other messages ;-)\n\n> @@ -176,12 +176,17 @@ If --porcelain is used, then each line of the output is of the form:\n> \\t : \\t ()\n> -------------------------------\n> \n> +The status of up to date refs is shown only if --porcelain or --verbose is used.\n> +\n> flag::\n> \tA single character indicating the status of the ref. This is\n> -\tblank for a successfully pushed ref, `!` for a ref that was\n> -\trejected or failed to push, and '=' for a ref that was up to\n> -\tdate and did not need pushing (note that the status of up to\n> -\tdate refs is shown only when `git push` is running verbosely).\n> +[horizontal]\n> +\t\" \" (space)::: for a successfully pushed fast-forward\n\nBoth [horizontal] and three colons are something we never have used in the\nexisting documentation set. How confident are you that various versions\nof deployed AsciiDoc people would use all support this?\n\nI am not _complaining_; I am just being cautious to see if I have to look\ninto the issue myself (if your answer is \"not at all\") or not (otherwise).\n\n> +\t\"+\"::: for a successful forced update\n> +\t\"-\"::: for a successfully deleted ref\n> +\t\"*\"::: for a successfully pushed new ref\n> +\t\"!\"::: for a ref that was rejected or failed to push\n> +\t\"=\"::: for a ref that was up to date and did not need pushing\n"},{"id":"133973","messageId":"7vaavj2qu1.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"7veikv2r50.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-push: fix the documentation to explain all the status flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T00:30:30Z","receivedAt":"2010-02-09T00:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Both [horizontal] and three colons are something we never have used in the\n> existing documentation set. How confident are you that various versions\n> of deployed AsciiDoc people would use all support this?\n>\n> I am not _complaining_; I am just being cautious to see if I have to look\n> into the issue myself (if your answer is \"not at all\") or not (otherwise).\n\nUnfortunately, http://www.methods.co.nz/asciidoc/newlists.html labels the\nsyntax as \"new in 8.3.0\", and Debian 5.0.X and Fedora 11 both use AsciiDoc\nthat are older than that (8.2.7 and 8.2.5).\n\nI'll flatten the description into prose for now.\n\nThanks.\n"},{"id":"133976","messageId":"7vk4un1bku.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"7vaavj2qu1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-push: fix the documentation to explain all the status flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T00:45:21Z","receivedAt":"2010-02-09T00:45:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Unfortunately, http://www.methods.co.nz/asciidoc/newlists.html labels the\n> syntax as \"new in 8.3.0\", and Debian 5.0.X and Fedora 11 both use AsciiDoc\n> that are older than that (8.2.7 and 8.2.5).\n>\n> I'll flatten the description into prose for now.\n\nActually, we already use another syntax for nested enumerations, namely\ndouble-semicolon, in config.txt, so let's mimic that.\n\n-- >8 --\nSubject: git-push: document all the status flags used by --porcelain output \n\nWe didn't talk about '-' (deletion), '*' (addition), nor '+' (forced).\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-push.txt | 15 ++++++++++-----\n 1 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 73a921c..bd79119 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -176,12 +176,17 @@ If --porcelain is used, then each line of the output is of the form:\n \\t : \\t ()\n -------------------------------\n \n+The status of up-to-date refs is shown only if --porcelain or --verbose\n+option is used.\n+\n flag::\n-\tA single character indicating the status of the ref. This is\n-\tblank for a successfully pushed ref, `!` for a ref that was\n-\trejected or failed to push, and '=' for a ref that was up to\n-\tdate and did not need pushing (note that the status of up to\n-\tdate refs is shown only when `git push` is running verbosely).\n+\tA single character indicating the status of the ref:\n+(space);; for a successfully pushed fast-forward;\n+`{plus}`;; for a successful forced update;\n+`-`;; for a successfully deleted ref;\n+`*`;; for a successfully pushed new ref;\n+`!`;; for a ref that was rejected or failed to push; and\n+`=`;; for a ref that was up to date and did not need pushing.\n \n summary::\n \tFor a successfully pushed ref, the summary shows the old and new\n"},{"id":"133977","messageId":"20100209005427.GA18023@cthulhu","threadId":"22574","inReplyTo":"7veikv2r50.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-push: fix the documentation to explain all the status flags","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-09T00:54:27Z","receivedAt":"2010-02-09T00:54:27Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100208 19:24]:\n> Both [horizontal] and three colons are something we never have used in the\n> existing documentation set. How confident are you that various versions\n> of deployed AsciiDoc people would use all support this?\n> \n> I am not _complaining_; I am just being cautious to see if I have to look\n> into the issue myself (if your answer is \"not at all\") or not (otherwise).\n\nApparently three colons is really new, its only in asciidoc 8.5.3, released this\nJanuary. [horizontal] goes back to 8.3.0, from 2008.\n\n --arry\n"},{"id":"133978","messageId":"20100209005622.GB18023@cthulhu","threadId":"22574","inReplyTo":"7vk4un1bku.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-push: fix the documentation to explain all the status flags","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-09T00:56:22Z","receivedAt":"2010-02-09T00:56:22Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100208 19:45]:\n> Junio C Hamano writes:\n> \n> > Unfortunately, http://www.methods.co.nz/asciidoc/newlists.html labels the\n> > syntax as \"new in 8.3.0\", and Debian 5.0.X and Fedora 11 both use AsciiDoc\n> > that are older than that (8.2.7 and 8.2.5).\n> >\n> > I'll flatten the description into prose for now.\n> \n> Actually, we already use another syntax for nested enumerations, namely\n> double-semicolon, in config.txt, so let's mimic that.\n> \n> -- >8 --\n> Subject: git-push: document all the status flags used by --porcelain output \n\nthe flags are used by normal git-push as well\n\n --larry\n"},{"id":"133980","messageId":"7vfx5bz0hk.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100209005622.GB18023@cthulhu","subject":"Re: [PATCH] git-push: fix the documentation to explain all the status flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T01:00:55Z","receivedAt":"2010-02-09T01:00:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n>> Subject: git-push: document all the status flags used by --porcelain output \n>\n> the flags are used by normal git-push as well\n\nI noticed it after I sent it out and reworded \"s/by.*/in the output/\".\nThanks for being careful.\n"},{"id":"133995","messageId":"20100209045417.GA15210@cthulhu","threadId":"22574","inReplyTo":"7vpr4f9wey.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-09T04:54:17Z","receivedAt":"2010-02-09T04:54:17Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100208 17:48]:\n> *1* As I hinted repeatedly, I think many of them are mere churn, except\n> for \"don't advice porcelain scripts\" (good) and perhaps \"exit with failure\n> status upon only this kind of failure\" (I am undecided). \n\nWhat about the \"To\" lines? It seems that they really should go to stdout (if\n--porcelain is selected). Otherwise, how does the reader of stdout know which\nrefs got pushed to which remote?\n"},{"id":"133997","messageId":"20100209054820.GA30907@cthulhu","threadId":"22574","inReplyTo":"7vpr4f9wey.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-09T05:48:20Z","receivedAt":"2010-02-09T05:48:20Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100208 17:48]:\n> *1* As I hinted repeatedly, I think many of them are mere churn, except\n> for \"don't advice porcelain scripts\" (good) and perhaps \"exit with failure\n> status upon only this kind of failure\" (I am undecided). Especially I\n> want to see the \"send advice to the standard error stream\" one as a\n> separate patch, if you insist including that change in the series.\n\nFor some reason I thought you wanted the series squashed into one patch. Here\nit is again (to follow) with each discrete change as a single patch, and without\nmessing with \"Everything up-to-date\"\n\n --larry\n"},{"id":"134011","messageId":"7v4olqlva7.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100209045417.GA15210@cthulhu","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T07:31:44Z","receivedAt":"2010-02-09T07:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> * Junio C Hamano (gitster@pobox.com) [100208 17:48]:\n>> *1* As I hinted repeatedly, I think many of them are mere churn, except\n>> for \"don't advice porcelain scripts\" (good) and perhaps \"exit with failure\n>> status upon only this kind of failure\" (I am undecided). \n>\n> What about the \"To\" lines? It seems that they really should go to stdout (if\n> --porcelain is selected). Otherwise, how does the reader of stdout know which\n> refs got pushed to which remote?\n\nI think that probably is an improvement.\n\nWe would probably need documentation updates to cover the parsing of \"To\",\nat least; it might be cleaner to have a separate section for porcelain\nwriters. And tests. But this series won't hit next until 1.7.0 ships so\nwe have enough time to polish the details.\n\nI queued the series and once merged it to 'pu' but seeing some tests fail,\nI reverted the merge. The error is somewhat curious...\n\n$ while sh t5401-*.sh -i; do :; done\n... wait for a while ...\n* FAIL 12: send-pack stderr contains hook messages\n\n grep ^remote: send.err | sed \"s/ *\\$//\" >actual &&\n test_cmp - actual Larry D'Anna writes:\n> \n> > * Junio C Hamano (gitster@pobox.com) [100208 17:48]:\n> >> *1* As I hinted repeatedly, I think many of them are mere churn, except\n> >> for \"don't advice porcelain scripts\" (good) and perhaps \"exit with failure\n> >> status upon only this kind of failure\" (I am undecided). \n> >\n> > What about the \"To\" lines? It seems that they really should go to stdout (if\n> > --porcelain is selected). Otherwise, how does the reader of stdout know which\n> > refs got pushed to which remote?\n> \n> I think that probably is an improvement.\n> \n> We would probably need documentation updates to cover the parsing of \"To\",\n> at least; it might be cleaner to have a separate section for porcelain\n> writers. And tests. But this series won't hit next until 1.7.0 ships so\n> we have enough time to polish the details.\n> \n> I queued the series and once merged it to 'pu' but seeing some tests fail,\n> I reverted the merge. The error is somewhat curious...\n\nI bisected, and this was introduced by \n\n6d525d3 receive-pack: Send hook output over side band #2\n\n --larry\n"},{"id":"134056","messageId":"20100209175139.GA28936@spearce.org","threadId":"22574","inReplyTo":"7v4olqlva7.fsf@alter.siamese.dyndns.org","subject":"Re: t5401-update-hooks test failure","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-09T17:51:39Z","receivedAt":"2010-02-09T17:51:39Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> $ while sh t5401-*.sh -i; do :; done\n> ... wait for a while ...\n> * FAIL 12: send-pack stderr contains hook messages\n> \n> grep ^remote: send.err | sed \"s/ *\\$//\" >actual &&\n> test_cmp - actual \n> $ t/(364643e...); cat trash\\ directory.t5401-update-hooks/actual \n> remote: STDOUT pre-receive\n> remote: STDERR pre-receive\n> remote: STDOUT update refs/heads/master\n> remote: STDERR update refs/heads/master\n> remote: STDOUT update refs/heads/tofail\n> remote: STDOUT post-receive\n> remote: STDERR post-receive\n> remote: STDOUT post-update\n> remote: STDERR post-update\n> $ t/(364643e...); cat trash\\ directory.t5401-update-hooks/expect \n> remote: STDOUT pre-receive\n> remote: STDERR pre-receive\n> remote: STDOUT update refs/heads/master\n> remote: STDERR update refs/heads/master\n> remote: STDOUT update refs/heads/tofail\n> remote: STDERR update refs/heads/tofail\n> remote: STDOUT post-receive\n> remote: STDERR post-receive\n> remote: STDOUT post-update\n> remote: STDERR post-update\n\nA quick visual inspection shows that only the STDERR tofail message\nis missing here. That sounds to me like a race condition in the\nrecv_sideband decoder. Or, a race condition in the hook code in\nbuiltin-receive-pack.c.\n\nI doubt its in receive-pack. run_update_hook() directly calls the\ncopy_to_sideband() function, and that reads until EOF on the hook's\nstderr stream before it returns and waits for the hook's exit status.\nSo we should be pulling everything and dumping it into the sideband.\n\nbuiltin-send-pack.c clearly isn't stopping early while processing\nthe stream, since we see later messages from the post-receive and\npost-update hooks just fine.\n\nSo I think the only code that is in question is the case 2 arm of\nrecv_sideband(). But to be honest, I can't find any fault with it.\n\n\nI've been running this test in a while loop for a while now, and\nI can't make it trigger this failure. Maybe its possible that\nits the update hook itself, not flushing its stderr buffer before\nit terminates?\n\n-- \nShawn.\n"},{"id":"134068","messageId":"alpine.LFD.2.00.1002091337421.1681@xanadu.home","threadId":"22574","inReplyTo":"20100209175139.GA28936@spearce.org","subject":"Re: t5401-update-hooks test failure","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-09T19:20:49Z","receivedAt":"2010-02-09T19:20:49Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 9 Feb 2010, Shawn O. Pearce wrote:\n\n> Junio C Hamano wrote:\n> > $ while sh t5401-*.sh -i; do :; done\n> > ... wait for a while ...\n> > * FAIL 12: send-pack stderr contains hook messages\n> > \n> > grep ^remote: send.err | sed \"s/ *\\$//\" >actual &&\n> > test_cmp - actual > \n> > $ t/(364643e...); cat trash\\ directory.t5401-update-hooks/actual \n> > remote: STDOUT pre-receive\n> > remote: STDERR pre-receive\n> > remote: STDOUT update refs/heads/master\n> > remote: STDERR update refs/heads/master\n> > remote: STDOUT update refs/heads/tofail\n> > remote: STDOUT post-receive\n> > remote: STDERR post-receive\n> > remote: STDOUT post-update\n> > remote: STDERR post-update\n> > $ t/(364643e...); cat trash\\ directory.t5401-update-hooks/expect \n> > remote: STDOUT pre-receive\n> > remote: STDERR pre-receive\n> > remote: STDOUT update refs/heads/master\n> > remote: STDERR update refs/heads/master\n> > remote: STDOUT update refs/heads/tofail\n> > remote: STDERR update refs/heads/tofail\n> > remote: STDOUT post-receive\n> > remote: STDERR post-receive\n> > remote: STDOUT post-update\n> > remote: STDERR post-update\n> \n> A quick visual inspection shows that only the STDERR tofail message\n> is missing here. That sounds to me like a race condition in the\n> recv_sideband decoder. Or, a race condition in the hook code in\n> builtin-receive-pack.c.\n> \n> I doubt its in receive-pack. run_update_hook() directly calls the\n> copy_to_sideband() function, and that reads until EOF on the hook's\n> stderr stream before it returns and waits for the hook's exit status.\n> So we should be pulling everything and dumping it into the sideband.\n> \n> builtin-send-pack.c clearly isn't stopping early while processing\n> the stream, since we see later messages from the post-receive and\n> post-update hooks just fine.\n> \n> So I think the only code that is in question is the case 2 arm of\n> recv_sideband(). But to be honest, I can't find any fault with it.\n\nNote that strict order of messages passed through the sideband can't be \nrelied upon. Often you have sideband 1 connected to stdin and sideband \n2 connected to stderr, and they are linked with pipes, and various \nfactors such as stdio buffering or even printf implementation in the C \nlib, pipe buffers in the OS, random scheduling between the processes \ninvolved, and other factors such as disk or network speed vs CPU speed, \nare all adding to a certain degree of randomness affecting how the data \nend up in various channels wrt each other.\n\nSee my efforts to reduce that issue so far with those commits:\n\n6b59f51b312f06d9420d34c09fa408c658aac6d2\n\n6b9c42b4daabe3d0471d9f90d2ae472c9d994e1c\n\nd048a96ee9bec968be0bdc9c43ffce61169545be\n\nebe8fa738dcf6911fe520adce0cfa0cb26dee5e2\n\n\nNicolas\n"},{"id":"134070","messageId":"20100209192628.GC28936@spearce.org","threadId":"22574","inReplyTo":"alpine.LFD.2.00.1002091337421.1681@xanadu.home","subject":"Re: t5401-update-hooks test failure","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-09T19:26:28Z","receivedAt":"2010-02-09T19:26:28Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Tue, 9 Feb 2010, Shawn O. Pearce wrote:\n> > \n> > builtin-send-pack.c clearly isn't stopping early while processing\n> > the stream, since we see later messages from the post-receive and\n> > post-update hooks just fine.\n> > \n> > So I think the only code that is in question is the case 2 arm of\n> > recv_sideband(). But to be honest, I can't find any fault with it.\n> \n> Note that strict order of messages passed through the sideband can't be \n> relied upon. Often you have sideband 1 connected to stdin and sideband \n> 2 connected to stderr,\n\nOh. Sure. But that isn't really the case here.\n\nThe messages are all coming down side band #2, before we write\nanything down side band #1. The missing message in question\nshould have appeared somewhere in the middle of that side band\n#2 stream that we are seeing in the test output. Given that its\nall serialized down into a single stream by the parent process\nreceive-pack, we really shouldn't see the messages out of order.\n\n\n> and they are linked with pipes, and various \n> factors such as stdio buffering or even printf implementation in the C \n> lib\n\nThe only way I can see this missing message happening is if the C\nlibrary isn't flushing the stdio buffer before the hook process\nexits. Given that the hook process is a /bin/sh shell script,\nand its using echo to print its messages... I'm at a loss for how\nto fix that in Git.\n\nUnless its the recv_sideband() somehow skipping a line. But I\ncan't see it doing that.\n\n-- \nShawn.\n"},{"id":"134083","messageId":"7vtytqyrlk.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"032264a40d15cb9f4a86885947ffa23a603bfb0e.1265661033.git.larry@elder-gods.org","subject":"Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T22:25:11Z","receivedAt":"2010-02-09T22:25:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> @@ -1052,7 +1053,7 @@ int transport_push(struct transport *transport,\n> \t\t\tflags & TRANSPORT_PUSH_FORCE);\n> \n> \t\tret = transport->push_refs(transport, remote_refs, flags);\n> -\t\terr = push_had_errors(remote_refs);\n> +\t\terr = (pretend && porcelain) ? 0 : push_had_errors(remote_refs);\n\nHmph, you are doing (rewritten in an easier to follow format)\n\n\tif (--dry-run && --porcelain)\n\t\terr = 0;\n\telse\n\t\terr = push_add_errors(remote_refs);\n\nhere, which I think changes the semantics of what follows immediately\nafter this hunk, namely:\n\n\tif (!quiet || err)\n\t\tprint_push_status(transport->url, remote_refs,\n\t\t\t\tverbose | porcelain, porcelain,\n\t\t\t\tnonfastforward);\n\nEarlier, the logic said \"even if you asked for --quiet, we would report if\nthere is an error\" but now you are changing the rule to \"under --dry-run\nand --porcelain, --quiet means don't ever report the status, even when\nthere are errors\".\n\nI don't necessarily think it is a bad change, but in any case the semantic\nchange is worth documenting.\n"},{"id":"134088","messageId":"7vpr4eyqok.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100209192628.GC28936@spearce.org","subject":"Re: t5401-update-hooks test failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T22:44:59Z","receivedAt":"2010-02-09T22:44:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> The only way I can see this missing message happening is if the C\n> library isn't flushing the stdio buffer before the hook process\n> exits. Given that the hook process is a /bin/sh shell script,\n> and its using echo to print its messages... I'm at a loss for how\n> to fix that in Git.\n>\n> Unless its the recv_sideband() somehow skipping a line. But I\n> can't see it doing that.\n\nThe detection method of test is fooled by intermixed message.\n\nThis is what send.err has, and you grep for '^remote:' in it.\n\n-- >8 --\nwarning: updating the current branch\nwarning: Updating the currently cheremote: STDERR pre-receive \n,\nwarning: as the index and work tree do not reflect changes that are in HEAD.\nwarning: As a result, you may see the changes you just pushed into it\nwarning: reverted when you run 'git diff' over there, and you may want\nwarning: to run 'git reset --hard' before starting to work to recover.\nwarning: \nwarning: You can set 'receive.denyCurrentBranch' configuration variable to\nwarning: 'refuse' in the remote repository to forbid pushing into its\nwarning: current branch.\nwarning: To allow pushing into the current branch, you can set it to 'ignore';\nwarning: but this is not recommended unless you arranged to update its work\nwarning: tree to match what you pushed in some other way.\nwarning: \nwarning: To squelch this message, you can set it to 'warn'.\nwarning: \nwarning: Note that the default will change in a future version of git\nwarning: to refuse updating the current branch unless you have the\nwarning: configuration variable set to either 'ignore' or 'warn'.\nremote: STDOUT update refs/heads/master \nremote: STDERR update refs/heads/master \nremote: STDOUT update refs/heads/tofail \nremote: STDERR update refs/heads/tofail \nerror: hook declined to update refs/heads/tofail\nremote: STDOUT post-receive \nremote: STDERR post-receive \nremote: STDOUT post-update \nremote: STDERR post-update \nTo ./victim/.git\n d4f8e4a..b07e5b8 master -> master\n ! [remote rejected] tofail -> tofail (hook declined)\n-- 8< --\n\nBut there indeed _is_ some skipping. \"Updating the currently che\"\nis interrupted by the output from the pre-receive hook, and I do not see\nthe remainder \"cked out branch may cause confusion,\\n\" anywhere.\n"},{"id":"134091","messageId":"7vljf2xane.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"7vpr4eyqok.fsf@alter.siamese.dyndns.org","subject":"Re: t5401-update-hooks test failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T23:16:37Z","receivedAt":"2010-02-09T23:16:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oh, I hate myself ;-)\n\nThis seems to fix the test. Could somebody please explain why?\n\nI am guessing it has something to do with O_APPEND but I cannot come up\nwith a commit log message that clearly describes the fix.\n\n t/t5401-update-hooks.sh | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\nindex c3cf397..804431a 100755\n--- a/t/t5401-update-hooks.sh\n+++ b/t/t5401-update-hooks.sh\n@@ -61,8 +61,9 @@ EOF\n chmod u+x victim/.git/hooks/post-update\n \n test_expect_success push '\n+\trm -f send.out send.err &&\n \ttest_must_fail git send-pack --force ./victim/.git \\\n-\t\tmaster tofail >send.out 2>send.err\n+\t\tmaster tofail >>send.out 2>>send.err\n '\n \n test_expect_success 'updated as expected' '\n"},{"id":"134102","messageId":"20100210012935.GD28936@spearce.org","threadId":"22574","inReplyTo":"7vpr4eyqok.fsf@alter.siamese.dyndns.org","subject":"Re: t5401-update-hooks test failure","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-10T01:29:35Z","receivedAt":"2010-02-10T01:29:35Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > The only way I can see this missing message happening is if the C\n> > library isn't flushing the stdio buffer before the hook process\n> > exits. Given that the hook process is a /bin/sh shell script,\n> > and its using echo to print its messages... I'm at a loss for how\n> > to fix that in Git.\n> >\n> > Unless its the recv_sideband() somehow skipping a line. But I\n> > can't see it doing that.\n> \n> The detection method of test is fooled by intermixed message.\n> \n> This is what send.err has, and you grep for '^remote:' in it.\n> \n> -- >8 --\n> warning: updating the current branch\n> warning: Updating the currently cheremote: STDERR pre-receive \n> ,\n..\n> But there indeed _is_ some skipping. \"Updating the currently che\"\n> is interrupted by the output from the pre-receive hook, and I do not see\n> the remainder \"cked out branch may cause confusion,\\n\" anywhere.\n\nUh. I got the problem now, thanks.\n\nWhat's going on is, other messages inside of builtin-receive-pack\nare being sent to stderr, while hook output is going over the\nmultiplexed side-band through stdout, where its parsed and written\nto stderr by send-pack.\n\nWhat I missed in my patch was changing all of these other messages\ninside of receive-pack to also go over the side-band #2 if we have\nuse_sideband enabled.\n\nPatch coming in a few minutes.\n\n-- \nShawn.\n"},{"id":"134114","messageId":"20100210041313.GA21516@cthulhu","threadId":"22574","inReplyTo":"7vtytqyrlk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-10T04:13:13Z","receivedAt":"2010-02-10T04:13:13Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100209 17:25]:\n> Larry D'Anna writes:\n> \n> > @@ -1052,7 +1053,7 @@ int transport_push(struct transport *transport,\n> > \t\t\tflags & TRANSPORT_PUSH_FORCE);\n> > \n> > \t\tret = transport->push_refs(transport, remote_refs, flags);\n> > -\t\terr = push_had_errors(remote_refs);\n> > +\t\terr = (pretend && porcelain) ? 0 : push_had_errors(remote_refs);\n> \n> Hmph, you are doing (rewritten in an easier to follow format)\n> \n> \tif (--dry-run && --porcelain)\n> \t\terr = 0;\n> \telse\n> \t\terr = push_add_errors(remote_refs);\n> \n> here, which I think changes the semantics of what follows immediately\n> after this hunk, namely:\n> \n> \tif (!quiet || err)\n> \t\tprint_push_status(transport->url, remote_refs,\n> \t\t\t\tverbose | porcelain, porcelain,\n> \t\t\t\tnonfastforward);\n> \n> Earlier, the logic said \"even if you asked for --quiet, we would report if\n> there is an error\" but now you are changing the rule to \"under --dry-run\n> and --porcelain, --quiet means don't ever report the status, even when\n> there are errors\".\n> \n> I don't necessarily think it is a bad change, but in any case the semantic\n> change is worth documenting.\n\nNow that you point it out, I can't really think of any reason why this patch\nought to mess with the semantics of --quiet. \n\n --larry\n"},{"id":"134115","messageId":"1265777470-11231-1-git-send-email-larry@elder-gods.org","threadId":"22574","inReplyTo":"20100210041313.GA21516@cthulhu","subject":"[PATCH 4/4] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-10T04:51:10Z","receivedAt":"2010-02-10T04:51:10Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"The script calling git push --dry-run --porcelain can see clearly from the\noutput that the updates will be rejected. However, it will probably need to\ndistinguish this condition from the push failing for other reasons, such as the\nremote not being reachable.\n\nSigned-off-by: Larry D'Anna \n---\n builtin-send-pack.c | 4 ++++\n send-pack.h | 1 +\n transport.c | 4 +++-\n 3 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 76c7206..358f5e1 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -476,6 +476,10 @@ int send_pack(struct send_pack_args *args,\n \n \tif (ret < 0)\n \t\treturn ret;\n+\n+\tif (args->porcelain && args->dry_run)\n+\t\treturn 0;\n+\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\ndiff --git a/send-pack.h b/send-pack.h\nindex 28141ac..60b4ba6 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -4,6 +4,7 @@\n struct send_pack_args {\n \tunsigned verbose:1,\n \t\tquiet:1,\n+\t\tporcelain:1,\n \t\tsend_mirror:1,\n \t\tforce_update:1,\n \t\tuse_thin_pack:1,\ndiff --git a/transport.c b/transport.c\nindex fb653c6..31f2e84 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -791,6 +791,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re\n \targs.verbose = !!(flags & TRANSPORT_PUSH_VERBOSE);\n \targs.quiet = !!(flags & TRANSPORT_PUSH_QUIET);\n \targs.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);\n+\targs.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);\n \n \tret = send_pack(&args, data->fd, data->conn, remote_refs,\n \t\t\t&data->extra_have);\n@@ -1054,7 +1055,8 @@ int transport_push(struct transport *transport,\n \t\tret = transport->push_refs(transport, remote_refs, flags);\n \t\terr = push_had_errors(remote_refs);\n \n-\t\tret |= err;\n+\t\tif ( !(pretend && porcelain) )\n+\t\t\tret |= err;\n \n \t\tif (!quiet || err)\n \t\t\tprint_push_status(transport->url, remote_refs,\n-- \n1.7.0.rc1.33.g07cf0f.dirty\n"},{"id":"134123","messageId":"20100210053937.GF28526@coredump.intra.peff.net","threadId":"22574","inReplyTo":"7viqa7cqs9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-10T05:39:37Z","receivedAt":"2010-02-10T05:39:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 02:21:26PM -0800, Junio C Hamano wrote:\n\n> I think the purpose of the patches that started this thread was to admit\n> that 1965ff7 (add --porcelain option to git-push, 2009-06-22) was not well\n> thought out, and to break compatibility to fix it.\n\nI thought it was simply to remove stdout cruft that had crept in, and at\nthe same time to remove some stderr cruft that was simply noise on the\nterminal. That being said, I am in favor of fixing it even if it means a\nslight compatibility breakage.\n\n> Having said that, I would say that what 1965ff7 specified was only these\n> two:\n> \n> = TAB refs/heads/master:refs/heads/master TAB [up to date]\n> - TAB :refs/heads/foobar TAB [deleted]\n> \n> so everything else that do not match this pattern is a fair game, most\n> importantly, the line that begins with \"To\" would not be mistaken with\n> this pattern, I think.\n\nIt depends on the parser. If the parser was something like:\n\n switch (line[0]) {\n case '=': ...; break;\n case '-': ...; break;\n default: die(\"wtf: %s\", line);\n }\n\nthen we are not introducing any ambiguity, but we are causing a\nbreakage. The problem is that we did not specify the format anywhere, so\nit is hard to say whether we are breaking any promises we made.\n\n> > This one, on the other hand, seems to me to be just noise. What does a\n> > --porcelain caller learn by seeing \"Everything up-to-date\" that it did\n> > not already know from seeing the list of refs?\n> \n> I do not care too much about this hunk either way. We could leave it as\n> is, as we will be giving some other stuff to the standard error stream\n> without squelching anyway, even with the three-patch series. We could\n> squelch only this message, but it is dubious what it is buying us. If you\n> forced me to decide, I would probably say \"let's just drop this hunk and\n> keep the code as-is\".\n\nI have not actually been running these patches, just reading them, but\nmy impression was the goal _was_ to squelch all of the stderr cruft. But\nif we are not even close, then probably we should just give up and\ncallers should \"2>/dev/null\".\n\n> As to the exit status, do you have any thoughts, by the way?\n> \n> I am not convinced that it would be necessary nor even a good idea to make\n> it behave inconsistently between the normal case and Porcelain case, only\n> to make it easier to special case the \"remote side would reject due to\n> non-fast-forward\" failure mode (iow, even if the calling script knows that\n> it would fail due to non-fast-forward but otherwise everything else would\n> be fine, what good would it do?)\n\nI had initially endorsed it, but now I am having second thoughts.\nEspecially if the \"usual\" calling convention is to redirect stderr as\nabove, then we are probably missing out on any useful error messages\nthat accompany a failure return, anyway. So maybe the sane thing to do\nis to leave the exit code alone, and include a --porcelain output line\nthat either says \"Everything was OK, see individual ref status\" or \"We\ncouldn't even talk to the other side\". Then the status code is\nirrelevant, and stdout contains all of the useful information (and if\nyou don't get an error or OK message, you know there was some\nserious error like a broken git installation).\n\n-Peff\n"},{"id":"134124","messageId":"20100210054957.GG28526@coredump.intra.peff.net","threadId":"22574","inReplyTo":"7vmxzj8hca.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-10T05:49:57Z","receivedAt":"2010-02-10T05:49:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 02:59:01PM -0800, Junio C Hamano wrote:\n\n> >> +\tif (flags & TRANSPORT_PUSH_PORCELAIN) {\n> >> +\t\t/* Do not give advice messages to Porcelain scripts */\n> >> +\t\tadvice_push_nonfastforward = 0;\n> >> +\t}\n> >\n> > I think this is sane.\n> \n> I am tempted to suggest adding \"clear_advice(void)\" in advice.[ch], so\n> that people adding new advices do not have to hunt for even the above\n> hunk. It would be a good direction to go in general _if_ we will have\n> more like this --porcelain thing in other parts of the system.\n> \n> I didn't do so because that \"_if_\" is still iffy.\n\nWould it make sense to clear _all_ advice if we are just in porcelain\nmode for git-push? That is, let's say I am in \"push --porcelain\" and I\ntry to write a reflog entry for a local tracking ref, but my identity is\nimplicitly picked up from the hostname[1]. Should it trigger\nadvice_implicit_identity and say \"by the way, your identity is implicit\"\non stderr or not?\n\nI would say yes. The advice output should all be on stderr, and the\nporcelain output should all be on stdout. So there is no parsing\nconflict. And stderr either goes to /dev/null (because the porcelain is\nnot terminal-based, or doesn't want the terminal screwed up), in which\ncase the advice does nothing, or stderr goes to the terminal (because\nthe porcelain is some simple script), in which case the message is\nprobably something the user would want to see.\n\n-Peff\n\n[1] I am actually not sure if you can trigger the implicit identity\nadvice in this way. But I am arguing from the perspective of \"assume you\nhave triggered some random advice through some sub-action\", which may\ninclude advice to be added in the future.\n"},{"id":"134125","messageId":"20100210055529.GA1566@cthulhu","threadId":"22574","inReplyTo":"20100210053937.GF28526@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-10T05:55:29Z","receivedAt":"2010-02-10T05:55:29Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Jeff King (peff@peff.net) [100210 00:41]:\n> I have not actually been running these patches, just reading them, but\n> my impression was the goal _was_ to squelch all of the stderr cruft. But\n> if we are not even close, then probably we should just give up and\n> callers should \"2>/dev/null\".\n\nPersonally, I don't really care about squelching stderr cruft. All I really\nwant is for what goes to stdout be sane and the calling script to be able to\nunambiguously figure out what happened, including\n\n* which refs go to which remotes \n\n* whether or not some mysterious error occurred (beyond those mentioned in the\n ref status lines)\n\n> I had initially endorsed it, but now I am having second thoughts.\n> Especially if the \"usual\" calling convention is to redirect stderr as\n> above, then we are probably missing out on any useful error messages\n> that accompany a failure return, anyway. So maybe the sane thing to do\n> is to leave the exit code alone, and include a --porcelain output line\n> that either says \"Everything was OK, see individual ref status\" or \"We\n> couldn't even talk to the other side\". Then the status code is\n> irrelevant, and stdout contains all of the useful information (and if\n> you don't get an error or OK message, you know there was some\n> serious error like a broken git installation).\n\nThat serves my purposes as well as the exit code would. Is this the consensus?\n\n --larry\n"},{"id":"134136","messageId":"be6fef0d1002100243l52d1cbd4x5ab01283d54c5e9e@mail.gmail.com","threadId":"22574","inReplyTo":"20100210055529.GA1566@cthulhu","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-02-10T10:43:39Z","receivedAt":"2010-02-10T10:43:39Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Wed, Feb 10, 2010 at 1:55 PM, Larry D'Anna wrote:\n> * Jeff King (peff@peff.net) [100210 00:41]:\n>> I had initially endorsed it, but now I am having second thoughts.\n>> Especially if the \"usual\" calling convention is to redirect stderr as\n>> above, then we are probably missing out on any useful error messages\n>> that accompany a failure return, anyway.  So maybe the sane thing to do\n>> is to leave the exit code alone, and include a --porcelain output line\n>> that either says \"Everything was OK, see individual ref status\" or \"We\n>> couldn't even talk to the other side\". Then the status code is\n>> irrelevant, and stdout contains all of the useful information (and if\n>> you don't get an error or OK message, you know there was some\n>> serious error like a broken git installation).\n>\n> That serves my purposes as well as the exit code would.  Is this the consensus?\n\ndoes this mean the ugly return status mangling patch (git-push: make\ngit push --dry-run --porcelain exit with status 0 even if updates will\nbe rejected) would be dropped? If so, I'm all for it. =p\n\n-- \nCheers,\nRay Chuan\n"},{"id":"134276","messageId":"7vwryjpdb6.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100210054957.GG28526@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-11T23:23:09Z","receivedAt":"2010-02-11T23:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Would it make sense to clear _all_ advice if we are just in porcelain\n> mode for git-push? That is, let's say I am in \"push --porcelain\" and I\n> try to write a reflog entry for a local tracking ref, but my identity is\n> implicitly picked up from the hostname[1]. Should it trigger\n> advice_implicit_identity and say \"by the way, your identity is implicit\"\n> on stderr or not?\n>\n> I would say yes. The advice output should all be on stderr, and the\n> porcelain output should all be on stdout. So there is no parsing\n> conflict. And stderr either goes to /dev/null (because the porcelain is\n> not terminal-based, or doesn't want the terminal screwed up), in which\n> case the advice does nothing, or stderr goes to the terminal (because\n> the porcelain is some simple script), in which case the message is\n> probably something the user would want to see.\n\nHmm, if we make it a rule to send all the advice messages to the standard\nerror stream, wouldn't the logical conclusion be that the hunk you are\ncommenting is unnecessary? \"push --porcelain\" should give advice just as\nusual (but we would simply send the advice to the standard error stream).\n"},{"id":"134278","messageId":"20100212000302.GB21930@coredump.intra.peff.net","threadId":"22574","inReplyTo":"7vwryjpdb6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] git-push: clean up some of the output from git push --porcelain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-12T00:03:02Z","receivedAt":"2010-02-12T00:03:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 11, 2010 at 03:23:09PM -0800, Junio C Hamano wrote:\n\n> > I would say yes. The advice output should all be on stderr, and the\n> > porcelain output should all be on stdout. So there is no parsing\n> > conflict. And stderr either goes to /dev/null (because the porcelain is\n> > not terminal-based, or doesn't want the terminal screwed up), in which\n> > case the advice does nothing, or stderr goes to the terminal (because\n> > the porcelain is some simple script), in which case the message is\n> > probably something the user would want to see.\n> \n> Hmm, if we make it a rule to send all the advice messages to the standard\n> error stream, wouldn't the logical conclusion be that the hunk you are\n> commenting is unnecessary? \"push --porcelain\" should give advice just as\n> usual (but we would simply send the advice to the standard error stream).\n\nYes and no. The advice it is giving is specifically related to things\nyou can't see, so it is not really as helpful as it could be. And in\nthat sense, squelching may be positive.\n\nBut it seems that the thread has come to the conclusion that we're still\ngoing to have messages on stderr. There is not much point in suppressing\none particular message if there is a bunch of other cruft. Either the\nuser should see stuff on stderr, or they shouldn't, and the caller can\ndecide by redirecting stderr. So yes, squelching that advice probably\nshould just be dropped.\n\n-Peff\n"},{"id":"134650","messageId":"20100215174013.GA12761@cthulhu","threadId":"22574","inReplyTo":"7vtytqyrlk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-15T17:40:13Z","receivedAt":"2010-02-15T17:40:13Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100209 17:25]:\n> Larry D'Anna writes:\n> \n> > @@ -1052,7 +1053,7 @@ int transport_push(struct transport *transport,\n> > \t\t\tflags & TRANSPORT_PUSH_FORCE);\n> > \n> > \t\tret = transport->push_refs(transport, remote_refs, flags);\n> > -\t\terr = push_had_errors(remote_refs);\n> > +\t\terr = (pretend && porcelain) ? 0 : push_had_errors(remote_refs);\n> \n> Hmph, you are doing (rewritten in an easier to follow format)\n> \n> \tif (--dry-run && --porcelain)\n> \t\terr = 0;\n> \telse\n> \t\terr = push_add_errors(remote_refs);\n> \n> here, which I think changes the semantics of what follows immediately\n> after this hunk, namely:\n> \n> \tif (!quiet || err)\n> \t\tprint_push_status(transport->url, remote_refs,\n> \t\t\t\tverbose | porcelain, porcelain,\n> \t\t\t\tnonfastforward);\n> \n> Earlier, the logic said \"even if you asked for --quiet, we would report if\n> there is an error\" but now you are changing the rule to \"under --dry-run\n> and --porcelain, --quiet means don't ever report the status, even when\n> there are errors\".\n> \n> I don't necessarily think it is a bad change, but in any case the semantic\n> change is worth documenting.\n\nWhich version of the --quiet behavior do you want for the next version of this\nseries? My feeling is that \"even if you asked for --quiet, we would report if\nthere is an error\" is the best policy, even under --dry-run --porcelain.\n\n --larry\n"},{"id":"134661","messageId":"7vmxzaxmc6.fsf@alter.siamese.dyndns.org","threadId":"22574","inReplyTo":"20100215174013.GA12761@cthulhu","subject":"Re: [PATCH v3 3/3] git-push: make git push --dry-run --porcelain exit with status 0 even if updates will be rejected","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-15T20:42:17Z","receivedAt":"2010-02-15T20:42:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> Which version of the --quiet behavior do you want for the next version\n> of this series? My feeling is that \"even if you asked for --quiet, we\n> would report if there is an error\" is the best policy, even under\n> --dry-run --porcelain.\n\nI think that is probably a sane and the least-surprising thing to do from\nthe end user's point of view.\n"}]}