Re: [PATCHv3 10/13] credentials: add "cache" helper

[Jeff King <peff@peff.net>]

On Mon, Jan 09, 2012 at 11:57:33PM -0500, Jeff King wrote:

> Subject: [PATCH] credential-cache: report more daemon connection errors
> 
> Originally, this code remained relatively silent when we
> failed to connect to the cache. The idea was that it was
> simply a cache, and we didn't want to bother the user with
> temporary failures (the worst case is that we would simply
> ask their password again).
> 
> However, if you have a configuration failure or other
> problem, it is helpful for the daemon to report those
> problems. Git will happily ignore the failed error code, but
> the extra information to stderr can help the user diagnose
> the problem.

This actually has a minor regression, fixed below.

-- >8 --
Subject: [PATCH] credential-cache: ignore "connection refused" errors

The credential-cache helper will try to connect to its
daemon over a unix socket. Originally, a failure to do so
was silently ignored, and we would either give up (if
performing a "get" or "erase" operation), or spawn a new
daemon (for a "store" operation).

But since 8ec6c8d, we try to report more errors. We detect a
missing daemon by checking for ENOENT on our connection
attempt.  If the daemon is missing, we continue as before
(giving up or spawning a new daemon). For any other error,
we die and report the problem.

However, checking for ENOENT is not sufficient for a missing
daemon. We might also get ECONNREFUSED if a dead daemon
process left a stale socket. This generally shouldn't
happen, as the daemon cleans up after itself, but the daemon
may not always be given a chance to do so (e.g., power loss,
"kill -9").

The resulting state is annoying not just because the helper
outputs an extra useless message, but because it actually
blocks the helper from spawning a new daemon to replace the
stale socket.

Fix it by checking for ECONNREFUSED.

Signed-off-by: Jeff King <peff@peff.net>
---
If we really want to go belt-and-suspenders, the logic should perhaps be
changed to:

  if (send_request(socket, &buf < 0) {
          /* if we're starting a new one, who cares why it didn't work */
          if (flags & FLAG_SPAWN) {
                  spawn_daemon(socket);
                  if (send_request(socket, &buf) < 0)
                          die_errno("unable to connect to spawned daemon");
          }
          /* otherwise, report any non-minor errors */
          else if(errno != ENOENT && errno != ECONNREFUSED)
                  die_errno("unable to connect to cache daemon");
          /* otherwise we are just missing the daemon, and we can ignore */
  }

but that implies there is some condition besides ENOENT and ECONNREFUSED
where actually starting a new daemon (which will try to unlink whatever
is there now!) would be a good idea. I'd rather be conservative and
see if anybody reports a real-world case.

 credential-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/credential-cache.c b/credential-cache.c
index 1933018..9a03792 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -72,7 +72,7 @@ static void do_cache(const char *socket, const char *action, int timeout,
 	}
 
 	if (send_request(socket, &buf) < 0) {
-		if (errno != ENOENT)
+		if (errno != ENOENT && errno != ECONNREFUSED)
 			die_errno("unable to connect to cache daemon");
 		if (flags & FLAG_SPAWN) {
 			spawn_daemon(socket);
-- 
1.7.9.rc0.33.gd3c17