{"thread":{"id":"34230","subject":"[BUG] clone: regression in error messages in master","startedAt":"2013-06-20T13:16:55Z","lastAt":"2013-06-22T09:55:07Z","messageCount":9,"participants":["Ramkumar Ramachandra","Jeff King","John Szakmeister","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"221480","messageId":"CALkWK0n7S8s-ABQ1qV5JSsyhYo6=rmK1UT+uYW9hjjeWjambug@mail.gmail.com","threadId":"34230","inReplyTo":null,"subject":"[BUG] clone: regression in error messages in master","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-20T13:16:55Z","receivedAt":"2013-06-20T13:16:55Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nSo this should explain the problem:\n\n  # using v1.8.3.1\n  $ git clone https://google.com\n  Cloning into 'google.com'...\n  fatal: repository 'https://google.com/' not found\n\n  # using master\n  $ git clone https://google.com\n  Cloning into 'google.com'...\n  fatal: repository 'https://google.com/' not found\n  fatal: Reading from helper 'git-remote-https' failed\n\nTo figure out where the regression was coming from, I ran a bisect\nwith this script:\n\n  #!/bin/sh\n  make clean &&\n  make -j 8 &&\n  cd t &&\n  sh -v -i clone-message.sh\n\nwhere clone-message.sh is:\n\n  test_description=clone-message\n\n  . ./test-lib.sh\n\n  test_expect_success setup '\n\n  \trm -fr .git &&\n  \ttest_create_repo src &&\n  \t(\n  \t\tcd src &&\n  \t\t>file &&\n  \t\tgit add file &&\n  \t\tgit commit -m initial &&\n  \t\techo 1 >file &&\n  \t\tgit add file &&\n  \t\tgit commit -m updated\n  \t)\n\n  '\n\n  test_expect_success 'clone invalid URL' '\n  \trm -fr dst &&\n  \ttest_must_fail git clone https://google.com 2>msg &&\n  \ttest_i18ngrep \"repository .* not found\" msg &&\n  \t! test_i18ngrep \"git-remote-https\" msg\n  '\n\n  test_done\n\nThe bisect pointed me to: 81d340d4 (transport-helper: report errors\nproperly, 2013-04-10).\n\n  $ git clone https://google.com\n  Cloning into 'google.com'...\n  fatal: https://google.com/info/refs?service=git-upload-pack not\nfound: did you run git update-server-info on the server?\n  fatal: Reading from remote helper failed\n\nWhat?!  Okay, the last \"Reading from remote helper failed\" was\nintroduced by this commit; my clone-message.sh has a bug.  So I\ncommented out the first test_i18ngrep and ran it.  Result: c096955\n(transport-helper: mention helper name when it dies, 2013-04-10).\nThis is not the real culprit: it just changed the message string that\n81d340d4 originally introduced.\n\nOkay, so am I reporting a valid bug?  Going through remote-curl, I can\nsee that it dies in remote-curl.c:213 if HTTP_TARGET_MISSING.  If that\nis the case, what is the point of printing the second message about\nthe remote helper program not being present?\n\nThanks.\n"},{"id":"221485","messageId":"20130620133422.GA18200@sigill.intra.peff.net","threadId":"34230","inReplyTo":"CALkWK0n7S8s-ABQ1qV5JSsyhYo6=rmK1UT+uYW9hjjeWjambug@mail.gmail.com","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T13:34:22Z","receivedAt":"2013-06-20T13:34:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 06:46:55PM +0530, Ramkumar Ramachandra wrote:\n\n> So this should explain the problem:\n> \n>   # using v1.8.3.1\n>   $ git clone https://google.com\n>   Cloning into 'google.com'...\n>   fatal: repository 'https://google.com/' not found\n> \n>   # using master\n>   $ git clone https://google.com\n>   Cloning into 'google.com'...\n>   fatal: repository 'https://google.com/' not found\n>   fatal: Reading from helper 'git-remote-https' failed\n>\n> [...]\n> The bisect pointed me to: 81d340d4 (transport-helper: report errors\n> properly, 2013-04-10).\n\nYeah, that is a not-so-great fallout from 81d340d4. The point of that\ncommit was that we do not know whether the remote helper has printed\nanything useful; it died unexpectedly while we tried to read from it.\n\nIn this case, of course it has, and so the extra message is redundant\nand unwanted.\n\nI'm not sure if there is a good way to distinguish the two cases\n(snooping on stderr would add complexity, and is not even robust, as we\ndo not know the meaning of human-readable messages coming over stderr).\nWaiting for an \"expected\" time for the helper give us EOF does not work\neither; I think in this case we asked for a \"list\" or \"fetch\", and the\nhelper died without giving us an answer (because there is no answer to\ngive; there is no \"oops, I could not complete your request\" on the\nfetch side of the transport helper protocol).\n\nSo I'm not sure if there is a better option than reverting 81d340d4 and\nliving with the lesser of two evils (no good message when the helper\ndies silently).\n\n-Peff\n"},{"id":"221560","messageId":"CALkWK0k8-go979S9sMytBpqs_C9Jm6q6thqwHa4-GX-e5Czx=g@mail.gmail.com","threadId":"34230","inReplyTo":"20130620133422.GA18200@sigill.intra.peff.net","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-21T06:44:33Z","receivedAt":"2013-06-21T06:44:33Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> So I'm not sure if there is a better option than reverting 81d340d4 and\n> living with the lesser of two evils (no good message when the helper\n> dies silently).\n\nI dug around, but I still can't justify that there is no better\noption.  Could you write a commit message for this?\n\n-- 8< --\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 06c08a1..db9bd18 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -56,7 +56,7 @@ static int recvline_fh(FILE *helper, struct strbuf\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 helper 'git-remote-%s' failed\", name);\n+\t\texit(128);\n \t}\n\n \tif (debug)\n"},{"id":"221561","messageId":"CALkWK0ksteLBcRcn_R5JP4zsW57SXdi1Qk5DMPqheHrabfBLog@mail.gmail.com","threadId":"34230","inReplyTo":"CALkWK0k8-go979S9sMytBpqs_C9Jm6q6thqwHa4-GX-e5Czx=g@mail.gmail.com","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-21T06:46:49Z","receivedAt":"2013-06-21T06:46:49Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ramkumar Ramachandra wrote:\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 06c08a1..db9bd18 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n\nOh, and we have to remove test 23 - \"proper failure checks for\npushing\" from t5801-remote-helpers.sh.\n"},{"id":"221562","messageId":"20130621070539.GA31552@sigill.intra.peff.net","threadId":"34230","inReplyTo":"CALkWK0k8-go979S9sMytBpqs_C9Jm6q6thqwHa4-GX-e5Czx=g@mail.gmail.com","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-21T07:05:39Z","receivedAt":"2013-06-21T07:05:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 21, 2013 at 12:14:33PM +0530, Ramkumar Ramachandra wrote:\n\n> Jeff King wrote:\n> > So I'm not sure if there is a better option than reverting 81d340d4 and\n> > living with the lesser of two evils (no good message when the helper\n> > dies silently).\n> \n> I dug around, but I still can't justify that there is no better\n> option.  Could you write a commit message for this?\n\nI think it is something like this:\n\n-- >8 --\nSubject: [PATCH] transport-helper: be quiet on read errors from helpers\n\nPrior to commit 81d340d4, we did not print any error message\nif a remote transport helper died unexpectedly. If a helper\ndid not print any error message (e.g., because it crashed),\nthe user could be left confused. That commit tried to\nrectify the situation by printing a note that the helper\nexited unexpectedly.\n\nHowever, this makes a much more common case worse: when a\nhelper does die with a useful message, we print the extra\n\"Reading from 'git-remote-foo failed\" message. This can also\nend up confusing users, as they may not even know what\nremote helpers are (e.g., the fact that http support comes\nthrough git-remote-https is purely an implementation detail\nthat most users do not know or care about).\n\nSince we do not have a good way of knowing whether the\nhelper printed a useful error, and since the common failure\nmode is for it to do so, let's default to remaining quiet.\nDebuggers can dig further by setting GIT_TRANSPORT_HELPER_DEBUG.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNote that I haven't thought too hard about this; there may be a way to\ndetect for specific operations that we were expecting more data from the\nhelper and didn't get it. But even if we do want to go that route, I\nthink reverting the change to recvline_fh is probably going to be the\nfirst step.\n\n t/t5801-remote-helpers.sh | 4 +---\n transport-helper.c        | 2 +-\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 4899af3..8c4c539 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -210,9 +210,7 @@ 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 helper .git-remote-testgit. failed\" error\n+\ttest_must_fail git push --all\n \t)\n '\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 06c08a1..db9bd18 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -56,7 +56,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)\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 helper 'git-remote-%s' failed\", name);\n+\t\texit(128);\n \t}\n \n \tif (debug)\n-- \n1.8.3.rc2.14.g7eee6b3\n"},{"id":"221572","messageId":"CAEBDL5UNNHnogHb2ztq2G8OxyCNMoLia7xYFNSy+kfHD3pS1pQ@mail.gmail.com","threadId":"34230","inReplyTo":"CALkWK0n7S8s-ABQ1qV5JSsyhYo6=rmK1UT+uYW9hjjeWjambug@mail.gmail.com","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2013-06-21T10:07:07Z","receivedAt":"2013-06-21T10:07:07Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Thu, Jun 20, 2013 at 9:16 AM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> Hi,\n>\n> So this should explain the problem:\n>\n>   # using v1.8.3.1\n>   $ git clone https://google.com\n>   Cloning into 'google.com'...\n>   fatal: repository 'https://google.com/' not found\n>\n>   # using master\n>   $ git clone https://google.com\n>   Cloning into 'google.com'...\n>   fatal: repository 'https://google.com/' not found\n>   fatal: Reading from helper 'git-remote-https' failed\n\nI can see where this is confusing, but can also see how it's useful\ninformation to have.  On clone, it's probably not that useful since\nyou're looking right at the url, but I could see that information\nbeing more useful on a pull or push with the default arguments (when\nthe source and destination aren't quite as obvious).\n\n-John\n"},{"id":"221603","messageId":"7vfvwbo3hf.fsf@alter.siamese.dyndns.org","threadId":"34230","inReplyTo":"20130621070539.GA31552@sigill.intra.peff.net","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-21T16:03:40Z","receivedAt":"2013-06-21T16:03:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Note that I haven't thought too hard about this; there may be a way to\n> detect for specific operations that we were expecting more data from the\n> helper and didn't get it. But even if we do want to go that route, I\n> think reverting the change to recvline_fh is probably going to be the\n> first step.\n\nYeah, I agree with that assessment.  Will queue.\n"},{"id":"221616","messageId":"7vli63mlro.fsf@alter.siamese.dyndns.org","threadId":"34230","inReplyTo":"CAEBDL5UNNHnogHb2ztq2G8OxyCNMoLia7xYFNSy+kfHD3pS1pQ@mail.gmail.com","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-21T17:11:39Z","receivedAt":"2013-06-21T17:11:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Szakmeister <john@szakmeister.net> writes:\n\n> On Thu, Jun 20, 2013 at 9:16 AM, Ramkumar Ramachandra\n> <artagnon@gmail.com> wrote:\n>> Hi,\n>>\n>> So this should explain the problem:\n>>\n>>   # using v1.8.3.1\n>>   $ git clone https://google.com\n>>   Cloning into 'google.com'...\n>>   fatal: repository 'https://google.com/' not found\n>>\n>>   # using master\n>>   $ git clone https://google.com\n>>   Cloning into 'google.com'...\n>>   fatal: repository 'https://google.com/' not found\n>>   fatal: Reading from helper 'git-remote-https' failed\n>\n> I can see where this is confusing, but can also see how it's useful\n> information to have.  On clone, it's probably not that useful since\n> you're looking right at the url, but I could see that information\n> being more useful on a pull or push with the default arguments (when\n> the source and destination aren't quite as obvious).\n\nThe \"extra\" error messages is not the first one, but the last one,\nand the suggested revert is a proposal to remove the latter, not the\n\"repository $URL not found\".\n"},{"id":"221674","messageId":"CAEBDL5WYCdMEOsoXtxgfNDmJrkj2DR8DgTwKY72iUJjQyJ-bRw@mail.gmail.com","threadId":"34230","inReplyTo":"7vli63mlro.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] clone: regression in error messages in master","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2013-06-22T09:55:07Z","receivedAt":"2013-06-22T09:55:07Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Fri, Jun 21, 2013 at 1:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> John Szakmeister <john@szakmeister.net> writes:\n[snip]\n>> I can see where this is confusing, but can also see how it's useful\n>> information to have.  On clone, it's probably not that useful since\n>> you're looking right at the url, but I could see that information\n>> being more useful on a pull or push with the default arguments (when\n>> the source and destination aren't quite as obvious).\n>\n> The \"extra\" error messages is not the first one, but the last one,\n> and the suggested revert is a proposal to remove the latter, not the\n> \"repository $URL not found\".\n\nSorry for the confusion.  I realize they were talking about removing\nthe remote helper line.  What I meant was that with the url there,\nit's a bit easier to determine which part of git failed (http, hg, or\nbzr remote helper, for instance).  What I was trying to say was\nperhaps there are paths through here where it's really helpful to know\nthat things failed in the remote helper, so we may not want to\nwholesale remove it.  Some of remote helpers, such as ones that talk\nto foreign VCSes, do quite a bit more than just transfer data, so it\nmight be helpful to know that it's not core git that's failing but the\nremote helper.  Seeing the URL is a reminder that you're interacting\nwith a remote helper, and it may not be helpful there.  But other\ncommands don't necessarily show that to you, and it may be more\nhelpful to have that reminder that it was a remote helper that failed.\n\nI hope that makes more sense.\n\n-John\n"}]}