{"thread":{"id":"27594","subject":"[PATCH] Only print an error for the last connect() failure","startedAt":"2011-06-09T16:18:10Z","lastAt":"2011-06-09T17:30:18Z","messageCount":5,"participants":["Dave Zarzycki","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"169784","messageId":"13539E82-3E8D-4210-9A3A-DD83F0CA6F85@apple.com","threadId":"27594","inReplyTo":null,"subject":"[PATCH] Only print an error for the last connect() failure","fromName":"Dave Zarzycki","fromEmail":"zarzycki@apple.com","sentAt":"2011-06-09T16:18:10Z","receivedAt":"2011-06-09T16:18:10Z","isPatch":true,"sender":{"key":"zarzycki@apple.com","avatar":"https://avatars.githubusercontent.com/u/1071982?v=4"},"body":"IPv6 hosts are often unreachable on the primarily IPv4 Internet and\ntherefore we shouldn't print an error if there are still other hosts we\ncan try to connect() to. This helps \"git fetch --quiet\" stay quiet.\n---\n connect.c |   12 +++++++-----\n 1 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 2119c3f..7f70ce7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -225,11 +225,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n \t\t}\n \t\tif (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n \t\t\tsaved_errno = errno;\n-\t\t\tfprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n-\t\t\t\thost,\n-\t\t\t\tcnt,\n-\t\t\t\tai_name(ai),\n-\t\t\t\tstrerror(saved_errno));\n+\t\t\tif (ai->ai_next == NULL) {\n+\t\t\t\tfprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n+\t\t\t\t\thost,\n+\t\t\t\t\tcnt,\n+\t\t\t\t\tai_name(ai),\n+\t\t\t\t\tstrerror(saved_errno));\n+\t\t\t}\n \t\t\tclose(sockfd);\n \t\t\tsockfd = -1;\n \t\t\tcontinue;\n-- \n1.7.6.rc1.1.g2e27\n"},{"id":"169787","messageId":"20110609163340.GD25885@sigill.intra.peff.net","threadId":"27594","inReplyTo":"13539E82-3E8D-4210-9A3A-DD83F0CA6F85@apple.com","subject":"Re: [PATCH] Only print an error for the last connect() failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-09T16:33:41Z","receivedAt":"2011-06-09T16:33:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2011 at 09:18:10AM -0700, Dave Zarzycki wrote:\n\n> IPv6 hosts are often unreachable on the primarily IPv4 Internet and\n> therefore we shouldn't print an error if there are still other hosts we\n> can try to connect() to. This helps \"git fetch --quiet\" stay quiet.\n> ---\n>  connect.c |   12 +++++++-----\n>  1 files changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/connect.c b/connect.c\n> index 2119c3f..7f70ce7 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -225,11 +225,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n>  \t\t}\n>  \t\tif (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n>  \t\t\tsaved_errno = errno;\n> -\t\t\tfprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n> -\t\t\t\thost,\n> -\t\t\t\tcnt,\n> -\t\t\t\tai_name(ai),\n> -\t\t\t\tstrerror(saved_errno));\n> +\t\t\tif (ai->ai_next == NULL) {\n> +\t\t\t\tfprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n> +\t\t\t\t\thost,\n> +\t\t\t\t\tcnt,\n> +\t\t\t\t\tai_name(ai),\n> +\t\t\t\t\tstrerror(saved_errno));\n> +\t\t\t}\n\nI agree being noisy about early failures when we succeed later is a bad\nthing. But when we fail completely, doesn't your code now mask early\nfailures and print only the final one? The early failures might be the\nimportant ones for the user.\n\nSo perhaps we should do something instead like:\n\n  struct strbuf error_message = STRBUF_INIT;\n  ...\n  if (connect(...) < 0) {\n          strbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n                      host, cnt, ai_name(ai), strerror(errno));\n          ...\n  }\n\n  if (sockfd < 0)\n          die(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n  strbuf_release(&error_message);\n\n-Peff\n"},{"id":"169788","messageId":"767D7D04-6089-4C7B-A532-C5EC9FE0CCC6@apple.com","threadId":"27594","inReplyTo":"20110609163340.GD25885@sigill.intra.peff.net","subject":"Re: [PATCH] Only print an error for the last connect() failure","fromName":"Dave Zarzycki","fromEmail":"zarzycki@apple.com","sentAt":"2011-06-09T16:49:28Z","receivedAt":"2011-06-09T16:49:28Z","isPatch":true,"sender":{"key":"zarzycki@apple.com","avatar":"https://avatars.githubusercontent.com/u/1071982?v=4"},"body":"On Jun 9, 2011, at 9:33 AM, Jeff King wrote:\n\n> On Thu, Jun 09, 2011 at 09:18:10AM -0700, Dave Zarzycki wrote:\n> \n>> IPv6 hosts are often unreachable on the primarily IPv4 Internet and\n>> therefore we shouldn't print an error if there are still other hosts we\n>> can try to connect() to. This helps \"git fetch --quiet\" stay quiet.\n>> ---\n>> connect.c |   12 +++++++-----\n>> 1 files changed, 7 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/connect.c b/connect.c\n>> index 2119c3f..7f70ce7 100644\n>> --- a/connect.c\n>> +++ b/connect.c\n>> @@ -225,11 +225,13 @@ static int git_tcp_connect_sock(char *host, int flags)\n>> \t\t}\n>> \t\tif (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n>> \t\t\tsaved_errno = errno;\n>> -\t\t\tfprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n>> -\t\t\t\thost,\n>> -\t\t\t\tcnt,\n>> -\t\t\t\tai_name(ai),\n>> -\t\t\t\tstrerror(saved_errno));\n>> +\t\t\tif (ai->ai_next == NULL) {\n>> +\t\t\t\tfprintf(stderr, \"%s[%d: %s]: errno=%s\\n\",\n>> +\t\t\t\t\thost,\n>> +\t\t\t\t\tcnt,\n>> +\t\t\t\t\tai_name(ai),\n>> +\t\t\t\t\tstrerror(saved_errno));\n>> +\t\t\t}\n> \n> I agree being noisy about early failures when we succeed later is a bad\n> thing. But when we fail completely, doesn't your code now mask early\n> failures and print only the final one? The early failures might be the\n> important ones for the user.\n> \n> So perhaps we should do something instead like:\n> \n>  struct strbuf error_message = STRBUF_INIT;\n>  ...\n>  if (connect(...) < 0) {\n>          strbuf_addf(&error_message, \"%s[%d: %s]: errno=%s\\n\",\n>                      host, cnt, ai_name(ai), strerror(errno));\n>          ...\n>  }\n> \n>  if (sockfd < 0)\n>          die(\"unable to connect to %s:\\n%s\", host, error_message.buf);\n>  strbuf_release(&error_message);\n\nI'm happy to make that change.\n\nPersonally speaking, I don't think we're masking failures any more than git is masking failures when it doesn't find a ref in .git/refs and it falls back to .git/packed-refs. This fallback is by design, and the same is true of any classic loop around getaddrinfo(). Of course, reasonable people may disagree about what the \"right\" thing to do here is. :-)\n\nIn any case, I'll update the patch later today.\n\ndavez\n"},{"id":"169789","messageId":"20110609170554.GA29760@sigill.intra.peff.net","threadId":"27594","inReplyTo":"767D7D04-6089-4C7B-A532-C5EC9FE0CCC6@apple.com","subject":"Re: [PATCH] Only print an error for the last connect() failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-09T17:05:54Z","receivedAt":"2011-06-09T17:05:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2011 at 09:49:28AM -0700, Dave Zarzycki wrote:\n\n> I'm happy to make that change.\n> \n> Personally speaking, I don't think we're masking failures any more\n> than git is masking failures when it doesn't find a ref in .git/refs\n> and it falls back to .git/packed-refs. This fallback is by design, and\n> the same is true of any classic loop around getaddrinfo(). Of course,\n> reasonable people may disagree about what the \"right\" thing to do here\n> is. :-)\n\nWhen the fallback actually _works_, then yes, there's no reason to say\nanything. But when the fallback fails, it can be useful to get\ninformation on each of the steps. In your refs analogy, it would just be\n\"I couldn't look up this ref; I tried .git/refs and packed refs\". Which\nwould be the same every time, so it's not really interesting enough to\nprint.\n\nBut in the case of host lookup (especially with ipv6), it may be very\nuseful to say \"I tried this address, and it failed for this reason; then\nI tried this address, and it failed for this reason\". If I see:\n\n  $ git fetch git://example.com/foo\n  example.com[0: 192.0.32.10]: errno=Connection timed out\n  example.com[0: 2620:0:2d0:200::10]: errno=Network is unreachable\n  fatal: unable to connect a socket (Network is unreachable)\n\nthen I don't care about the second error. Of course the IPv6 network is\nunreachable; I don't have IPv6 connectivity. The first line contains the\nuseful information. But with your patch, it won't be shown at all.\n\nWe should perhaps also always print intermediate error messages as we\nget them in verbose mode. Even if we succeed, seeing connection timeouts\non earlier addresses can explain long pauses before success (but I agree\nthey shouldn't be shown by default).\n\n-Peff\n"},{"id":"169796","messageId":"7vhb7yykt1.fsf@alter.siamese.dyndns.org","threadId":"27594","inReplyTo":"767D7D04-6089-4C7B-A532-C5EC9FE0CCC6@apple.com","subject":"Re: [PATCH] Only print an error for the last connect() failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-09T17:30:18Z","receivedAt":"2011-06-09T17:30:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Zarzycki <zarzycki@apple.com> writes:\n\n> Personally speaking, I don't think we're masking failures any more than\n> git is masking failures when it doesn't find a ref in .git/refs and it\n> falls back to .git/packed-refs.\n\nThat is not even a fallback. If a loose ref is found, the corresponding\nentry in packed-refs is *STALE* and should *NEVER* be looked at.\n\nBack to the topic.\n\nIf a host advertises that it can be reached at any of these addresses,\nthen you can say these addresses are fall-back for each other.  It is fine\nnot to report about addresses we tried but didn't get connection as long\nas we manage to reach it in one of the addresses.  We do not report other\naddresses we didn't even try, and it is not useful to report earlier\naddresses we tried but didn't get the dial tone in such a case.\n\nAs Peff said, if we failed to reach any connection, reporting all\nfailed addresses would give the user a better clue when diagnosing the\nnetwork problem.\n"}]}