{"thread":{"id":"31618","subject":"[PATCH 0/3] nicer receive-pack errors over http","startedAt":"2012-09-21T05:30:57Z","lastAt":"2012-09-21T17:40:00Z","messageCount":8,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"199640","messageId":"20120921053057.GA9768@sigill.intra.peff.net","threadId":"31618","inReplyTo":null,"subject":"[PATCH 0/3] nicer receive-pack errors over http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T05:30:57Z","receivedAt":"2012-09-21T05:30:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While we are on the subject of http user experience, I thought I'd\nmention this patch to route more errors from index-pack back to the\nuser. We're not doing it yet at GitHub, but I plan to apply it soon.\n\nThe first patch is a cleanup and minor bug fix. The second is the\ninteresting one. The third is purely cosmetic, and doesn't need to be\ntied to the others.\n\n [1/3]: receive-pack: redirect unpack-objects stdout to /dev/null\n [2/3]: receive-pack: send pack-processing stderr over sideband\n [3/3]: receive-pack: drop \"n/a\" on unpacker errors\n\n-Peff\n"},{"id":"199641","messageId":"20120921053252.GA9863@sigill.intra.peff.net","threadId":"31618","inReplyTo":"20120921053057.GA9768@sigill.intra.peff.net","subject":"[PATCH 1/3] receive-pack: redirect unpack-objects stdout to /dev/null","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T05:32:52Z","receivedAt":"2012-09-21T05:32:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The unpack-objects command should not generally produce any\noutput on stdout. However, if it's given extra input after\nthe packfile, it will spew the remainder to stdout. When\ncalled by receive-pack, this means we will break protocol,\nsince our stdout is connected to the remote send-pack.\n\nSigned-off-by: Jeff King \n---\nI've never actually seen this bug in practice, but I noticed it while\nauditing the outputs of receive-pack's children recently.\n\n builtin/receive-pack.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9145f1a..5ba0c98 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -815,6 +815,7 @@ static const char *unpack(void)\n \n \tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n \t\tint code, i = 0;\n+\t\tstruct child_process child;\n \t\tconst char *unpacker[5];\n \t\tunpacker[i++] = \"unpack-objects\";\n \t\tif (quiet)\n@@ -823,7 +824,11 @@ static const char *unpack(void)\n \t\t\tunpacker[i++] = \"--strict\";\n \t\tunpacker[i++] = hdr_arg;\n \t\tunpacker[i++] = NULL;\n-\t\tcode = run_command_v_opt(unpacker, RUN_GIT_CMD);\n+\t\tmemset(&child, 0, sizeof(child));\n+\t\tchild.argv = unpacker;\n+\t\tchild.no_stdout = 1;\n+\t\tchild.git_cmd = 1;\n+\t\tcode = run_command(&child);\n \t\tif (!code)\n \t\t\treturn NULL;\n \t\treturn \"unpack-objects abnormal exit\";\n-- \n1.7.11.7.15.g085c6bd\n"},{"id":"199642","messageId":"20120921053455.GB9863@sigill.intra.peff.net","threadId":"31618","inReplyTo":"20120921053057.GA9768@sigill.intra.peff.net","subject":"[PATCH 2/3] receive-pack: send pack-processing stderr over sideband","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T05:34:55Z","receivedAt":"2012-09-21T05:34:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Receive-pack invokes either unpack-objects or index-pack to\nhandle the incoming pack. However, we do not redirect the\nstderr of the sub-processes at all, so it is never seen by\nthe client. From the initial thread adding sideband support,\nwhich is here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/139471\n\nit is clear that some messages are specifically kept off the\nsideband (with the assumption that they are of interest only\nto an administrator, not the client). The stderr of the\nsubprocesses is mentioned in the thread, but it's unclear if\nthey are included in that group, or were simply forgotten.\n\nHowever, there are a few good reasons to show them to the\nclient:\n\n 1. In many cases, they are directly about the incoming\n packfile (e.g., fsck warnings with --strict, corruption\n in the packfile, etc). Without these messages, the\n client just gets \"unpacker error\" with no extra useful\n diagnosis.\n\n 2. No matter what the cause, we are probably better off\n showing the errors to the client. If the client and the\n server admin are not the same entity, it is probably\n much easier for the client to cut-and-paste the errors\n they see than for the admin to try to dig them out of a\n log and correlate them with a particular session.\n\n 3. Users of the ssh transport typically already see these\n stderr messages, as the remote's stderr is copied\n literally by ssh. This brings other transports (http,\n and push-over-git if you are crazy enough to enable it)\n more in line with ssh. As a bonus for ssh users,\n because the messages are now fed through the sideband\n and printed by the local git, they will have \"remote:\"\n prepended and be properly interleaved with any local\n output to stderr.\n\nSigned-off-by: Jeff King \n---\nThis one is logically independent of the first patch, but relies\ntextually on the conversion of unpack-objects to use a separate\nchild_process.\n\n builtin/receive-pack.c | 26 ++++++++++++++++++++++++--\n 1 file changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 5ba0c98..ac679ab 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -795,7 +795,7 @@ static const char *pack_lockfile;\n \n static const char *pack_lockfile;\n \n-static const char *unpack(void)\n+static const char *unpack(int err_fd)\n {\n \tstruct pack_header hdr;\n \tconst char *hdr_err;\n@@ -827,6 +827,7 @@ static const char *unpack(void)\n \t\tmemset(&child, 0, sizeof(child));\n \t\tchild.argv = unpacker;\n \t\tchild.no_stdout = 1;\n+\t\tchild.err = err_fd;\n \t\tchild.git_cmd = 1;\n \t\tcode = run_command(&child);\n \t\tif (!code)\n@@ -853,6 +854,7 @@ static const char *unpack(void)\n \t\tmemset(&ip, 0, sizeof(ip));\n \t\tip.argv = keeper;\n \t\tip.out = -1;\n+\t\tip.err = err_fd;\n \t\tip.git_cmd = 1;\n \t\tstatus = start_command(&ip);\n \t\tif (status) {\n@@ -869,6 +871,26 @@ static const char *unpack(void)\n \t}\n }\n \n+static const char *unpack_with_sideband(void)\n+{\n+\tstruct async muxer;\n+\tconst char *ret;\n+\n+\tif (!use_sideband)\n+\t\treturn unpack(0);\n+\n+\tmemset(&muxer, 0, sizeof(muxer));\n+\tmuxer.proc = copy_to_sideband;\n+\tmuxer.in = -1;\n+\tif (start_async(&muxer))\n+\t\treturn NULL;\n+\n+\tret = unpack(muxer.in);\n+\n+\tfinish_async(&muxer);\n+\treturn ret;\n+}\n+\n static void report(struct command *commands, const char *unpack_status)\n {\n \tstruct command *cmd;\n@@ -966,7 +988,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tconst char *unpack_status = NULL;\n \n \t\tif (!delete_only(commands))\n-\t\t\tunpack_status = unpack();\n+\t\t\tunpack_status = unpack_with_sideband();\n \t\texecute_commands(commands, unpack_status);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n-- \n1.7.11.7.15.g085c6bd\n"},{"id":"199643","messageId":"20120921053846.GC9863@sigill.intra.peff.net","threadId":"31618","inReplyTo":"20120921053057.GA9768@sigill.intra.peff.net","subject":"[PATCH 3/3] receive-pack: drop \"n/a\" on unpacker errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T05:38:46Z","receivedAt":"2012-09-21T05:38:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The output from git push currently looks like this:\n\n $ git push dest HEAD\n fatal: [some message from index-pack]\n error: unpack failed: index-pack abnormal exit\n To dest\n ! [remote rejected] HEAD -> master (n/a (unpacker error))\n\nThat n/a is meant to be \"the per-ref status is not\navailable\" but the nested parentheses just make it look\nugly. Let's turn the final line into just:\n\n ! [remote rejected] HEAD -> master (unpacker error)\n\nSigned-off-by: Jeff King \n---\nMaybe it is just me, but I have always found the \"n/a\" and extra\nparentheses ugly and unnecessary. But obviously others may differ.\nIt doesn't really come up that often, since index-pack failing usually\nimplies a git bug. But with transfer.fsckObjects turn on, it is more\ncommon.\n\nI don't think there should be any backwards compatibility issues with\nchanging this. The \"reason\" field sent back by receive-pack has always\nbeen a free-form human-readable string.\n\nI also dislike the \"index-pack abnormal exit\" message. Again, when\nindex-pack really crashes, it's fine, but it can die due to bogus\nobjects, too, in which case it might be nice to have a more\nhuman-readable message.\n\n builtin/receive-pack.c | 2 +-\n t/t5504-fetch-receive-strict.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ac679ab..ff781fe 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -695,7 +695,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tif (unpacker_error) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next)\n-\t\t\tcmd->error_string = \"n/a (unpacker error)\";\n+\t\t\tcmd->error_string = \"unpacker error\";\n \t\treturn;\n \t}\n \ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 35ec294..69ee13c 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -89,7 +89,7 @@ To dst\n \n cat >exp < writes:\n\n> Receive-pack invokes either unpack-objects or index-pack to\n> handle the incoming pack. However, we do not redirect the\n> stderr of the sub-processes at all, so it is never seen by\n> the client. From the initial thread adding sideband support,\n> which is here:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/139471\n>\n> it is clear that some messages are specifically kept off the\n> sideband (with the assumption that they are of interest only\n> to an administrator, not the client). The stderr of the\n> subprocesses is mentioned in the thread, but it's unclear if\n> they are included in that group, or were simply forgotten.\n>\n> However, there are a few good reasons to show them to the\n> client:\n>\n> 1. In many cases, they are directly about the incoming\n> packfile (e.g., fsck warnings with --strict, corruption\n> in the packfile, etc). Without these messages, the\n> client just gets \"unpacker error\" with no extra useful\n> diagnosis.\n>\n> 2. No matter what the cause, we are probably better off\n> showing the errors to the client. If the client and the\n> server admin are not the same entity, it is probably\n> much easier for the client to cut-and-paste the errors\n> they see than for the admin to try to dig them out of a\n> log and correlate them with a particular session.\n\nI agree with the \"probably\" above (and also with points 1 and 3),\nbut it at the same time feel a bit iffy. The server side would lose\nlog entries to check when the operator observes higher error rate\nand starts suspecting something recently broke, and the lost clue\ncannot be recovered without contacting the pushers, no?\n\n> 3. Users of the ssh transport typically already see these\n> stderr messages, as the remote's stderr is copied\n> literally by ssh. This brings other transports (http,\n> and push-over-git if you are crazy enough to enable it)\n> more in line with ssh. As a bonus for ssh users,\n> because the messages are now fed through the sideband\n> and printed by the local git, they will have \"remote:\"\n> prepended and be properly interleaved with any local\n> output to stderr.\n>\n> Signed-off-by: Jeff King \n> ---\n\nWill queue; thanks.\n"},{"id":"199670","messageId":"20120921170539.GA19707@sigill.intra.peff.net","threadId":"31618","inReplyTo":"7v7grn8gnv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] receive-pack: send pack-processing stderr over sideband","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T17:05:39Z","receivedAt":"2012-09-21T17:05:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 21, 2012 at 09:49:40AM -0700, Junio C Hamano wrote:\n\n> > 2. No matter what the cause, we are probably better off\n> > showing the errors to the client. If the client and the\n> > server admin are not the same entity, it is probably\n> > much easier for the client to cut-and-paste the errors\n> > they see than for the admin to try to dig them out of a\n> > log and correlate them with a particular session.\n> \n> I agree with the \"probably\" above (and also with points 1 and 3),\n> but it at the same time feel a bit iffy. The server side would lose\n> log entries to check when the operator observes higher error rate\n> and starts suspecting something recently broke, and the lost clue\n> cannot be recovered without contacting the pushers, no?\n\nYeah, that is true, although that is already the case with ssh pushes.\nConversely, it also means that servers using the ssh transport have lost\nthe option of redirecting the server-side stderr (e.g., with a wrapper\naround git-receive-pack) to a log if they were already doing so.\n\nHowever, this does make things more consistent with upload-pack, which\nconnects the stderr of pack-objects to sideband (which it must to handle\nprogress). Furthermore, many of the messages from receive-pack are\nhandled by rp_error, which sends to the sideband. So if you were\nmonitoring your git purely by trying to capture stderr, you were already\nonly getting a fraction of the real data.\n\nIf a server side really did want to capture the error output, I think\nthe right way to do it would be:\n\n 1. Modify send_sideband to send a copy of all band-2 data to stderr.\n\n 2. Redirect stderr from all processes to a log processor (it's\n tempting to just send it somewhere besides stderr in (1) above, but\n you may also get clients which do not claim to support sidebands,\n in which case we will just spew to stderr).\n\n 3. Do immediate culling on the output before storage, because some of\n what you get will be junk like progress reports (and they all come\n down the same fd, since it is typically stderr from a subprocess).\n\nSo I think this patch is not really making anything _worse_ if somebody\nwanted to do that. It just moves the messages from being caught by step\n(2) to being caught by step (1). But you have to do both either way.\n\n-Peff\n"},{"id":"199674","messageId":"7vtxur70fv.fsf@alter.siamese.dyndns.org","threadId":"31618","inReplyTo":"20120921170539.GA19707@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] receive-pack: send pack-processing stderr over sideband","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-21T17:25:24Z","receivedAt":"2012-09-21T17:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Sep 21, 2012 at 09:49:40AM -0700, Junio C Hamano wrote:\n>\n>> > 2. No matter what the cause, we are probably better off\n>> > showing the errors to the client. If the client and the\n>> > server admin are not the same entity, it is probably\n>> > much easier for the client to cut-and-paste the errors\n>> > they see than for the admin to try to dig them out of a\n>> > log and correlate them with a particular session.\n>> \n>> I agree with the \"probably\" above (and also with points 1 and 3),\n>> but it at the same time feel a bit iffy. The server side would lose\n>> log entries to check when the operator observes higher error rate\n>> and starts suspecting something recently broke, and the lost clue\n>> cannot be recovered without contacting the pushers, no?\n>\n> Yeah, that is true, although that is already the case with ssh pushes.\n> Conversely, it also means that servers using the ssh transport have lost\n> the option of redirecting the server-side stderr (e.g., with a wrapper\n> around git-receive-pack) to a log if they were already doing so.\n\nYes.\n\n> However, this does make things more consistent with upload-pack, which\n> connects the stderr of pack-objects to sideband (which it must to handle\n> progress). Furthermore, many of the messages from receive-pack are\n> handled by rp_error, which sends to the sideband. So if you were\n> monitoring your git purely by trying to capture stderr, you were already\n> only getting a fraction of the real data.\n\nThe comments were not meant as a rejection notice ;-) Just to see if\nsome server operators have input on the matter.\n\nI personally do not think tee-ing the error output is worth it; it\nwould be reasonably simple to arrange, and the server operators who\nwant it can ask later if that is need.\n\nThanks.\n"},{"id":"199677","messageId":"20120921174000.GA20896@sigill.intra.peff.net","threadId":"31618","inReplyTo":"7vtxur70fv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] receive-pack: send pack-processing stderr over sideband","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T17:40:00Z","receivedAt":"2012-09-21T17:40:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 21, 2012 at 10:25:24AM -0700, Junio C Hamano wrote:\n\n> > However, this does make things more consistent with upload-pack, which\n> > connects the stderr of pack-objects to sideband (which it must to handle\n> > progress). Furthermore, many of the messages from receive-pack are\n> > handled by rp_error, which sends to the sideband. So if you were\n> > monitoring your git purely by trying to capture stderr, you were already\n> > only getting a fraction of the real data.\n> \n> The comments were not meant as a rejection notice ;-) Just to see if\n> some server operators have input on the matter.\n\nI know. But your comment made me second-guess a little whether\nanybody would be inadvertently hurt, but thinking it through and writing\nit out helped convince myself that it's the right thing to do.\n\nOne of the hardest parts of working on a mature software project is not\njust thinking about what you want to do, but thinking about what\neveryone else wants to do (or is doing). So it never hurts to\ndouble-check your assumptions in such a case, and I don't mind working\nthrough these \"what ifs\" even if they end in us doing the original\nthing. I hope you do not mind reading them too much. :)\n\n> I personally do not think tee-ing the error output is worth it; it\n> would be reasonably simple to arrange, and the server operators who\n> want it can ask later if that is need.\n\nAgreed.\n\n-Peff\n"}]}