[ Jeff: sorry for not CC:-ing you on the original email - I had intended to do just that, but forgot! :( See: https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@xxxxxxxxxxxxxxxxxxxx/ ] On 07/07/2022 07:15, Junio C Hamano wrote: > Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> writes: > >> However, I had some time to kill tonight, so I decided to take a _quick_ look >> to see if there was something that could be done ... (famous last words). >> ... >> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c >> index 78c02ad531..84fd513c62 100644 >> --- a/builtin/credential-cache.c >> +++ b/builtin/credential-cache.c >> @@ -27,7 +27,7 @@ static int connection_fatally_broken(int error) >> >> static int connection_closed(int error) >> { >> - return (error == ECONNRESET); >> + return (error == ECONNRESET) || (error == ECONNABORTED); >> } > > This feels like papering over the problem. Agreed, ... which is what I really meant by "(Well, it side-steps the problem, really)." >> Having noticed that the 'timeout' test was not failing, I decided to try >> making the 'action=exit' code-path behave more like the timeout code, as >> far as exiting the server is concerned. Indeed, you might ask why the >> timeout code doesn't just 'exit(0)' as well ... >> >> Anyway, the following patch does that, and it also provides a 'fix' for this >> issue! > > If this codepath was written like this (i.e. [PATCH 1C]) from the > beginning, I would have found it very sensible (i.e. instead of > caling exit() in the middle of the infinite client serving loop, > exiting the loop cleanly is easier to follow and maintain), even if > we didn't know the issue on Cygwin you investigated. Yep, apart from the variable name, I quite like the approach taken by the 1C patch. All three of these patches were really just "showing my working" and allowing anyone to "follow along" without the hassle of trying to scrape the diffs from the email. As I said, I don't think we can determine a suitable fix without first finding the cygwin commit which caused this test failure. But if we can't determine this, for whatever reason, then I would favour a patch to git based on the 1C patch. (Writing the commit message to justify the change, without mentioning this cygwin issue, may be more challenging! :) Also, I would like to understand why the code is written as it is currently. I'm sure there must be a good reason - I just don't know what it is! I suspect (ie I'm guessing), it has something to do with operating in a high contention context [TOCTOU on socket?] ... dunno. ;-) ATB, Ramsay Jones
