On Tue, Sep 14, 2021 at 12:09:18PM -0700, Junio C Hamano wrote:
> 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.
> 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
