Re: What should happen in credential-cache on recoverable error without SPAWN option?

From: Jeff King <peff@peff.net>
Date: Sep 14, 2021, 19:33 UTC
Message-ID: <YUD5I/hwxpcyocEU@coredump.intra.peff.net>
In-Reply-To: <xmqqilz30yap.fsf@gitster.g>

On Tue, Sep 14, 2021 at 12:09:18PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> While reviewing Carlo's "credential-cache: check for windows
> specific errors", I noticed this piece of code, that came from
> 8ec6c8d7 (credential-cache: report more daemon connection errors,
> 2012-01-09):
> 
> 	if (send_request(socket, &buf) < 0) {
> 		if (errno != ENOENT && errno != ECONNREFUSED)
> 			die_errno("unable to connect to cache daemon");
> 		if (flags & FLAG_SPAWN) {
> 			spawn_daemon(socket);
> 			if (send_request(socket, &buf) < 0)
> 				die_errno("unable to connect to cache daemon");
> 		}
> 	}
> 
> What would happen when we get a resumable error and then weren't
> given the SPAWN flag?  It seems that do_cache() simply returns
> without dying.  Shouldn't we get "unable to connect" in such a case?

It's subtle, but I think we end up doing the right thing. Those errors aren't just "resumable"; they are "we do not have a daemon to talk to at all".

The "exit", "get", and "erase" operations do not pass the SPAWN flag. But if there is no daemon, they are all noops.

The "store" operation is the only one which uses SPAWN, and of course there we want to spin up a daemon so that we can put something in it.

It may be that SPAWN could have a better name to make this more clear.

Show 22 quoted lines

> diff --git c/builtin/credential-cache.c w/builtin/credential-cache.c
> index 78c02ad531..a41a17e58f 100644
> --- c/builtin/credential-cache.c
> +++ w/builtin/credential-cache.c
> @@ -101,13 +101,11 @@ static void do_cache(const char *socket, const char *action, int timeout,
>  	}
>  
>  	if (send_request(socket, &buf) < 0) {
> -		if (connection_fatally_broken(errno))
> +		if (connection_fatally_broken(errno) && !(flag & FLAG_SPAWN))
> +			die_errno("unable to connect to cache daemon");
> +		spawn_daemon(socket);
> +		if (send_request(socket, &buf) < 0)
>  			die_errno("unable to connect to cache daemon");
> -		if (flags & FLAG_SPAWN) {
> -			spawn_daemon(socket);
> -			if (send_request(socket, &buf) < 0)
> -				die_errno("unable to connect to cache daemon");
> -		}
>  	}
>  	strbuf_release(&buf);
>  }

If you do this, then I think we'll start producing spurious errors during normal use of the helper. Most interaction will generally start with a "get" request to the helpers. So if you don't have anything cached and the daemon stopped running, right now we just don't return a credential. With this we'd complain "unable to connect to daemon".

And then of course we'd follow that up by asking for the credential and spinning up a daemon to store it. But that first request after the daemon times out will always say "unable to connect".

-Peff