| Message ID | xmqqilz30yap.fsf@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | What should happen in credential-cache on recoverable error without SPAWN option? | expand |
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
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); }
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? This in turn came from 98c2924c (credentials: unable to connect to cache daemon, 2012-01-07), before it, the code read like this: if (!send_request(socket, &buf)) return; if (flags & FLAG_SPAWN) { spawn_daemon(socket); send_request(socket, &buf); } so it looks to me that I am puzzled by an incomplete error handling introduced by 98c2924c, and if we wanted to complete it, it may perhaps look like this patch on top of Carlo's patch? Or am I barking up a wrong tree and error checking in this command does not really make a real-life difference? Thanks. builtin/credential-cache.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)