From: Jeff King <peff@peff.net>
Date: Wed, 13 Jul 2011 21:06:36 GMT
Subject: Re: [PATCH] Do not log unless all connect() attempts fail
Message-ID: <20110713210636.GF31965@sigill.intra.peff.net>
In-Reply-To: <CABPQNSaPXmHE1qECUbG9oWU43HbAXxAY42T1P=MNHgkkWM936w@mail.gmail.com>

On Wed, Jul 13, 2011 at 11:23:33AM +0200, Erik Faye-Lund wrote:

> >  static int git_tcp_connect_sock(char *host, int flags)
> >  {
> > +       struct strbuf error_message = STRBUF_INIT;
> >        int sockfd = -1, saved_errno = 0;
> >        const char *port = STR(DEFAULT_GIT_PORT);
> >        struct addrinfo hints, *ai0, *ai;
> > @@ -225,11 +226,8 @@ 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));
> > +                       strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
> > +                               host, cnt, ai_name(ai), strerror(saved_errno));
> >                        close(sockfd);
> >                        sockfd = -1;
> >                        continue;
> > @@ -242,11 +240,13 @@ static int git_tcp_connect_sock(char *host, int flags)
> >        freeaddrinfo(ai0);
> >
> >        if (sockfd < 0)
> > -               die("unable to connect a socket (%s)", strerror(saved_errno));
> > +               die("unable to connect to %s:\n%s", host, error_message.buf);
> >
> 
> This kills the output from the case where "sockfd < 0" evaluates to
> true for the last entry in ai, no (just above your second hunk), no?
> In that case errno gets copied to saved_errno, and the old output
> would do strerror(old_errno), but now you just print the log you've
> gathered, and don't even look at saved_errno.

But that's OK, because the value of that saved_errno is in the gathered
log, isn't it? So the output is not identical, but IMHO it's much
better. It's gone from:

  $ git fetch git://example.com/foo
  example.com[0: 192.0.32.10]: errno=Connection timed out
  example.com[0: 2620:0:2d0:200::10]: errno=Network is unreachable
  fatal: unable to connect a socket (Network is unreachable)

to:

  $ git fetch git://example.com/foo
  fatal: unable to connect to example.com:
  example.com[0: 192.0.32.10]: errno=Connection timed out
  example.com[0: 2620:0:2d0:200::10]: errno=Network is unreachable

IMHO, the "Network is unreachable" at the end of the first one is just
noise; it's a duplicate of what was already printed, and it may not be
the errno value that is most interesting to you.

> If this is intentional then you should probably kill the saved_errno
> variable also, it's rendered pointless by this patch.

That is sensible, though, I think.

-Peff