{"thread":{"id":"33432","subject":"[PATCH v4] transport-helper: report errors properly","startedAt":"2013-04-08T14:40:04Z","lastAt":"2013-04-14T15:54:07Z","messageCount":31,"participants":["Felipe Contreras","Sverre Rabbelier","Jeff King","Thomas Rast","Eric Sunshine","Junio C Hamano","rh"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"213518","messageId":"1365432004-20132-1-git-send-email-felipe.contreras@gmail.com","threadId":"33432","inReplyTo":null,"subject":"[PATCH v4] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-08T14:40:04Z","receivedAt":"2013-04-08T14:40:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"If a push fails because the remote-helper died (with fast-export), the\nuser won't see any error message. So let's add one.\n\nAt the same time lets add tests to ensure this error is reported, and\nwhile we are at it, check the error from fast-import\n\nSuggested-by: Jeff King \nSigned-off-by: Felipe Contreras \n---\n git-remote-testgit | 13 +++++++++++++\n t/t5801-remote-helpers.sh | 21 +++++++++++++++++++++\n transport-helper.c | 2 +-\n 3 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex b395c8d..2eb7889 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -61,12 +61,25 @@ do\n \t\t\techo \"feature import-marks=$gitmarks\"\n \t\t\techo \"feature export-marks=$gitmarks\"\n \t\tfi\n+\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\techo \"feature done\"\n+\t\t\texit 1\n+\t\tfi\n+\n \t\techo \"feature done\"\n \t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n \t\techo \"done\"\n \t\t;;\n \texport)\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\tsleep 1 # don't let fast-export get SIGPIPE\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n \n \t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex f387027..2dfcf64 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -166,4 +166,25 @@ test_expect_success 'push ref with existing object' '\n \tcompare_refs local dup server dup\n '\n \n+test_expect_success 'proper failure checks for fetching' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git fetch 2> error &&\n+\tcat error &&\n+\tgrep -q \"Error while running fast-import\" error\n+\t)\n+'\n+\n+# We sleep to give fast-export a chance to catch the SIGPIPE\n+test_expect_success 'proper failure checks for pushing' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git push --all 2> error &&\n+\tcat error &&\n+\tgrep -q \"Reading from remote helper failed\" error\n+\t)\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cb3ef7d..96081cc 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)\n \tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n-\t\texit(128);\n+\t\tdie(\"Reading from remote helper failed\");\n \t}\n \n \tif (debug)\n-- \n1.8.2\n"},{"id":"213572","messageId":"CAGdFq_h9o+oMriF52Bh2e60eJfLM7F9GTbwcSE1mD3Leyt76Yg@mail.gmail.com","threadId":"33432","inReplyTo":"1365432004-20132-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v4] transport-helper: report errors properly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2013-04-08T18:20:15Z","receivedAt":"2013-04-08T18:20:15Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras\n wrote:\n> + die(\"Reading from remote helper failed\");\n\nDoes the user know what a remote helper is? Could we point them at\nsome helpful docs in case they don't?\n\n--\nCheers,\n\nSverre Rabbelier\n"},{"id":"213591","messageId":"20130408192829.GC7337@sigill.intra.peff.net","threadId":"33432","inReplyTo":"1365432004-20132-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v4] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T19:28:29Z","receivedAt":"2013-04-08T19:28:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2013 at 09:40:04AM -0500, Felipe Contreras wrote:\n\n> If a push fails because the remote-helper died (with fast-export), the\n> user won't see any error message. So let's add one.\n> \n> At the same time lets add tests to ensure this error is reported, and\n> while we are at it, check the error from fast-import\n\nThanks, I think this patch is definitely the right direction.\n\nIt seems like there is a lot of back-story that had to be clarified\nduring the review/discussion. Is there a reason not to summarize it here\nso later readers of this commit are enlightened?\n\nI'm thinking something like:\n\n If a push fails because the remote-helper died (with fast-export), the\n user does not see any error message. We do correctly die with a failed\n exit code, as we notice that the helper has died while reading back\n the ref status from the helper. However, we don't print any message.\n This is OK if the helper itself printed a useful error message, but we\n cannot count on that; let's let the user know that the helper failed.\n\n In the long run, it may make more sense to propagate the error back up\n to push, so that it can present the usual status table and give a\n nicer message. But this is a much simpler fix that can help\n immediately.\n\n While we're adding tests, let's also confirm that the remote-helper\n dying is also detect when importing refs. We currently do so robustly\n when the helper uses the \"done\" feature (and that is what we test). We\n cannot do so reliably when the helper does not use the \"done\" feature,\n but it is not even worth testing; the right solution is for the helper\n to start using \"done\".\n\n> \texport)\n> +\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> +\t\tthen\n> +\t\t\tsleep 1 # don't let fast-export get SIGPIPE\n> +\t\t\texit 1\n> +\t\tfi\n\nWe can do away with this sleep with:\n\n while read line; do\n test \"$line\" = \"done\" && break\n done\n\nThe version I posted yesterday had both the read and the sleep, but the\nsleep was only necessary there to demonstrate the race with\ncheck_command.\n\n> +# We sleep to give fast-export a chance to catch the SIGPIPE\n> +test_expect_success 'proper failure checks for pushing' '\n\nI think we can drop this comment now, right?\n\n-Peff\n"},{"id":"213592","messageId":"20130408193043.GD7337@sigill.intra.peff.net","threadId":"33432","inReplyTo":"CAGdFq_h9o+oMriF52Bh2e60eJfLM7F9GTbwcSE1mD3Leyt76Yg@mail.gmail.com","subject":"Re: [PATCH v4] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T19:30:43Z","receivedAt":"2013-04-08T19:30:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2013 at 11:20:15AM -0700, Sverre Rabbelier wrote:\n\n> On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras\n> wrote:\n> > + die(\"Reading from remote helper failed\");\n> \n> Does the user know what a remote helper is? Could we point them at\n> some helpful docs in case they don't?\n\nThat's a good point. I wonder if it would be enough to say:\n\n fatal: Reading from helper git-remote-X failed\n\nThat might make it more clear what the helper's role is, and showing the\ncommand name gives the user a starting point for running \"man\".\n\n-Peff\n"},{"id":"213724","messageId":"87ip3v1j2a.fsf@hexa.v.cablecom.net","threadId":"33432","inReplyTo":"1365432004-20132-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v4] transport-helper: report errors properly","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-09T21:38:05Z","receivedAt":"2013-04-09T21:38:05Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> If a push fails because the remote-helper died (with fast-export), the\n> user won't see any error message. So let's add one.\n>\n> At the same time lets add tests to ensure this error is reported, and\n> while we are at it, check the error from fast-import\n[...]\n> +# We sleep to give fast-export a chance to catch the SIGPIPE\n> +test_expect_success 'proper failure checks for pushing' '\n> +\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> +\texport GIT_REMOTE_TESTGIT_FAILURE &&\n> +\tcd local &&\n> +\ttest_must_fail git push --all 2> error &&\n> +\tcat error &&\n> +\tgrep -q \"Reading from remote helper failed\" error\n> +\t)\n> +'\n\nThere appears to be a race in the version that is in today's pu\n(5eb25f737b). I reproduced with this:\n\n cd git/t\n i=1\n while ./t5801-remote-helpers.sh --root=/dev/shm --valgrind\n do\n i=$(($i+1))\n done\n\nTwo out of six of these loops quit within 1 and 2 iterations,\nrespectively, both with an error along the lines of:\n\n expecting success: \n (GIT_REMOTE_TESTGIT_FAILURE=1 &&\n export GIT_REMOTE_TESTGIT_FAILURE &&\n cd local &&\n test_must_fail git push --all 2> error &&\n cat error &&\n grep -q \"Reading from remote helper failed\" error\n )\n\n error: fast-export died of signal 13\n fatal: Error while running fast-export\n not ok 21 - proper failure checks for pushing\n\nI haven't been able to reproduce outside of valgrind tests. Is this an\nexpected issue, caused by overrunning the sleep somehow? If so, can you\nincrease the sleep delay under valgrind so as to not cause intermittent\nfailures in the test suite?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"213726","messageId":"20130409215018.GA28271@sigill.intra.peff.net","threadId":"33432","inReplyTo":"87ip3v1j2a.fsf@hexa.v.cablecom.net","subject":"Re: [PATCH v4] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-09T21:50:19Z","receivedAt":"2013-04-09T21:50:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 09, 2013 at 11:38:05PM +0200, Thomas Rast wrote:\n\n> Two out of six of these loops quit within 1 and 2 iterations,\n> respectively, both with an error along the lines of:\n> \n> expecting success: \n> (GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> export GIT_REMOTE_TESTGIT_FAILURE &&\n> cd local &&\n> test_must_fail git push --all 2> error &&\n> cat error &&\n> grep -q \"Reading from remote helper failed\" error\n> )\n> \n> error: fast-export died of signal 13\n> fatal: Error while running fast-export\n> not ok 21 - proper failure checks for pushing\n> \n> I haven't been able to reproduce outside of valgrind tests. Is this an\n> expected issue, caused by overrunning the sleep somehow? If so, can you\n> increase the sleep delay under valgrind so as to not cause intermittent\n> failures in the test suite?\n\nYeah, I am not too surprised. The failing helper sleeps before exiting\nso that fast-export puts all of its data into the pipe buffer before the\nhelper dies, and does not get SIGPIPE. But obviously the sleep is just\ndelaying the problem if your fast-export runs really slowly (which, if\nyou are running under valgrind, is a possibility).\n\nThe helper should instead just consume all of fast-export's input before\nexiting, which accomplishes the same thing, finishes sooner in the\nnormal case, and doesn't race. And I think it also simulates a\nreasonable real-world setup (a helper reads and converts the data, but\nthen dies while writing the output to disk, the network, or whatever).\n\nI posted review comments, including that, and I'm assuming that Felipe\nis going to re-roll at some point.\n\n-Peff\n"},{"id":"213846","messageId":"20130410211311.GA24277@sigill.intra.peff.net","threadId":"33432","inReplyTo":"1365432004-20132-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v5 0/2] reporting transport helper errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-10T21:13:11Z","receivedAt":"2013-04-10T21:13:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I think this topic is close to being done, so I just wanted to move it\nalong.\n\n [1/2]: transport-helper: report errors properly\n\n This is Felipe's v4 patch with the adjustments I suggested in\n review. It explains more in the commit message, and should fix\n Thomas's valgrind failures (it consumes fast-export's data before\n dying rather than sleeping and hoping that fast-export is done\n writing).\n\n [2/2]: transport-helper: mention helper name when it dies\n\n This changes the error message, to help with the issue raised by\n Sverre.\n\n-Peff\n"},{"id":"213847","messageId":"20130410211552.GA3256@sigill.intra.peff.net","threadId":"33432","inReplyTo":"20130410211311.GA24277@sigill.intra.peff.net","subject":"[PATCH 1/2] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-10T21:15:52Z","receivedAt":"2013-04-10T21:15:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Felipe Contreras \n\nIf a push fails because the remote-helper died (with\nfast-export), the user does not see any error message. We do\ncorrectly die with a failed exit code, as we notice that the\nhelper has died while reading back the ref status from the\nhelper. However, we don't print any message. This is OK if\nthe helper itself printed a useful error message, but we\ncannot count on that; let's let the user know that the\nhelper failed.\n\nIn the long run, it may make more sense to propagate the\nerror back up to push, so that it can present the usual\nstatus table and give a nicer message. But this is a much\nsimpler fix that can help immediately.\n\nWhile we're adding tests, let's also confirm that the\nremote-helper dying is also detect when importing refs. We\ncurrently do so robustly when the helper uses the \"done\"\nfeature (and that is what we test). We cannot do so\nreliably when the helper does not use the \"done\" feature,\nbut it is not even worth testing; the right solution is for\nthe helper to start using \"done\".\n\nSuggested-by: Jeff King \nSigned-off-by: Felipe Contreras \nSigned-off-by: Jeff King \n---\nFelipe,\n\nCan you acknowledge that it's OK to stick your name on this, as it's not\nexactly what you submitted before?\n\n git-remote-testgit | 19 +++++++++++++++++++\n t/t5801-remote-helpers.sh | 20 ++++++++++++++++++++\n transport-helper.c | 2 +-\n 3 files changed, 40 insertions(+), 1 deletion(-)\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex b395c8d..5fd09f9 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -61,12 +61,31 @@ do\n \t\t\techo \"feature import-marks=$gitmarks\"\n \t\t\techo \"feature export-marks=$gitmarks\"\n \t\tfi\n+\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\techo \"feature done\"\n+\t\t\texit 1\n+\t\tfi\n+\n \t\techo \"feature done\"\n \t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n \t\techo \"done\"\n \t\t;;\n \texport)\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\t# consume input so fast-export doesn't get SIGPIPE;\n+\t\t\t# git would also notice that case, but we want\n+\t\t\t# to make sure we are exercising the later\n+\t\t\t# error checks\n+\t\t\twhile read line; do\n+\t\t\t\ttest \"done\" = \"$line\" && break\n+\t\t\tdone\n+\t\t\texit 1\n+\t\tfi\n+\n \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n \n \t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex f387027..aafc46a 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -166,4 +166,24 @@ test_expect_success 'push ref with existing object' '\n \tcompare_refs local dup server dup\n '\n \n+test_expect_success 'proper failure checks for fetching' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git fetch 2> error &&\n+\tcat error &&\n+\tgrep -q \"Error while running fast-import\" error\n+\t)\n+'\n+\n+test_expect_success 'proper failure checks for pushing' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git push --all 2> error &&\n+\tcat error &&\n+\tgrep -q \"Reading from remote helper failed\" error\n+\t)\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cb3ef7d..96081cc 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)\n \tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n-\t\texit(128);\n+\t\tdie(\"Reading from remote helper failed\");\n \t}\n \n \tif (debug)\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213848","messageId":"20130410211603.GB3256@sigill.intra.peff.net","threadId":"33432","inReplyTo":"20130410211311.GA24277@sigill.intra.peff.net","subject":"[PATCH 2/2] transport-helper: mention helper name when it dies","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-10T21:16:03Z","receivedAt":"2013-04-10T21:16:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we try to read from a remote-helper and get EOF or an\nerror, we print a message indicating that the helper died.\nHowever, users may not know that a remote helper was in use\n(e.g., when using git-over-http), or even what a remote\nhelper is.\n\nLet's print the name of the helper (e.g., \"git-remote-https\");\nthis makes it more obvious what the program is for, and\nprovides a useful token for reporting bugs or searching for\nmore information (e.g., in manpages).\n\nSigned-off-by: Jeff King \n---\n t/t5801-remote-helpers.sh | 2 +-\n transport-helper.c | 8 ++++----\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex aafc46a..8b2cb68 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -182,7 +182,7 @@ test_expect_success 'proper failure checks for pushing' '\n \tcd local &&\n \ttest_must_fail git push --all 2> error &&\n \tcat error &&\n-\tgrep -q \"Reading from remote helper failed\" error\n+\tgrep -q \"Reading from helper .git-remote-testgit. failed\" error\n \t)\n '\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 96081cc..3fc43b9 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -46,7 +46,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)\n \t\tdie_errno(\"Full write to remote helper failed\");\n }\n \n-static int recvline_fh(FILE *helper, struct strbuf *buffer)\n+static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)\n {\n \tstrbuf_reset(buffer);\n \tif (debug)\n@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)\n \tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n-\t\tdie(\"Reading from remote helper failed\");\n+\t\tdie(\"Reading from helper 'git-remote-%s' failed\", name);\n \t}\n \n \tif (debug)\n@@ -64,7 +64,7 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer)\n \n static int recvline(struct helper_data *helper, struct strbuf *buffer)\n {\n-\treturn recvline_fh(helper->out, buffer);\n+\treturn recvline_fh(helper->out, buffer, helper->name);\n }\n \n static void xchgline(struct helper_data *helper, struct strbuf *buffer)\n@@ -536,7 +536,7 @@ static int process_connect_service(struct transport *transport,\n \t\tgoto exit;\n \n \tsendline(data, &cmdbuf);\n-\trecvline_fh(input, &cmdbuf);\n+\trecvline_fh(input, &cmdbuf, name);\n \tif (!strcmp(cmdbuf.buf, \"\")) {\n \t\tdata->no_disconnect_req = 1;\n \t\tif (debug)\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213851","messageId":"CAGdFq_iUEg8gxgDXvKsEW=M=hW0CmO7yZURd82s5fW+rOiBMrg@mail.gmail.com","threadId":"33432","inReplyTo":"20130410211552.GA3256@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2013-04-10T21:22:44Z","receivedAt":"2013-04-10T21:22:44Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Apr 10, 2013 at 2:15 PM, Jeff King wrote:\n> From: Felipe Contreras \n>\n> If a push fails because the remote-helper died (with\n> fast-export), the user does not see any error message. We do\n> correctly die with a failed exit code, as we notice that the\n> helper has died while reading back the ref status from the\n> helper. However, we don't print any message. This is OK if\n> the helper itself printed a useful error message, but we\n> cannot count on that; let's let the user know that the\n> helper failed.\n>\n> In the long run, it may make more sense to propagate the\n> error back up to push, so that it can present the usual\n> status table and give a nicer message. But this is a much\n> simpler fix that can help immediately.\n>\n> While we're adding tests, let's also confirm that the\n> remote-helper dying is also detect when importing refs. We\n> currently do so robustly when the helper uses the \"done\"\n> feature (and that is what we test). We cannot do so\n> reliably when the helper does not use the \"done\" feature,\n> but it is not even worth testing; the right solution is for\n> the helper to start using \"done\".\n>\n> Suggested-by: Jeff King \n> Signed-off-by: Felipe Contreras \n> Signed-off-by: Jeff King \n\nThe fixes you made to this patch make a lot of sense, glad to not have\na 'sleep 1' in our tests.\n\nAcked-by: Sverre Rabbelier \n\n--\nCheers,\n\nSverre Rabbelier\n"},{"id":"213854","messageId":"CAGdFq_j5vB+OJAkuk-EMLjyrbiY7QrBiwWPraPA7SWTtuUqgZA@mail.gmail.com","threadId":"33432","inReplyTo":"20130410211603.GB3256@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] transport-helper: mention helper name when it dies","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2013-04-10T21:23:56Z","receivedAt":"2013-04-10T21:23:56Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Apr 10, 2013 at 2:16 PM, Jeff King wrote:\n> When we try to read from a remote-helper and get EOF or an\n> error, we print a message indicating that the helper died.\n> However, users may not know that a remote helper was in use\n> (e.g., when using git-over-http), or even what a remote\n> helper is.\n>\n> Let's print the name of the helper (e.g., \"git-remote-https\");\n> this makes it more obvious what the program is for, and\n> provides a useful token for reporting bugs or searching for\n> more information (e.g., in manpages).\n>\n> Signed-off-by: Jeff King \n\nBetter than nothing:\n\nAcked-by: Sverre Rabbelier \n\n--\nCheers,\n\nSverre Rabbelier\n"},{"id":"213855","messageId":"20130410212833.GA5909@sigill.intra.peff.net","threadId":"33432","inReplyTo":"CAGdFq_j5vB+OJAkuk-EMLjyrbiY7QrBiwWPraPA7SWTtuUqgZA@mail.gmail.com","subject":"Re: [PATCH 2/2] transport-helper: mention helper name when it dies","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-10T21:28:33Z","receivedAt":"2013-04-10T21:28:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 10, 2013 at 02:23:56PM -0700, Sverre Rabbelier wrote:\n\n> On Wed, Apr 10, 2013 at 2:16 PM, Jeff King wrote:\n> > When we try to read from a remote-helper and get EOF or an\n> > error, we print a message indicating that the helper died.\n> > However, users may not know that a remote helper was in use\n> > (e.g., when using git-over-http), or even what a remote\n> > helper is.\n> >\n> > Let's print the name of the helper (e.g., \"git-remote-https\");\n> > this makes it more obvious what the program is for, and\n> > provides a useful token for reporting bugs or searching for\n> > more information (e.g., in manpages).\n> >\n> > Signed-off-by: Jeff King \n> \n> Better than nothing:\n> \n> Acked-by: Sverre Rabbelier \n\nNow that's the kind of whole-hearted endorsement I strive for. :)\n\nIf you have better wording, I'm open to it. I do note that we don't\nactually have a manpage for \"git-remote-https\", though we do for others.\nProbably \"man git-remote-helpers\" is the most sensible thing to point\nthe user to. But I don't even think this is worthy of a big advice\nmessage. It's a bug in the helper, it shouldn't really happen, and\ngiving the user a token they can use to report or google for the error\nis probably good enough.\n\n-Peff\n"},{"id":"213859","messageId":"CAGdFq_ju7d59jm+d+cmM2T8zR7WjLEVf=HDGa-3Uo3GLbnqh6w@mail.gmail.com","threadId":"33432","inReplyTo":"20130410212833.GA5909@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] transport-helper: mention helper name when it dies","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2013-04-10T21:35:41Z","receivedAt":"2013-04-10T21:35:41Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Apr 10, 2013 at 2:28 PM, Jeff King wrote:\n> Now that's the kind of whole-hearted endorsement I strive for. :)\n\nIt's nothing wrong with your patch, the main problem is that there's\nnot really a good place to point users at.\n\n> If you have better wording, I'm open to it. I do note that we don't\n> actually have a manpage for \"git-remote-https\", though we do for others.\n> Probably \"man git-remote-helpers\" is the most sensible thing to point\n> the user to. But I don't even think this is worthy of a big advice\n> message. It's a bug in the helper, it shouldn't really happen, and\n> giving the user a token they can use to report or google for the error\n> is probably good enough.\n\nYeah, exactly. man git-remote-helpers is more a place for developers\nto read how to implement a git-remote-helper, not so much a place for\nusers to read what they are, and/or how to use them.\n\n--\nCheers,\n\nSverre Rabbelier\n"},{"id":"213861","messageId":"CAPig+cR_zL5AW+h7ovGjo-Xc=wVcKPbHoRtG0wBG_b9oVXc05Q@mail.gmail.com","threadId":"33432","inReplyTo":"20130410211552.GA3256@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-04-10T21:46:25Z","receivedAt":"2013-04-10T21:46:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 10, 2013 at 5:15 PM, Jeff King wrote:\n> From: Felipe Contreras \n>\n> If a push fails because the remote-helper died (with\n> fast-export), the user does not see any error message. We do\n> correctly die with a failed exit code, as we notice that the\n> helper has died while reading back the ref status from the\n> helper. However, we don't print any message. This is OK if\n> the helper itself printed a useful error message, but we\n> cannot count on that; let's let the user know that the\n> helper failed.\n>\n> In the long run, it may make more sense to propagate the\n> error back up to push, so that it can present the usual\n> status table and give a nicer message. But this is a much\n> simpler fix that can help immediately.\n>\n> While we're adding tests, let's also confirm that the\n> remote-helper dying is also detect when importing refs. We\n\ns/detect/detected/\n\n> currently do so robustly when the helper uses the \"done\"\n> feature (and that is what we test). We cannot do so\n> reliably when the helper does not use the \"done\" feature,\n> but it is not even worth testing; the right solution is for\n> the helper to start using \"done\".\n>\n> Suggested-by: Jeff King \n> Signed-off-by: Felipe Contreras \n> Signed-off-by: Jeff King \n"},{"id":"299208","messageId":"20130410161320.679b68ca07cd1fe32bb25c70@lavabit.com","threadId":"33432","inReplyTo":"1365432004-20132-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v4] transport-helper: report errors properly","fromName":"rh","fromEmail":"richard_hubbe11@lavabit.com","sentAt":"2013-04-10T23:13:20Z","receivedAt":"2013-04-10T23:13:20Z","isPatch":true,"sender":{"key":"richard_hubbe11@lavabit.com","avatar":null},"body":"On Mon, 8 Apr 2013 09:40:04 -0500\nFelipe Contreras wrote:\n\n> If a push fails because the remote-helper died (with fast-export), the\n> user won't see any error message. So let's add one.\n> \n> At the same time lets add tests to ensure this error is reported, and\n> while we are at it, check the error from fast-import\n> \n\n....\n\n> +++ b/transport-helper.c\n> @@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf\n> *buffer) if (strbuf_getline(buffer, helper, '\\n') == EOF) {\n> \t\tif (debug)\n> \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\n> \\n\");\n> -\t\texit(128);\n> +\t\tdie(\"Reading from remote helper failed\");\n\nDo I read this correctly? If I'm in debug mode the remote helper quit\nbut if not in debug mode it failed? Debuggers never fail they only quit!\n\n\n> \t}\n> \n> \tif (debug)\n\n"},{"id":"213891","messageId":"20130411033932.GC14551@sigill.intra.peff.net","threadId":"33432","inReplyTo":"20130410161320.679b68ca07cd1fe32bb25c70@lavabit.com","subject":"Re: [PATCH v4] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T03:39:32Z","receivedAt":"2013-04-11T03:39:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 10, 2013 at 04:13:20PM -0700, rh wrote:\n\n> > +++ b/transport-helper.c\n> > @@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf\n> > *buffer) if (strbuf_getline(buffer, helper, '\\n') == EOF) {\n> > \t\tif (debug)\n> > \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\n> > \\n\");\n> > -\t\texit(128);\n> > +\t\tdie(\"Reading from remote helper failed\");\n> \n> Do I read this correctly? If I'm in debug mode the remote helper quit\n> but if not in debug mode it failed? Debuggers never fail they only quit!\n\nIn debug mode, it prints both messages. The debug version is superfluous\nat this point, though, and we can probably just drop it.\n\n-Peff\n"},{"id":"213956","messageId":"CAMP44s02K5ydKLNi0umMkuAicoVTWyCdVfjs0yssCa2oyFShGQ@mail.gmail.com","threadId":"33432","inReplyTo":"20130410211552.GA3256@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-11T13:22:26Z","receivedAt":"2013-04-11T13:22:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Apr 10, 2013 at 4:15 PM, Jeff King wrote:\n> From: Felipe Contreras \n>\n> If a push fails because the remote-helper died (with\n> fast-export), the user does not see any error message. We do\n> correctly die with a failed exit code, as we notice that the\n> helper has died while reading back the ref status from the\n> helper. However, we don't print any message. This is OK if\n> the helper itself printed a useful error message, but we\n> cannot count on that; let's let the user know that the\n> helper failed.\n\nThis explained the same thing:\n> If a push fails because the remote-helper died (with fast-export), the user won't see any error message. So let's add one.\n\nGranted, depending on the way the remote-helper died, an error might\nor might not been printed, so s/won't/might not/.\n\nThe fact that an exit code was returned before is not relevant,\nneither is how the exit was returned, and for that matter neither is\nall the other things that are happening in this code. It's just noise.\n\nThe only thing that is relevant is this:\n\n- exit(128);\n+ die(\"Reading from remote helper failed\");\n\nIt's a simple change, and simple to explain.\n\n> In the long run, it may make more sense to propagate the\n> error back up to push, so that it can present the usual\n> status table and give a nicer message. But this is a much\n> simpler fix that can help immediately.\n\nYes it might, and it might make sense to rewrite much of this code,\nbut that's not relevant.\n\n> While we're adding tests, let's also confirm that the\n> remote-helper dying is also detect when importing refs.\n\nThat is enough explanation.\n\n> We\n> currently do so robustly when the helper uses the \"done\"\n> feature (and that is what we test). We cannot do so\n> reliably when the helper does not use the \"done\" feature,\n> but it is not even worth testing; the right solution is for\n> the helper to start using \"done\".\n\nThis doesn't help anyone, and it's not even accurate. I think it might\nbe possible enforce remote-helpers to implement the \"done\" feature,\nand we might want to do that later. But of course, discussing what bad\nthings remote-helpers could do, and how we should test and babysit\nthem is not relevant here.\n\nIf it was important to explain the subtleties and reasoning behind\nthis change, it should be a separate patch.\n\n> Suggested-by: Jeff King \n> Signed-off-by: Felipe Contreras \n> Signed-off-by: Jeff King \n\nI would add:\n\n[jk: rewrote every piece of text]\n\n> export)\n> + if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> + then\n> + # consume input so fast-export doesn't get SIGPIPE;\n\nI think this is explanation enough.\n\n> + # git would also notice that case, but we want\n> + # to make sure we are exercising the later\n> + # error checks\n\nI don't understand what is being said here. What is \"that case\"?\n\n> + while read line; do\n> + test \"done\" = \"$line\" && break\n> + done\n> + exit\n\nLGTM.\n\nCheers.\n\n--\nFelipe Contreras\n"},{"id":"213964","messageId":"20130411161845.GA665@sigill.intra.peff.net","threadId":"33432","inReplyTo":"CAMP44s02K5ydKLNi0umMkuAicoVTWyCdVfjs0yssCa2oyFShGQ@mail.gmail.com","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T16:18:45Z","receivedAt":"2013-04-11T16:18:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote:\n\n> > We\n> > currently do so robustly when the helper uses the \"done\"\n> > feature (and that is what we test). We cannot do so\n> > reliably when the helper does not use the \"done\" feature,\n> > but it is not even worth testing; the right solution is for\n> > the helper to start using \"done\".\n> \n> This doesn't help anyone, and it's not even accurate. I think it might\n> be possible enforce remote-helpers to implement the \"done\" feature,\n> and we might want to do that later. But of course, discussing what bad\n> things remote-helpers could do, and how we should test and babysit\n> them is not relevant here.\n> \n> If it was important to explain the subtleties and reasoning behind\n> this change, it should be a separate patch.\n\nI am OK with adding the test for import as a separate patch. What I am\nnot OK with (and this goes for the rest of the commit message, too) is\nfailing to explain any back-story at all for why the change is done in\nthe way it is.\n\n_You_ may understand it _right now_, but that is not the primary\naudience of the message. The primary audience is somebody else a year\nfrom now who is wondering why this patch was done the way it was. When\nthey are trying to find out why git does not detect errors in a helper,\nand they notice that our test for failure only check the \"done\" case,\nisn't it more helpful to say \"we considered the other case, but it was\nnot worth fixing\" rather than leaving them to guess?\n\nI may be more verbose than necessary in some of my commit messages, but\nI would much rather err on the side of explaining too much than too\nlittle.\n\n> > export)\n> > + if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> > + then\n> > + # consume input so fast-export doesn't get SIGPIPE;\n> \n> I think this is explanation enough.\n> \n> > + # git would also notice that case, but we want\n> > + # to make sure we are exercising the later\n> > + # error checks\n> \n> I don't understand what is being said here. What is \"that case\"?\n\nThe case that fast-export gets SIGPIPE. I was trying to explain not\njust _what_ we are doing, but _why_ it is important. Perhaps a better\nwording would be:\n\n # consume input so fast-export doesn't get SIGPIPE;\n # we do not technically need to do so in order for\n # git to notice the failure to export, as it will\n # detect problems either with fast-export or with\n # the helper failing to report ref status. But since\n # we are trying to demonstrate that the latter\n # check works, we must avoid the SIGPIPE, which would\n # trigger the former.\n\n-Peff\n"},{"id":"213969","messageId":"CAMP44s2-4i_tSzz8Y88_YnK5d1AjNoTqOa7eXZ0W5Vzk9Uosng@mail.gmail.com","threadId":"33432","inReplyTo":"20130411161845.GA665@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-11T16:49:11Z","receivedAt":"2013-04-11T16:49:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 11, 2013 at 11:18 AM, Jeff King wrote:\n> On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote:\n>\n>> > We\n>> > currently do so robustly when the helper uses the \"done\"\n>> > feature (and that is what we test). We cannot do so\n>> > reliably when the helper does not use the \"done\" feature,\n>> > but it is not even worth testing; the right solution is for\n>> > the helper to start using \"done\".\n>>\n>> This doesn't help anyone, and it's not even accurate. I think it might\n>> be possible enforce remote-helpers to implement the \"done\" feature,\n>> and we might want to do that later. But of course, discussing what bad\n>> things remote-helpers could do, and how we should test and babysit\n>> them is not relevant here.\n>>\n>> If it was important to explain the subtleties and reasoning behind\n>> this change, it should be a separate patch.\n>\n> I am OK with adding the test for import as a separate patch. What I am\n> not OK with (and this goes for the rest of the commit message, too) is\n> failing to explain any back-story at all for why the change is done in\n> the way it is.\n>\n> _You_ may understand it _right now_, but that is not the primary\n> audience of the message. The primary audience is somebody else a year\n> from now who is wondering why this patch was done the way it was.\n\nWho would be this person? Somebody who wonders why this test is using\n\"feature done\"? I doubt such a person would exist, as using this\nfeature is standard, as can be seen below this chunk. *If* the test\nwas *not* using this \"feature done\", *then* sure, an explanation would\nbe needed.\n\nBut why is this test doing something expected is not a question\nanybody would benefit from asking.\n\n> When\n> they are trying to find out why git does not detect errors in a helper,\n> and they notice that our test for failure only check the \"done\" case,\n> isn't it more helpful to say \"we considered the other case, but it was\n> not worth fixing\" rather than leaving them to guess?\n\nIf you are worried about such hypothetical people, they would be\nbetter served by a comment in the source code of the test, or even\nbetter, the c file, or even better, to document that remote helpers\nshould use this feature. But wait:\n\n---\nJust like 'push', a batch sequence of one or more 'import' is\nterminated with a blank line. For each batch of 'import', the remote\nhelper should produce a fast-import stream terminated by a 'done'\ncommand.\n---\n\nSo it's already explained, if somebody fails to follow this\ndocumentation, it's dubious a commit message that introduces a test\nwould help. Surely, the writer of this bad remote helper would _never_\nlook there.\n\n> I may be more verbose than necessary in some of my commit messages, but\n> I would much rather err on the side of explaining too much than too\n> little.\n\nI wouldn't. The only thing an overload of information achieves is that\nthe reader would simply skip or skim it.\n\n>> > export)\n>> > + if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n>> > + then\n>> > + # consume input so fast-export doesn't get SIGPIPE;\n>>\n>> I think this is explanation enough.\n>>\n>> > + # git would also notice that case, but we want\n>> > + # to make sure we are exercising the later\n>> > + # error checks\n>>\n>> I don't understand what is being said here. What is \"that case\"?\n>\n> The case that fast-export gets SIGPIPE.\n\nIf we are trying to avoid SIGPIPE wouldn't that imply that git notices\nthe SIGPIPE?\n\n> # consume input so fast-export doesn't get SIGPIPE;\n> # we do not technically need to do so in order for\n> # git to notice the failure to export, as it will\n> # detect problems either with fast-export or with\n> # the helper failing to report ref status. But since\n> # we are trying to demonstrate that the latter\n> # check works, we must avoid the SIGPIPE, which would\n> # trigger the former.\n\n# consume input so fast-export doesn't get SIGPIPE; we want to test\nthe remote-helper's code after fast-export.\n\n--\nFelipe Contreras\n"},{"id":"213970","messageId":"20130411165937.GA1255@sigill.intra.peff.net","threadId":"33432","inReplyTo":"CAMP44s2-4i_tSzz8Y88_YnK5d1AjNoTqOa7eXZ0W5Vzk9Uosng@mail.gmail.com","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-11T16:59:37Z","receivedAt":"2013-04-11T16:59:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote:\n\n> > I am OK with adding the test for import as a separate patch. What I am\n> > not OK with (and this goes for the rest of the commit message, too) is\n> > failing to explain any back-story at all for why the change is done in\n> > the way it is.\n> >\n> > _You_ may understand it _right now_, but that is not the primary\n> > audience of the message. The primary audience is somebody else a year\n> > from now who is wondering why this patch was done the way it was.\n> \n> Who would be this person? Somebody who wonders why this test is using\n> \"feature done\"? I doubt such a person would exist, as using this\n> feature is standard, as can be seen below this chunk. *If* the test\n> was *not* using this \"feature done\", *then* sure, an explanation would\n> be needed.\n\nIf it was so obvious, why did your initial patch not use \"feature done\"?\nIf it was so obvious, why did our email discussion go back and forth so\nmany times before arriving at this patch?\n\nIt was certainly not obvious to me when this email thread started. So in\nresponse to your question: *I* am that person. I was him two weeks ago,\nand there is a good chance that I will be him a year from now. Much of\nmy work on git is spent tracking down bugs in older code, and those\ncommit messages are extremely valuable to me in understanding what\nhappened at the time.\n\nBut I give up on you. I find most of your commit messages lacking in\ndetails and motivation, making assumptions that the reader is as\nfamiliar with the code when reading the commit as you are when you wrote\nit. I tried to help by suggesting in review that you elaborate. That\ndidn't work. So I tried to help by writing the text myself. But clearly\nI am not going to convince you that it is valuable, even if it requires\nno work at all from you, so I have nothing else to say on the matter.\n\n-Peff\n"},{"id":"213980","messageId":"CAMP44s1KgpT5YGwAr2KAToaoB6rUmtM3ocA-OtFSGfOzudx5RA@mail.gmail.com","threadId":"33432","inReplyTo":"20130411165937.GA1255@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-11T17:57:34Z","receivedAt":"2013-04-11T17:57:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 11, 2013 at 11:59 AM, Jeff King wrote:\n> On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote:\n>\n>> > I am OK with adding the test for import as a separate patch. What I am\n>> > not OK with (and this goes for the rest of the commit message, too) is\n>> > failing to explain any back-story at all for why the change is done in\n>> > the way it is.\n>> >\n>> > _You_ may understand it _right now_, but that is not the primary\n>> > audience of the message. The primary audience is somebody else a year\n>> > from now who is wondering why this patch was done the way it was.\n>>\n>> Who would be this person? Somebody who wonders why this test is using\n>> \"feature done\"? I doubt such a person would exist, as using this\n>> feature is standard, as can be seen below this chunk. *If* the test\n>> was *not* using this \"feature done\", *then* sure, an explanation would\n>> be needed.\n>\n> If it was so obvious, why did your initial patch not use \"feature done\"?\n\nBecause I didn't want to test the obvious, I wanted to test something else.\n\n> If it was so obvious, why did our email discussion go back and forth so\n> many times before arriving at this patch?\n\nThis patch has absolutely nothing to do with that, in fact, forget\nabout it, such a minor check is not worth this time and effort:\nhttp://article.gmane.org/gmane.comp.version-control.git/220899\n\n> It was certainly not obvious to me when this email thread started. So in\n> response to your question: *I* am that person. I was him two weeks ago,\n> and there is a good chance that I will be him a year from now.\n\nNo, you are not. I didn't send a patch with \"feature done\" originally,\nthe only reason you wondered about the patch with \"feature done\" is\nthat you saw one without it. It will _never_ happen again.\n\n> Much of\n> my work on git is spent tracking down bugs in older code, and those\n> commit messages are extremely valuable to me in understanding what\n> happened at the time.\n\nLets make a bet. Let's push the simpler version, and when you hit this\ncommit message retrospectively and find that you don't understand what\nis happening, I loose, and I will forever accept verbose commit\nmessages. It will never happen.\n\n> But I give up on you. I find most of your commit messages lacking in\n> details and motivation, making assumptions that the reader is as\n> familiar with the code when reading the commit as you are when you wrote\n> it. I tried to help by suggesting in review that you elaborate. That\n> didn't work. So I tried to help by writing the text myself. But clearly\n> I am not going to convince you that it is valuable, even if it requires\n> no work at all from you, so I have nothing else to say on the matter.\n\nMe neither. I picked your solution, but that's not enough, you\n*always* want me to do EXACTLY what you want, and never argue back.\n\nIt's not going to happen. There's nothing wrong with disagreeing.\n\nCheers.\n\n--\nFelipe Contreras\n"},{"id":"213990","messageId":"7vfvywj4au.fsf@alter.siamese.dyndns.org","threadId":"33432","inReplyTo":"CAMP44s02K5ydKLNi0umMkuAicoVTWyCdVfjs0yssCa2oyFShGQ@mail.gmail.com","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-11T18:44:09Z","receivedAt":"2013-04-11T18:44:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Wed, Apr 10, 2013 at 4:15 PM, Jeff King wrote:\n>> From: Felipe Contreras \n>>\n>> If a push fails because the remote-helper died (with\n>> fast-export), the user does not see any error message. We do\n\nI agree with you that s/does not see/may not see/ would be more\nhelpful here, so I'll squash it in while queuing.\n\n>> In the long run, it may make more sense to propagate the\n>> error back up to push, so that it can present the usual\n>> status table and give a nicer message. But this is a much\n>> simpler fix that can help immediately.\n>\n> Yes it might, and it might make sense to rewrite much of this code,\n> but that's not relevant.\n\nIt is a good reminder for people who later inspect this part of the\ncode and wonder if it was a conscious design choice not to propagate\nthe error or just being \"simple and sufficient for now\", I think.\nIt would help them by making it clear that it is the latter, no?\n\n> ... I think it might\n> be possible enforce remote-helpers to implement the \"done\" feature,\n> and we might want to do that later.\n\nYes, all these are possible and I think writing it down explicitly\nwill serve as a reminder for our future selves, I think.\n\n>> + if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n>> + then\n>> + # consume input so fast-export doesn't get SIGPIPE;\n>\n> I think this is explanation enough.\n>\n>> + # git would also notice that case, but we want\n>> + # to make sure we are exercising the later\n>> + # error checks\n>\n> I don't understand what is being said here. What is \"that case\"?\n\nIn my first reading, it felt to me that it was natural to interpret\nthat this is \"even if we didn't have this loop that avoids killing\nfast-export with SIGPIPE, we would notice death of fast-export by\nSIGPIPE\".\n"},{"id":"213991","messageId":"7vbo9kj41y.fsf@alter.siamese.dyndns.org","threadId":"33432","inReplyTo":"CAMP44s1KgpT5YGwAr2KAToaoB6rUmtM3ocA-OtFSGfOzudx5RA@mail.gmail.com","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-11T18:49:29Z","receivedAt":"2013-04-11T18:49:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Thu, Apr 11, 2013 at 11:59 AM, Jeff King wrote:\n>\n>> But I give up on you. I find most of your commit messages lacking in\n>> details and motivation, making assumptions that the reader is as\n>> familiar with the code when reading the commit as you are when you wrote\n>> it. I tried to help by suggesting in review that you elaborate. That\n>> didn't work. So I tried to help by writing the text myself. But clearly\n>> I am not going to convince you that it is valuable, even if it requires\n>> no work at all from you, so I have nothing else to say on the matter.\n>\n> Me neither. I picked your solution, but that's not enough, you\n> *always* want me to do EXACTLY what you want, and never argue back.\n>\n> It's not going to happen. There's nothing wrong with disagreeing.\n\nHeh, it seems that I was late for the party.\n\nWriting only minimally sufficient in the log messages is fine for\nyour own project. We won't decide nor dictate the policy for your\nproject for you.\n\nBut _this_ project wants its log messages to be understandable by\npeople who you may disagree with and who may have shorter memory\nspan than you do. Disagreeing with that policy is fine. You need\nto learn to disagree but accept to be part of the project.\n\nThanks.\n"},{"id":"214008","messageId":"CAMP44s2QJJnSRVVJscLsTnXk5zdGbA2utefF5SO7=90+ttENew@mail.gmail.com","threadId":"33432","inReplyTo":"7vfvywj4au.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-11T21:21:15Z","receivedAt":"2013-04-11T21:21:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 11, 2013 at 1:44 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n\n>>> In the long run, it may make more sense to propagate the\n>>> error back up to push, so that it can present the usual\n>>> status table and give a nicer message. But this is a much\n>>> simpler fix that can help immediately.\n>>\n>> Yes it might, and it might make sense to rewrite much of this code,\n>> but that's not relevant.\n>\n> It is a good reminder for people who later inspect this part of the\n> code and wonder if it was a conscious design choice not to propagate\n> the error or just being \"simple and sufficient for now\", I think.\n> It would help them by making it clear that it is the latter, no?\n\nNo. Design choices is what code comments are for, of which Git only\nhas too few, according to ohloh[1]. No wonder they are so few, people\nare spending time writing novels on commit messages and forgetting\nthere's also code where you should clarify things.\n\n>> ... I think it might\n>> be possible enforce remote-helpers to implement the \"done\" feature,\n>> and we might want to do that later.\n>\n> Yes, all these are possible and I think writing it down explicitly\n> will serve as a reminder for our future selves, I think.\n\nYes, but not writing them here. By spending so much time in commit\nmessages you neglect the code, and the wiki (which is actually the\nplace to write these things on.\n\nAnd if all you want is to write them down, we already did, right here.\nThere's no need to punish the readers of the commit messages in the\nfuture only so we can flex our memory, because we already did.\n\nAnd if you must, you might was well label them with \"REMINDER\", no,\nwait, that's what \"TODO\" comments are for, where people can see them,\nand not *forget* them.\n\nCheers.\n\n[1] https://www.ohloh.net/p/git/factoids#FactoidCommentsLow\n\n--\nFelipe Contreras\n"},{"id":"214010","messageId":"CAMP44s1EJO3gMyb-SCGL3mWQOgsgYDb87e2mx3spO=V71hs+=g@mail.gmail.com","threadId":"33432","inReplyTo":"7vbo9kj41y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-11T21:35:00Z","receivedAt":"2013-04-11T21:35:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 11, 2013 at 1:49 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Thu, Apr 11, 2013 at 11:59 AM, Jeff King wrote:\n>>\n>>> But I give up on you. I find most of your commit messages lacking in\n>>> details and motivation, making assumptions that the reader is as\n>>> familiar with the code when reading the commit as you are when you wrote\n>>> it. I tried to help by suggesting in review that you elaborate. That\n>>> didn't work. So I tried to help by writing the text myself. But clearly\n>>> I am not going to convince you that it is valuable, even if it requires\n>>> no work at all from you, so I have nothing else to say on the matter.\n>>\n>> Me neither. I picked your solution, but that's not enough, you\n>> *always* want me to do EXACTLY what you want, and never argue back.\n>>\n>> It's not going to happen. There's nothing wrong with disagreeing.\n>\n> Heh, it seems that I was late for the party.\n>\n> Writing only minimally sufficient in the log messages is fine for\n> your own project. We won't decide nor dictate the policy for your\n> project for you.\n>\n> But _this_ project wants its log messages to be understandable by\n> people who you may disagree with and who may have shorter memory\n> span than you do.\n\nHaving a shorter memory span is irrelevant when you are _never_ going\nto go back and ask the question the commit message is answering. And\nif it indeed is an important question, the answer belongs in the code\ncomments.\n\n> Disagreeing with that policy is fine. You need\n> to learn to disagree but accept to be part of the project.\n\nYeah, I accept that you will commit whatever you want, but I still\ndon't think this verbosity serves the purpose you think it serves.\nSome one-liners deserve pages of commit messages, but this one is not\none of them. People are easily deceived, and because you saw one\ncommit message that needed more information, you think all of them do,\nbut no, some don't don't, and this is one of them. It's not serving\nany real purpose.\n\n--\nFelipe Contreras\n"},{"id":"214018","messageId":"7vd2u0hdmj.fsf@alter.siamese.dyndns.org","threadId":"33432","inReplyTo":"CAMP44s2QJJnSRVVJscLsTnXk5zdGbA2utefF5SO7=90+ttENew@mail.gmail.com","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-11T23:05:40Z","receivedAt":"2013-04-11T23:05:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> And if you must, you might was well label them with \"REMINDER\", no,\n> wait, that's what \"TODO\" comments are for, where people can see them,\n> and not *forget* them.\n\nYeah, good point.\n"},{"id":"214124","messageId":"CAMP44s1pZW6OJ2nkegKFQq6=npPSiD4dX_z37t63B9baaFW16w@mail.gmail.com","threadId":"33432","inReplyTo":"7vd2u0hdmj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-13T05:42:29Z","receivedAt":"2013-04-13T05:42:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 11, 2013 at 6:05 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> And if you must, you might was well label them with \"REMINDER\", no,\n>> wait, that's what \"TODO\" comments are for, where people can see them,\n>> and not *forget* them.\n>\n> Yeah, good point.\n\nMoreover, I think there's a clear double standard. Consider this commit:\n\ncommit 99d3206010ba1fcc9311cbe8376c0b5e78f4a136\nAuthor: Antoine Pelisse \nDate: Sat Mar 23 18:23:28 2013 +0100\n\n combine-diff: coalesce lost lines optimally\n\n This replaces the greedy implementation to coalesce lost lines by using\n dynamic programming to find the Longest Common Subsequence.\n\n The O(n²) time complexity is obviously bigger than previous\n implementation but it can produce shorter diff results (and most likely\n easier to read).\n\n List of lost lines is now doubly-linked because we reverse-read it when\n reading the direction matrix.\n\nThe commit message is 9 lines, and the diffstat 320 insertions(+), 64\ndeletions(-). Moreover, there are some important bits of information\non the mailing list that never made it to the commit message:\n\n---\nBest-case analysis:\nAll p parents have the same n lines.\nWe will find LCS and provide a n lines (the same lines) new list in\nO(n²), and then run it again in O(n²) with the next parent, etc.\nIt will end-up being O(pn²).\n\nWorst-case analysis:\nAll p parents have no lines in common.\nWe will find LCS and provide a 2n new list in O(n²).\nThen we run it again in O(2n x n), and again O(3n x n), etc, until\nO(pn x n).\nWhen we sum these all, we end-up with O(p² x n²)\n---\n\n---\nUnfortunately on a commit that would remove A LOT of lines (10000)\nfrom 7 parents, the times goes from 0.01s to 1.5s... I'm pretty sure\nthat scenario is quite uncommon though.\n---\n\nThis is not mentioned in the commit message; on which situations this\nimplementation would be worst and why it's OK either way.\n\n---\nAs you can see the last test is broken because the solution is not\noptimal for more than two parents. It would probably require to extend\nthe dynamic programming to a k-dimension matrix (for k parents) but the\nresult would end-up being O(n^k) (when removing n consecutives lines\nfrom p parents). I'm not sure there is any better solution known yet to\nthe k-LCS problem.\nImplementing the dynamic solution with the k-dimension matrix would\nprobably require to re-hash the strings (I guess it's already done by\nxdiff), as the number of string comparisons would increase.\n---\n\nThe fact that the last test is broken is not mentioned at all.\n\nNow let's compare to the final version of my patch which is 19 lines\n40 insertions(+), 1 deletion(-). The ration of commit message lines\nvs. code changed lines is 19/41(0.46) whereas Antoine's patch is\n3/128(0.02), a difference of over 19 times. Granted, some single-line\nchanges do require a good chunk of explanation, but this is not one of\nthem; this single line patch doesn't even change the behavior of the\ncode, simply changes a silent error exit to a verbose error exit,\nthat's all. Antoine's patch has a lot more potential to trigger\nsomething unexpected.\n\nAnd the chances that somebody would have to look at Antoine's patch is\nquite high, especially since a failing test-case is introduced. The\nchances that anybody would look at mine are very very low.\n\nSo either Antoine's commit message was fine, and so was mine, or it\nwas sorely lacking explanation.\n\nTo me, the reality is obvious: my patch didn't require such a big\ncommit message, the short version was fine, the only reason Jeff King\ninsisted on a longer version is because the patch came from me.\nAntoine's patch might have benefited from a little more explanation,\nbut not every issue that was discussed in the mailing list was\nnecessary (in my patch virtually every issue discussed was added to\nthe commit message).\n\nThis is the definition of double standard.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"214126","messageId":"20130413060031.GA22374@sigill.intra.peff.net","threadId":"33432","inReplyTo":"CAMP44s1pZW6OJ2nkegKFQq6=npPSiD4dX_z37t63B9baaFW16w@mail.gmail.com","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-13T06:00:31Z","receivedAt":"2013-04-13T06:00:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:\n\n> To me, the reality is obvious: my patch didn't require such a big\n> commit message, the short version was fine, the only reason Jeff King\n> insisted on a longer version is because the patch came from me.\n\nGet over yourself. The reason I suggested a longer commit message for\nyour commit is because after spending several hours figuring out what\nthe current code did, and what it should be doing instead, I wanted to\ndocument that effort so that I and other readers did not have to do it\nagain later. I didn't even review the other patch you mention, so I\ncould not possibly have come to the same point with it.\n\nBut hey, if you want to have paranoid fantasies that I'm persecuting you\n(by writing the longer commit messages for you!), go ahead.\n\nIf you don't want me to review your patches, that's fine by me, too; our\ndiscussions often end up frustrating, and it's clear we do not agree on\nvery much with respect to process or design. But if you don't want that,\nplease stop cc'ing me when you send out the patches.\n\n-Peff\n"},{"id":"214128","messageId":"CAMP44s3gOXvHknN1yXQcDYP=OBfjm7=eJnSkh5cj5QJNOarEWQ@mail.gmail.com","threadId":"33432","inReplyTo":"20130413060031.GA22374@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-13T06:43:26Z","receivedAt":"2013-04-13T06:43:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 13, 2013 at 1:00 AM, Jeff King wrote:\n> On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:\n>\n>> To me, the reality is obvious: my patch didn't require such a big\n>> commit message, the short version was fine, the only reason Jeff King\n>> insisted on a longer version is because the patch came from me.\n>\n> Get over yourself. The reason I suggested a longer commit message for\n> your commit is because after spending several hours figuring out what\n> the current code did, and what it should be doing instead, I wanted to\n> document that effort so that I and other readers did not have to do it\n> again later. I didn't even review the other patch you mention, so I\n> could not possibly have come to the same point with it.\n\nThe double standard might not come from you, perhaps you subject all\nthe patches you review to the same standard, it comes from the fact\nthat the patches you review have an unfair disadvantage.\n\n> But hey, if you want to have paranoid fantasies that I'm persecuting you\n> (by writing the longer commit messages for you!), go ahead.\n\nYou don't persecute me, you persecute my patches. I could almost\npicture the moment you see a patch is coming from me, you have already\ndecided to rewrite the commit message, even before reading it. Antoine\nis not me, so you simply didn't review that patch.\n\n> If you don't want me to review your patches, that's fine by me, too; our\n> discussions often end up frustrating, and it's clear we do not agree on\n> very much with respect to process or design. But if you don't want that,\n> please stop cc'ing me when you send out the patches.\n\nThis comment was directed towards Junio, I do hope he is able to see\nthe double standard. As for you, I think your reviews have value, but\nI also think you dwelling in irrelevant details do slow things down,\nwhich is not too bad, what is bad is that you assume that your\nopinions are facts (e.g. the commit message need to be bigger), and\nget angry when somebody disagrees with them.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"214155","messageId":"7v38ut7ilq.fsf@alter.siamese.dyndns.org","threadId":"33432","inReplyTo":"CAMP44s3gOXvHknN1yXQcDYP=OBfjm7=eJnSkh5cj5QJNOarEWQ@mail.gmail.com","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-14T05:23:22Z","receivedAt":"2013-04-14T05:23:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Sat, Apr 13, 2013 at 1:00 AM, Jeff King wrote:\n>> On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:\n>>\n>>> To me, the reality is obvious: my patch didn't require such a big\n>>> commit message, the short version was fine, the only reason Jeff King\n>>> insisted on a longer version is because the patch came from me.\n>>\n>> Get over yourself. The reason I suggested a longer commit message for\n>> your commit is because after spending several hours figuring out what\n>> the current code did, and what it should be doing instead, I wanted to\n>> document that effort so that I and other readers did not have to do it\n>> again later.\n> ...\n> The double standard might not come from you, perhaps you subject all\n> the patches you review to the same standard, it comes from the fact\n> that the patches you review have an unfair disadvantage.\n\nThere are reviewers who share the basic values [*1*] with I and the\ntradition of this project and whose judgement I can trust.\n\nWhen somebody (like Peff) whose judgement I trust spends time to\nreview a series, and writes his thought process to the degree that\nhe thinks is appropriate, that's his judgement and I trust it.\n\nYou cannot expect perfect evenness from multiple people. For that\nmatter, you cannot expect perfect evenness even from a single\nperson, either.\n\nWhen reviewing a proposed change to an area that I am intimately\nfamiliar with, I may immediately know some subtleties involved in a\nproposed solution without even reading the patch, and I may not even\nrealize that such subtleties are hard to know without being somebody\nwho are already familiar with that part of the codebase, and either\nin-code comment or in the log message may better spelled them out.\nOn the other hand, when the change is in another area that I am not\nfamiliar with, I may request more explanation, if only for me to\nunderstand the issue.\n\nI try to avoid the \"I may know too well\" pitfalls, but I am not\nperfect. I will not speak for Peff or any other reviewers whose\njugement I trust, but I would be very surprised if any of them\nclaimed he is perfectly even.\n\n\"Double\" may only be showing that we do not have enough trusted\nmaintainers; ideally I would like it to have \"Triple\" or more.\n\n\n[Footnote]\n\n*1* A few examples of core values.\n\n - we should make sure that future developers who wonder why a part\n of the code is how it is can find out what thought process\n brought the code into the current shape.\n\n - when we add something, we try not to overengineer or to shoot for\n unattainable perfection, but we still try to make sure we will\n not paint ourselves into an unescapable corner when we later want\n to extend it.\n"},{"id":"214216","messageId":"CAMP44s2Ksi_nNm8f+YTdceSSavtiPODHKw-4eASeEeCWW2N91g@mail.gmail.com","threadId":"33432","inReplyTo":"7v38ut7ilq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] transport-helper: report errors properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-14T15:54:07Z","receivedAt":"2013-04-14T15:54:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Apr 14, 2013 at 12:23 AM, Junio C Hamano wrote:\n\n> \"Double\" may only be showing that we do not have enough trusted\n> maintainers; ideally I would like it to have \"Triple\" or more.\n\nA double or triple review raises *a single* standard higher, but\nhaving more than one standard is never good. But apparently your trust\nin Jeff means more than caring about the double standard.\n\nBut good to know, my patches will always have an unfair disadvantages\nto everybody else's.\n\n> - when we add something, we try not to overengineer or to shoot for\n> unattainable perfection, but we still try to make sure we will\n> not paint ourselves into an unescapable corner when we later want\n> to extend it.\n\nAnd there is such a thing as being too cautious, to the point where\nyou walk WAY too slowly for the fear of tumbling, because it happened\na few times in the fast, when you were running. But how would you even\nknow that you are being too cautions? If you don't dare to walk a bit\nfaster to find out.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}