{"thread":{"id":"45305","subject":"[PATCH 1/6] receive-pack: fix deadlock when we cannot create tmpdir","startedAt":"2017-03-07T13:42:59Z","lastAt":"2017-03-08T17:58:52Z","messageCount":12,"participants":["Jeff King","Junio C Hamano","Horst Schirmeier"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"313410","messageId":"20170307133534.bgar57ojq52snns3@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"[PATCH 1/6] receive-pack: fix deadlock when we cannot create tmpdir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:35:34Z","receivedAt":"2017-03-07T13:42:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The err_fd descriptor passed to the unpack() function is\nintended to be handed off to the child index-pack, and our\nasync muxer will read until it gets EOF. However, if we\nencounter an error before handing off the descriptor, we\nmust manually close(err_fd). Otherwise we will be waiting\nfor our muxer to finish, while the muxer is waiting for EOF\non err_fd.\n\nWe fixed an identical deadlock already in 49ecfa13f\n(receive-pack: close sideband fd on early pack errors,\n2013-04-19). But since then, the function grew a new\nearly-return in 722ff7f87 (receive-pack: quarantine objects\nuntil pre-receive accepts, 2016-10-03), when we fail to\ncreate a temporary directory. This return needs the same\ntreatment.\n\nReported-by: Horst Schirmeier \nSigned-off-by: Jeff King \n---\n builtin/receive-pack.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9ed8fbbfa..f2c6953a3 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1667,8 +1667,11 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t}\n \n \ttmp_objdir = tmp_objdir_create();\n-\tif (!tmp_objdir)\n+\tif (!tmp_objdir) {\n+\t\tif (err_fd > 0)\n+\t\t\tclose(err_fd);\n \t\treturn \"unable to create temporary object directory\";\n+\t}\n \tchild.env = tmp_objdir_env(tmp_objdir);\n \n \t/*\n-- \n2.12.0.429.gde83c8049\n\n"},{"id":"313411","messageId":"20170307133557.xoqv6kigxzakgysi@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"[PATCH 2/6] send-pack: extract parsing of \"unpack\" response","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:35:57Z","receivedAt":"2017-03-07T13:43:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After sending the pack, we call receive_status() which gets\nboth the \"unpack\" line and the ref status. Let's break these\ninto two functions so we can call the first part\nindependently.\n\nSigned-off-by: Jeff King \n---\n send-pack.c | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 6195b43e9..12e229e44 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -130,22 +130,27 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n \treturn 0;\n }\n \n-static int receive_status(int in, struct ref *refs)\n+static int receive_unpack_status(int in)\n {\n-\tstruct ref *hint;\n-\tint ret = 0;\n-\tchar *line = packet_read_line(in, NULL);\n+\tconst char *line = packet_read_line(in, NULL);\n \tif (!starts_with(line, \"unpack \"))\n \t\treturn error(\"did not receive remote status\");\n-\tif (strcmp(line, \"unpack ok\")) {\n-\t\terror(\"unpack failed: %s\", line + 7);\n-\t\tret = -1;\n-\t}\n+\tif (strcmp(line, \"unpack ok\"))\n+\t\treturn error(\"unpack failed: %s\", line + 7);\n+\treturn 0;\n+}\n+\n+static int receive_status(int in, struct ref *refs)\n+{\n+\tstruct ref *hint;\n+\tint ret;\n+\n \thint = NULL;\n+\tret = receive_unpack_status(in);\n \twhile (1) {\n \t\tchar *refname;\n \t\tchar *msg;\n-\t\tline = packet_read_line(in, NULL);\n+\t\tchar *line = packet_read_line(in, NULL);\n \t\tif (!line)\n \t\t\tbreak;\n \t\tif (!starts_with(line, \"ok \") && !starts_with(line, \"ng \")) {\n-- \n2.12.0.429.gde83c8049\n\n"},{"id":"313413","messageId":"20170307133948.x3w6nufae7tbgeka@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"[PATCH 6/6] send-pack: report signal death of pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:39:48Z","receivedAt":"2017-03-07T13:48:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If our pack-objects sub-process dies of a signal, then it\nlikely didn't have a chance to write anything useful to\nstderr. The user may be left scratching their head why the\npush failed. Let's detect this situation and write something\nto stderr.\n\nSigned-off-by: Jeff King \n---\nWe could drop the SIGPIPE special-case, but I think it's just noise\nafter the unpack-status fix in the previous commit.\n\n send-pack.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex e15232739..d2d2a49a0 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -72,6 +72,7 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n \tstruct child_process po = CHILD_PROCESS_INIT;\n \tFILE *po_in;\n \tint i;\n+\tint rc;\n \n \ti = 4;\n \tif (args->use_thin_pack)\n@@ -125,8 +126,20 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n \t\tpo.out = -1;\n \t}\n \n-\tif (finish_command(&po))\n+\trc = finish_command(&po);\n+\tif (rc) {\n+\t\t/*\n+\t\t * For a normal non-zero exit, we assume pack-objects wrote\n+\t\t * something useful to stderr. For death by signal, though,\n+\t\t * we should mention it to the user. The exception is SIGPIPE\n+\t\t * (141), because that's a normal occurence if the remote end\n+\t\t * hangs up (and we'll report that by trying to read the unpack\n+\t\t * status).\n+\t\t */\n+\t\tif (rc > 128 && rc != 141)\n+\t\t\terror(\"pack-objects died of signal %d\", rc - 128);\n \t\treturn -1;\n+\t}\n \treturn 0;\n }\n \n-- \n2.12.0.429.gde83c8049\n"},{"id":"313415","messageId":"20170307133850.aaykcpuzlzqecz7x@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"[PATCH 5/6] send-pack: read \"unpack\" status even on pack-objects failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:38:51Z","receivedAt":"2017-03-07T14:06:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the local pack-objects of a push fails, we'll tell the\nuser about it. But one likely cause is that the remote\nindex-pack stopped reading for some reason (because it\ndidn't like our input, or encountered another error). In\nthat case we'd expect the remote to report more details to\nus via the \"unpack ...\" status line. However, the current\ncode just hangs up completely, and the user never sees it.\n\nInstead, let's call receive_unpack_status(), which will\ncomplain on stderr with whatever reason the remote told us.\nNote that if our pack-objects fails because the connection\nwas severed or the remote just crashed entirely, then our\npacket_read_line() call may fail with \"the remote end hung\nup unexpectedly\". That's OK. It's a more accurate\ndescription than what we get now (which is just \"some refs\nfailed to push\").\n\nThis should be safe from any deadlocks. At the point we make\nthis call we'll have closed the writing end of the\nconnection to the server (either by handing it off to\na pack-objects which exited, explicitly in the stateless_rpc\ncase, or by doing a half-duplex shutdown for a socket). So\nthere should be no chance that the other side is waiting\nfor the rest of our pack-objects input.\n\nSigned-off-by: Jeff King \n---\n send-pack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 83c23aef6..e15232739 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -562,6 +562,14 @@ int send_pack(struct send_pack_args *args,\n \t\t\t\tclose(out);\n \t\t\tif (git_connection_is_socket(conn))\n \t\t\t\tshutdown(fd[0], SHUT_WR);\n+\n+\t\t\t/*\n+\t\t\t * Do not even bother with the return value; we know we\n+\t\t\t * are failing, and just want the error() side effects.\n+\t\t\t */\n+\t\t\tif (status_report)\n+\t\t\t\treceive_unpack_status(in);\n+\n \t\t\tif (use_sideband) {\n \t\t\t\tclose(demux.out);\n \t\t\t\tfinish_async(&demux);\n-- \n2.12.0.429.gde83c8049\n\n"},{"id":"313416","messageId":"20170307133736.4lpn7mgme26dqs3m@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"[PATCH 4/6] send-pack: improve unpack-status error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:37:36Z","receivedAt":"2017-03-07T14:06:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the remote tells us that the \"unpack\" step failed, we\nshow an error message. However, unless you are familiar with\nthe internals of send-pack and receive-pack, it was not\nclear that this represented an error on the remote side.\nLet's re-word to make that more obvious.\n\nLikewise, when we got an unexpected packet from the other\nend, we complained with a vague message but did not actually\nshow the packet. Let's fix that.\n\nAnd finally, neither message was marked for translation. The\nmessage from the remote probably won't be translated, but\nthere's no reason we can't do better for the local half.\n\nSigned-off-by: Jeff King \n---\n send-pack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 243633da1..83c23aef6 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -134,9 +134,9 @@ static int receive_unpack_status(int in)\n {\n \tconst char *line = packet_read_line(in, NULL);\n \tif (!skip_prefix(line, \"unpack \", &line))\n-\t\treturn error(\"did not receive remote status\");\n+\t\treturn error(_(\"unable to parse remote unpack status: %s\"), line);\n \tif (strcmp(line, \"ok\"))\n-\t\treturn error(\"unpack failed: %s\", line);\n+\t\treturn error(_(\"remote unpack failed: %s\"), line);\n \treturn 0;\n }\n \n-- \n2.12.0.429.gde83c8049\n\n"},{"id":"313427","messageId":"20170307135012.fdk6jhhl4zspdk2v@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:50:13Z","receivedAt":"2017-03-07T14:48:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 07, 2017 at 08:34:37AM -0500, Jeff King wrote:\n\n> Yuck. In the original, the error was generated by the child index-pack,\n> and we relayed it over the sideband. But we don't even get as far as\n> running index-pack in the newer version; we fail trying to make the\n> tmpdir. The error ends up in the \"unpack\" protocol field, but the client\n> side does a bad job of showing that. With the rest of the patches, it\n> looks like:\n> \n> $ git push ~/tmp/foo.git HEAD\n> Counting objects: 210973, done.\n> Delta compression using up to 8 threads.\n> Compressing objects: 100% (52799/52799), done.\n> error: remote unpack failed: unable to create temporary object directory\n> error: failed to push some refs to '/home/peff/tmp/foo.git'\n\nThere are two other options here I should mention.\n\nOne is that when pack-objects dies, we suppress the ref-status table\nentirely. Which is fair, because it doesn't have anything interesting to\nsay. But for a case like this where the other side stops reading our\npack early but still produces status reports, we could actually read\nthem all and have a status table like:\n\n $ git push ~/tmp/foo.git HEAD\n Counting objects: 209843, done.\n Delta compression using up to 8 threads.\n Compressing objects: 100% (52186/52186), done.\n error: remote unpack failed: unable to create temporary object directory\n To /home/peff/tmp/foo.git\n ! [remote rejected] HEAD -> jk/push-index-pack-deadlock (unpacker error)\n error: failed to push some refs to '/home/peff/tmp/foo.git'\n\nWe'd have to take some care to make sure we handle the case when the\nremote _doesn't_ manage to give us the status (e.g., when there's a\ncomplete hangup). I don't know if it's worth the effort. There's no new\ninformation there.\n\nThe second thing is that I think the design of the \"unpack \"\nreport is a bit dated. These days everybody supports the sideband\nprotocol, so it would probably make more sense to just issue the\n\"\" report via the sideband (at which point it gets a nice\n\"remote: \" prefix).\n\nWe could do something like this:\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex f2c6953a3..6204d3d00 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1670,6 +1670,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \tif (!tmp_objdir) {\n \t\tif (err_fd > 0)\n \t\t\tclose(err_fd);\n+\t\trp_error(\"unable to create temporary object directory: %s\",\n+\t\t\t strerror(errno));\n \t\treturn \"unable to create temporary object directory\";\n \t}\n \tchild.env = tmp_objdir_env(tmp_objdir);\n\nand drop the \"try to show the unpack failure\" parts of my series, and\nyou'd end up with:\n\n $ git push ~/tmp/foo.git HEAD\n Counting objects: 209843, done.\n Delta compression using up to 8 threads.\n Compressing objects: 100% (52186/52186), done.\n remote: error: unable to create temporary object directory: Permission denied\n error: failed to push some refs to '/home/peff/tmp/foo.git'\n\nbut in cases where pack-objects _doesn't_ fail, existing git versions\ndo show the \"unpack\" error. So you'd see it twice.\n\nI don't know if it's worth trying to hack around.\n\n-Peff\n"},{"id":"313438","messageId":"20170307133618.azljsixuv4o6f24p@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"[PATCH 3/6] send-pack: use skip_prefix for parsing unpack status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:36:19Z","receivedAt":"2017-03-07T18:36:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids repeating ourselves, and the use of magic\nnumbers.\n\nSigned-off-by: Jeff King \n---\nObviously not necessary, but just a cleanup while I was here.\n\n send-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 12e229e44..243633da1 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -133,10 +133,10 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n static int receive_unpack_status(int in)\n {\n \tconst char *line = packet_read_line(in, NULL);\n-\tif (!starts_with(line, \"unpack \"))\n+\tif (!skip_prefix(line, \"unpack \", &line))\n \t\treturn error(\"did not receive remote status\");\n-\tif (strcmp(line, \"unpack ok\"))\n-\t\treturn error(\"unpack failed: %s\", line + 7);\n+\tif (strcmp(line, \"ok\"))\n+\t\treturn error(\"unpack failed: %s\", line);\n \treturn 0;\n }\n \n-- \n2.12.0.429.gde83c8049\n\n"},{"id":"313460","messageId":"xmqq37eo66hw.fsf@gitster.mtv.corp.google.com","threadId":"45305","inReplyTo":"20170307133736.4lpn7mgme26dqs3m@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] send-pack: improve unpack-status error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-07T22:56:27Z","receivedAt":"2017-03-07T23:03:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When the remote tells us that the \"unpack\" step failed, we\n> show an error message. However, unless you are familiar with\n> the internals of send-pack and receive-pack, it was not\n> clear that this represented an error on the remote side.\n> Let's re-word to make that more obvious.\n>\n> Likewise, when we got an unexpected packet from the other\n> end, we complained with a vague message but did not actually\n> show the packet. Let's fix that.\n\nBoth make sense.\n\n> And finally, neither message was marked for translation. The\n> message from the remote probably won't be translated, but\n> there's no reason we can't do better for the local half.\n\nHmm, OK.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> send-pack.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index 243633da1..83c23aef6 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -134,9 +134,9 @@ static int receive_unpack_status(int in)\n> {\n> \tconst char *line = packet_read_line(in, NULL);\n> \tif (!skip_prefix(line, \"unpack \", &line))\n> -\t\treturn error(\"did not receive remote status\");\n> +\t\treturn error(_(\"unable to parse remote unpack status: %s\"), line);\n> \tif (strcmp(line, \"ok\"))\n> -\t\treturn error(\"unpack failed: %s\", line);\n> +\t\treturn error(_(\"remote unpack failed: %s\"), line);\n> \treturn 0;\n> }\n"},{"id":"313480","messageId":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","threadId":"45305","inReplyTo":"20170307111406.GB1955@quickstop.soohrt.org","subject":"[PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-07T13:34:38Z","receivedAt":"2017-03-08T02:26:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 07, 2017 at 12:14:06PM +0100, Horst Schirmeier wrote:\n\n> On Tue, 07 Mar 2017, Horst Schirmeier wrote:\n> > I observe a regression that seems to have been introduced between\n> > v2.10.0 and v2.11.0. When I try to push into a repository on the local\n> > filesystem that belongs to another user and has not explicitly been\n> > prepared for shared use, v2.11.0 shows some of the usual diagnostic\n> > output and then freezes instead of announcing why it failed to push.\n> \n> Bisecting points to v2.10.1-373-g722ff7f:\n> \n> 722ff7f876c8a2ad99c42434f58af098e61b96e8 is the first bad commit\n> commit 722ff7f876c8a2ad99c42434f58af098e61b96e8\n> Author: Jeff King \n> Date: Mon Oct 3 16:49:14 2016 -0400\n> \n> receive-pack: quarantine objects until pre-receive accepts\n\nThanks, I was able to reproduce easily with:\n\n git init --bare foo.git\n chown -R nobody foo.git\n git push foo.git HEAD\n\nHere's a series to fix it. The first patch addresses the deadlock. The\nrest try to improve the output on the client side. With v2.10.0, this\ncase looks like:\n\n $ git push ~/tmp/foo.git HEAD\n Counting objects: 209837, done.\n Delta compression using up to 8 threads.\n Compressing objects: 100% (52180/52180), done.\n remote: fatal: Unable to create temporary file '/home/peff/tmp/foo.git/./objects/pack/tmp_pack_XXXXXX': Permission denied\n error: failed to push some refs to '/home/peff/tmp/foo.git'\n\nWith just the first patch applied, you get just:\n\n $ git push ~/tmp/foo.git HEAD\n Counting objects: 210973, done.\n Delta compression using up to 8 threads.\n Compressing objects: 100% (52799/52799), done.\n error: failed to push some refs to '/home/peff/tmp/foo.git'\n\nYuck. In the original, the error was generated by the child index-pack,\nand we relayed it over the sideband. But we don't even get as far as\nrunning index-pack in the newer version; we fail trying to make the\ntmpdir. The error ends up in the \"unpack\" protocol field, but the client\nside does a bad job of showing that. With the rest of the patches, it\nlooks like:\n\n $ git push ~/tmp/foo.git HEAD\n Counting objects: 210973, done.\n Delta compression using up to 8 threads.\n Compressing objects: 100% (52799/52799), done.\n error: remote unpack failed: unable to create temporary object directory\n error: failed to push some refs to '/home/peff/tmp/foo.git'\n\nCompared to v2.10.0, that omits the name of the directory and the errno\nvalue (because receive-pack does not send them). That potentially makes\ndebugging harder, but arguably it's the right thing to do. On a remote\nsite, those details aren't really the client's business. But if people\nfeel strongly, we could add them back into this error code path.\n\nSo the first patch is a no-brainer and should go to maint. The rest can\nbe applied separately if need be.\n\n [1/6]: receive-pack: fix deadlock when we cannot create tmpdir\n [2/6]: send-pack: extract parsing of \"unpack\" response\n [3/6]: send-pack: use skip_prefix for parsing unpack status\n [4/6]: send-pack: improve unpack-status error messages\n [5/6]: send-pack: read \"unpack\" status even on pack-objects failure\n [6/6]: send-pack: report signal death of pack-objects\n\n builtin/receive-pack.c | 5 ++++-\n send-pack.c | 46 ++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 40 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"313490","messageId":"20170308054533.7wy52myrbpovyoet@sigill.intra.peff.net","threadId":"45305","inReplyTo":"xmqq37eo66hw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] send-pack: improve unpack-status error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-08T05:45:33Z","receivedAt":"2017-03-08T12:52:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 07, 2017 at 02:56:27PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > When the remote tells us that the \"unpack\" step failed, we\n> > show an error message. However, unless you are familiar with\n> > the internals of send-pack and receive-pack, it was not\n> > clear that this represented an error on the remote side.\n> > Let's re-word to make that more obvious.\n> >\n> > Likewise, when we got an unexpected packet from the other\n> > end, we complained with a vague message but did not actually\n> > show the packet. Let's fix that.\n> \n> Both make sense.\n> \n> > And finally, neither message was marked for translation. The\n> > message from the remote probably won't be translated, but\n> > there's no reason we can't do better for the local half.\n> \n> Hmm, OK.\n\nI'll admit that I don't actually use the translations myself, being a\nnative English speaker. So I am just guessing that somebody for whom\nEnglish is a second language would rather see the first half in a more\nintelligible format. That at least tells them what the second half _is_,\nso they might be able to search for the error with more context.\n\nIf my guess is wrong, though, I'm happy to retract that part or bump it\nout to a separate patch.\n\n-Peff\n"},{"id":"313509","messageId":"xmqqtw733cv9.fsf@gitster.mtv.corp.google.com","threadId":"45305","inReplyTo":"20170308054533.7wy52myrbpovyoet@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] send-pack: improve unpack-status error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-08T17:19:22Z","receivedAt":"2017-03-08T17:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > And finally, neither message was marked for translation. The\n>> > message from the remote probably won't be translated, but\n>> > there's no reason we can't do better for the local half.\n>> \n>> Hmm, OK.\n>\n> I'll admit that I don't actually use the translations myself, being a\n> native English speaker. So I am just guessing that somebody for whom\n> English is a second language would rather see the first half in a more\n> intelligible format. That at least tells them what the second half _is_,\n> so they might be able to search for the error with more context.\n>\n> If my guess is wrong, though, I'm happy to retract that part or bump it\n> out to a separate patch.\n\nI was merely undecided between \"at least half is in my language\" and\n\"both are consistently untranslated\" which one is easier to use by\nmy highschool friends who do not grok English. And I still cannot\ndecide. \n\nWhen responding to a request-for-help that quotes messages that was\ntranslated, I would imagine we would need one extra \"git grep\" to\nfind the message (without understanding that ourselves) from po/ for\nthe original before running \"git grep\" to find the code that\nproduced the message [*1*].\n\nOn the other hand, we get request-for-help from those having issues\nin a setup where the software running on the other side is not even\nours, so I am (slightly) more inclined to agree that \"half is in my\nlanguage\" is better than nothing.\n\nThanks.\n\n\n[Footnote]\n\n*1* If the translation came from us, that is. If I recall\ncorrectly, some distros do their own po/ and core Git developers are\nnot likely to have them around, so \"git grep\" in our po/ may not see\nany hit.\n\n\n\n"},{"id":"313526","messageId":"20170308175844.GG7566@quickstop.soohrt.org","threadId":"45305","inReplyTo":"20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] deadlock regression in v2.11.0 with failed mkdtemp","fromName":"Horst Schirmeier","fromEmail":"horst@schirmeier.com","sentAt":"2017-03-08T17:58:44Z","receivedAt":"2017-03-08T17:58:52Z","isPatch":true,"sender":{"key":"horst@schirmeier.com","avatar":null},"body":"On Tue, 07 Mar 2017, Jeff King wrote:\n> On Tue, Mar 07, 2017 at 12:14:06PM +0100, Horst Schirmeier wrote:\n> > On Tue, 07 Mar 2017, Horst Schirmeier wrote:\n> > > I observe a regression that seems to have been introduced between\n> > > v2.10.0 and v2.11.0. When I try to push into a repository on the local\n> > > filesystem that belongs to another user and has not explicitly been\n> > > prepared for shared use, v2.11.0 shows some of the usual diagnostic\n> > > output and then freezes instead of announcing why it failed to push.\n> > \n> > Bisecting points to v2.10.1-373-g722ff7f:\n> > \n> > 722ff7f876c8a2ad99c42434f58af098e61b96e8 is the first bad commit\n> > commit 722ff7f876c8a2ad99c42434f58af098e61b96e8\n> > Author: Jeff King \n> > Date: Mon Oct 3 16:49:14 2016 -0400\n> > \n> > receive-pack: quarantine objects until pre-receive accepts\n> \n> Thanks, I was able to reproduce easily with:\n> \n> git init --bare foo.git\n> chown -R nobody foo.git\n> git push foo.git HEAD\n> \n> Here's a series to fix it. The first patch addresses the deadlock. The\n> rest try to improve the output on the client side. With v2.10.0, this\n> case looks like:\n> \n> $ git push ~/tmp/foo.git HEAD\n> Counting objects: 209837, done.\n> Delta compression using up to 8 threads.\n> Compressing objects: 100% (52180/52180), done.\n> remote: fatal: Unable to create temporary file '/home/peff/tmp/foo.git/./objects/pack/tmp_pack_XXXXXX': Permission denied\n> error: failed to push some refs to '/home/peff/tmp/foo.git'\n\nI tested your jk/push-deadlock-regression-fix branch in my local clone,\nyour patches fix the problem for me. Thanks!\n\nHorst\n\n-- \nPGP-Key 0xD40E0E7A\n"}]}