Re: [PATCH v2 2/3] credential-cache: check for windows specific errors

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org, bagasdotme@gmail.com, gitster@pobox.com
Subject: Re: [PATCH v2 2/3] credential-cache: check for windows specific errors
Date: Mon, 13 Sep 2021 13:58:55 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2109131357280.55@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20210913085600.35506-3-carenas@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

Hi Carlo,

On Mon, 13 Sep 2021, Carlo Marcelo Arenas Belón wrote:

> Connect and reset errors aren't what will be expected by POSIX but
> are compatible with the ones used by WinSock.
>
> To avoid any possibility of confusion with other systems checks
> for disconnection and availability had been abstracted into helper
> functions that are platform specific.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> V2:
> * Use helper functions to separate error handling as suggested by Junio
>
>  builtin/credential-cache.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
> index e8a7415747..fd9f33d993 100644
> --- a/builtin/credential-cache.c
> +++ b/builtin/credential-cache.c
> @@ -11,6 +11,32 @@
>  #define FLAG_SPAWN 0x1
>  #define FLAG_RELAY 0x2
>
> +#ifdef _WIN32

While that works, I think we prefer `WIN32` (`_WIN32` is only used in
`compat/` and `contrib/`).

Other than that, looks good!

Ciao,
Dscho

> +
> +static int connection_closed(int error)
> +{
> +	return (error == EINVAL);
> +}
> +
> +static int connection_fatally_broken(int error)
> +{
> +	return (error != ENOENT) && (error != ENETDOWN);
> +}
> +
> +#else
> +
> +static int connection_closed(int error)
> +{
> +	return (error == ECONNRESET);
> +}
> +
> +static int connection_fatally_broken(int error)
> +{
> +	return (error != ENOENT) && (error != ECONNREFUSED);
> +}
> +
> +#endif
> +
>  static int send_request(const char *socket, const struct strbuf *out)
>  {
>  	int got_data = 0;
> @@ -28,7 +54,7 @@ static int send_request(const char *socket, const struct strbuf *out)
>  		int r;
>
>  		r = read_in_full(fd, in, sizeof(in));
> -		if (r == 0 || (r < 0 && errno == ECONNRESET))
> +		if (r == 0 || (r < 0 && connection_closed(errno)))
>  			break;
>  		if (r < 0)
>  			die_errno("read error from cache daemon");
> @@ -75,7 +101,7 @@ static void do_cache(const char *socket, const char *action, int timeout,
>  	}
>
>  	if (send_request(socket, &buf) < 0) {
> -		if (errno != ENOENT && errno != ECONNREFUSED)
> +		if (connection_fatally_broken(errno))
>  			die_errno("unable to connect to cache daemon");
>  		if (flags & FLAG_SPAWN) {
>  			spawn_daemon(socket);
> --
> 2.33.0.481.g26d3bed244
>
>