Re: [PATCH] Only print an error for the last connect() failure

From: Jeff King <peff@peff.net>
Date: Jun 9, 2011, 16:33 UTC
Message-ID: <20110609163340.GD25885@sigill.intra.peff.net>
In-Reply-To: <13539E82-3E8D-4210-9A3A-DD83F0CA6F85@apple.com>

On Thu, Jun 09, 2011 at 09:18:10AM -0700, Dave Zarzycki wrote:

Show 27 quoted lines

> IPv6 hosts are often unreachable on the primarily IPv4 Internet and
> therefore we shouldn't print an error if there are still other hosts we
> can try to connect() to. This helps "git fetch --quiet" stay quiet.
> ---
>  connect.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index 2119c3f..7f70ce7 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -225,11 +225,13 @@ static int git_tcp_connect_sock(char *host, int flags)
>  		}
>  		if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
>  			saved_errno = errno;
> -			fprintf(stderr, "%s[%d: %s]: errno=%s\n",
> -				host,
> -				cnt,
> -				ai_name(ai),
> -				strerror(saved_errno));
> +			if (ai->ai_next == NULL) {
> +				fprintf(stderr, "%s[%d: %s]: errno=%s\n",
> +					host,
> +					cnt,
> +					ai_name(ai),
> +					strerror(saved_errno));
> +			}

I agree being noisy about early failures when we succeed later is a bad thing. But when we fail completely, doesn't your code now mask early failures and print only the final one? The early failures might be the important ones for the user.

So perhaps we should do something instead like:

  struct strbuf error_message = STRBUF_INIT;
  ...
  if (connect(...) < 0) {
          strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
                      host, cnt, ai_name(ai), strerror(errno));
          ...
  }

  if (sockfd < 0)
          die("unable to connect to %s:\n%s", host, error_message.buf);
  strbuf_release(&error_message);

-Peff