[PATCH 0/3] windows: allow building without NO_UNIX_SOCKETS

[PATCH 0/3] windows: allow building without NO_UNIX_SOCKETS

[Carlo Marcelo Arenas Belón]

Eventhough NO_UNIX_SOCKETS was specifically added to support Windows,
it might not be necessary in the future, because Windows added support
for Unix Sockets in late 2017.

The first patch is messy, specially considering how little was needed
to build and run `git credential-cache` and its daemon, nothing broke
on the trace2 code or tests, but additional testing to make sure will
be recommended.

Carlo Marcelo Arenas Belón (3):
  t0301: fixes for windows compatibility
  credential-cache: check for windows specific errors
  git-compat-util: include declaration for unix sockets

 builtin/credential-cache.c  |  5 +++--
 git-compat-util.h           |  3 +++
 t/t0301-credential-cache.sh | 28 ++++++++++++++++++++--------
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.33.0.481.g26d3bed244

[PATCH 1/3] t0301: fixes for windows compatibility

[Carlo Marcelo Arenas Belón]

In preparation for a future patch that will allow building with
Unix Sockets in Windows, workaround a couple of issues from the
Mingw-W64 compatibility layer.

test -S is not able to detect that a file is a socket, so use
test -f instead.

`mkdir -m` will always fail with permission problems, so instead
call mkdir followed by chmod.

The last invocation of mkdir would likely need the same treatment
but SYMLINK is unlikely to be enabled on Windows so it has been
punted for now.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0301-credential-cache.sh | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index ebd5fa5249..f5cce6c6a6 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -9,6 +9,17 @@ test -z "$NO_UNIX_SOCKETS" || {
 	test_done
 }
 
+# in Windows, Unix Sockets look just like regular files
+uname_s=$(uname -s)
+case $uname_s in
+*MINGW*)
+	FLAG=-f
+	;;
+*)
+	FLAG=-S
+	;;
+esac
+
 # don't leave a stale daemon running
 test_atexit 'git credential-cache exit'
 
@@ -21,7 +32,7 @@ test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
 		rmdir -p .cache/git/credential/
 	" &&
 	test_path_is_missing "$HOME/.git-credential-cache" &&
-	test -S "$HOME/.cache/git/credential/socket"
+	test $FLAG "$HOME/.cache/git/credential/socket"
 '
 
 XDG_CACHE_HOME="$HOME/xdg"
@@ -31,7 +42,7 @@ helper_test cache
 
 test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
 	test_when_finished "git credential-cache exit" &&
-	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
+	test $FLAG "$XDG_CACHE_HOME/git/credential/socket" &&
 	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
 	test_path_is_missing "$HOME/.cache/git/credential/socket"
 '
@@ -48,7 +59,7 @@ test_expect_success 'credential-cache --socket option overrides default location
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/dir/socket"
+	test $FLAG "$HOME/dir/socket"
 '
 
 test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
@@ -62,7 +73,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.cache/git/credential/socket" &&
+	test $FLAG "$HOME/.cache/git/credential/socket" &&
 	XDG_CACHE_HOME="$HOME/xdg" &&
 	export XDG_CACHE_HOME &&
 	check approve cache <<-\EOF &&
@@ -71,7 +82,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$XDG_CACHE_HOME/git/credential/socket"
+	test $FLAG "$XDG_CACHE_HOME/git/credential/socket"
 '
 
 test_expect_success 'use user socket if user directory exists' '
@@ -79,14 +90,15 @@ test_expect_success 'use user socket if user directory exists' '
 		git credential-cache exit &&
 		rmdir \"\$HOME/.git-credential-cache/\"
 	" &&
-	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
+	mkdir -p "$HOME/.git-credential-cache/" &&
+	chmod 700 "$HOME/.git-credential-cache/" &&
 	check approve cache <<-\EOF &&
 	protocol=https
 	host=example.com
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.git-credential-cache/socket"
+	test $FLAG "$HOME/.git-credential-cache/socket"
 '
 
 test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
@@ -103,7 +115,7 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.git-credential-cache/socket"
+	test $FLAG "$HOME/.git-credential-cache/socket"
 '
 
 helper_test_timeout cache --timeout=1
-- 
2.33.0.481.g26d3bed244

Re: [PATCH 1/3] t0301: fixes for windows compatibility

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> In preparation for a future patch that will allow building with
> Unix Sockets in Windows, workaround a couple of issues from the
> Mingw-W64 compatibility layer.
>
> test -S is not able to detect that a file is a socket, so use
> test -f instead.

The cause deserves to be sympathized, but ...

> +# in Windows, Unix Sockets look just like regular files
> +uname_s=$(uname -s)
> +case $uname_s in
> +*MINGW*)
> +	FLAG=-f
> +	;;
> +*)
> +	FLAG=-S
> +	;;
> +esac
> +
>  # don't leave a stale daemon running
>  test_atexit 'git credential-cache exit'
>  
> @@ -21,7 +32,7 @@ test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
>  		rmdir -p .cache/git/credential/
>  	" &&
>  	test_path_is_missing "$HOME/.git-credential-cache" &&
> -	test -S "$HOME/.cache/git/credential/socket"
> +	test $FLAG "$HOME/.cache/git/credential/socket"

This is horrible.  Unlike the original, it is now impossible for a
casual reader to tell what this thing is testing, because $FLAG is
so generic a name that does not convey anything to readers.

Introduce a helper, say, "test_socket_exists", and replace "test -S"
with it.  In the implementation of that test_socket_exists() helper,
you can hide the difference between -f and -S.  In such a small scope,
you can even call the variable $FLAG if you want, as it is so isolated.

> -	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
> +	mkdir -p "$HOME/.git-credential-cache/" &&
> +	chmod 700 "$HOME/.git-credential-cache/" &&

OK.  This is the only use of "mkdir -m MODE" in the test suite.  It
is strange that you are OK with "mkdir && chmod 700" but not OK with
"mkdir -m 700" (it just is illogical, but we have to live with it,
as the real world may not be logical after all).

It is somewhat strange that we insist on mode 700 but the test does
not have SANITY as its prerequisite.  Does it really matter if we
set the permission bits to close the directory from others?  If not,
our "portability clean-up" could just be to lose "-m 700" here.

Thanks.

>  	check approve cache <<-\EOF &&
>  	protocol=https
>  	host=example.com
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$HOME/.git-credential-cache/socket"
> +	test $FLAG "$HOME/.git-credential-cache/socket"
>  '
>  
>  test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
> @@ -103,7 +115,7 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$HOME/.git-credential-cache/socket"
> +	test $FLAG "$HOME/.git-credential-cache/socket"
>  '
>  
>  helper_test_timeout cache --timeout=1

Re: [PATCH 1/3] t0301: fixes for windows compatibility

[Bagas Sanjaya]

On 13/09/21 03.28, Carlo Marcelo Arenas Belón wrote:
> test -S is not able to detect that a file is a socket, so use
> test -f instead.
> 

Isn't test -f just check for socket as regular file?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH 1/3] t0301: fixes for windows compatibility

[Carlo Arenas]

On Sun, Sep 12, 2021 at 10:34 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> On 13/09/21 03.28, Carlo Marcelo Arenas Belón wrote:
> > test -S is not able to detect that a file is a socket, so use
> > test -f instead.
>
> Isn't test -f just check for socket as regular file?

and that is exactly how they look; ironically a -f check in Linux
fails for sockets so maybe better to do -e?

an empty file with nothing that indicates in Windows Explorer or a
stat call (from WSL or git bash), that they are anything else.

Carlo

Re: [PATCH 1/3] t0301: fixes for windows compatibility

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Sun, Sep 12, 2021 at 10:34 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>> On 13/09/21 03.28, Carlo Marcelo Arenas Belón wrote:
>> > test -S is not able to detect that a file is a socket, so use
>> > test -f instead.
>>
>> Isn't test -f just check for socket as regular file?
>
> and that is exactly how they look; ironically a -f check in Linux
> fails for sockets so maybe better to do -e?
>
> an empty file with nothing that indicates in Windows Explorer or a
> stat call (from WSL or git bash), that they are anything else.

It actually is a quite attractive idea to use "-e", or even more
preferrably, test_path_exists.  For example:

@@ -31,7 +42,7 @@ helper_test cache
 
 test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
 	test_when_finished "git credential-cache exit" &&
-	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
+	test $FLAG "$XDG_CACHE_HOME/git/credential/socket" &&
 	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
 	test_path_is_missing "$HOME/.cache/git/credential/socket"
 '

test_path_exists contrasts better with the two test_path_is_missing
and explains what is being tested better.  Before this part, we have
run some "git credential" test, and there are three possible places
that the socket may appear (XDG, HOME/.git-credential-cache/ and
HOME/.cache/git/credential/), and we want to make sure only one of
them gets it.

One possible downside is that it makes us rely more on our knowledge
that we communicate via unix-domain socket (i.e. what the "socket"
the test is checking is).  By assuming that a mere presence of some
filesystem entity at the inspected path is OK, we may not notice a
breakage that creates a regular file or a directory there by mistake,
yet successfully carry out the credential tests.  It may even be a
good thing, if future ourselves have somehow found out how to use a
regular file or a directory for IPC instead of using a socket ;-).

Thanks.

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

[Carlo Marcelo Arenas Belón]

Connect and reset errors aren't what will be expected by POSIX but
are compatible with the ones used by WinSock.

Alternatively a translation layer for read could be added (similar
to the one provided by mingw-write()) to translate the odd EINVAL,
into a more reasonable EPIPE, but this is more localized and still
unlikely to cause confusion in other systems.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/credential-cache.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index 76a6ba3722..b12a79ae01 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -28,7 +28,8 @@ 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 && (errno == ECONNRESET || errno == EINVAL)))
 			break;
 		if (r < 0)
 			die_errno("read error from cache daemon");
@@ -75,7 +76,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 (errno != ENOENT && errno != ECONNREFUSED && errno != ENETDOWN)
 			die_errno("unable to connect to cache daemon");
 		if (flags & FLAG_SPAWN) {
 			spawn_daemon(socket);
-- 
2.33.0.481.g26d3bed244

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

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> Connect and reset errors aren't what will be expected by POSIX but
> are compatible with the ones used by WinSock.
>
> Alternatively a translation layer for read could be added (similar
> to the one provided by mingw-write()) to translate the odd EINVAL,
> into a more reasonable EPIPE, but this is more localized and still
> unlikely to cause confusion in other systems.

Are we sure EINVAL or ENETDOWN from outside the Windows world is
tolerable after read_in_full() failure in this context?

I'd rather see something like

	if (!r || (r < 0 && connection_closed(errno)))
		break;

with a local helper function connection_closed() allowing EINVAL
only with "#ifdef WINDOWS" to be safe.  The same for "the socket is
not yet open" codepath with another local helper function.

Thanks.

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/credential-cache.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
> index 76a6ba3722..b12a79ae01 100644
> --- a/builtin/credential-cache.c
> +++ b/builtin/credential-cache.c
> @@ -28,7 +28,8 @@ 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 && (errno == ECONNRESET || errno == EINVAL)))
>  			break;
>  		if (r < 0)
>  			die_errno("read error from cache daemon");
> @@ -75,7 +76,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 (errno != ENOENT && errno != ECONNREFUSED && errno != ENETDOWN)
>  			die_errno("unable to connect to cache daemon");
>  		if (flags & FLAG_SPAWN) {
>  			spawn_daemon(socket);

[PATCH 3/3] git-compat-util: include declaration for unix sockets

[Carlo Marcelo Arenas Belón]

Available since Windows 10 release 1803, therefore only added if
not using NO_UNIX_SOCKETS (which is not the current default).

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 git-compat-util.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index b46605300a..6a420d104c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -160,6 +160,9 @@
 # endif
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
 #include <winsock2.h>
+#ifndef NO_UNIX_SOCKETS
+#include <afunix.h>
+#endif
 #include <windows.h>
 #define GIT_WINDOWS_NATIVE
 #endif
-- 
2.33.0.481.g26d3bed244

[PATCH v2 0/3] windows: allow building without NO_UNIX_SOCKETS

[Carlo Marcelo Arenas Belón]

Eventhough NO_UNIX_SOCKETS was specifically added to support Windows,
it might not be necessary in the future, because Windows added support
for Unix Sockets in late 2017.

The implementation of Unix Sockets, uses an internal NTFS mechanism
and is therefore not visible at the filesystem layer, like it is in
UNIX, but seems to be good enough to allow to build and run the
`git credential-cache` and its daemon; additional testing to confirm
trace2 (builds and doesn't fail any tests) is functional will be
needed.

V2 reuses the same third patch from V1, and applies all suggested
feedback on patches 1 and 2 and should apply cleanly as a reroll of
cb/unix-sockets-with-windows.

Carlo Marcelo Arenas Belón (3):
  t0301: fixes for windows compatibility
  credential-cache: check for windows specific errors
  git-compat-util: include declaration for unix sockets

 builtin/credential-cache.c  | 30 ++++++++++++++++++++++++++++--
 git-compat-util.h           |  3 +++
 t/t0301-credential-cache.sh | 32 ++++++++++++++++++++++++--------
 3 files changed, 55 insertions(+), 10 deletions(-)

-- 
2.33.0.481.g26d3bed244

[PATCH v2 1/3] t0301: fixes for windows compatibility

[Carlo Marcelo Arenas Belón]

In preparation for a future patch that will allow building with
Unix Sockets in Windows, workaround a couple of issues from the
Mingw-W64 compatibility layer.

test -S is not able to detect that a file is a socket, so use
test -e instead (through a library function).

`mkdir -m` will always fail with permission problems, so instead
call mkdir followed by chmod.

The last invocation of mkdir would likely need the same treatment
but SYMLINK is unlikely to be enabled on Windows so it has been
punted for now.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2:
* avoid the confusing -f test as suggested by Bagas
* try to help casual readers as suggested by Junio

 t/t0301-credential-cache.sh | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index ebd5fa5249..1f7b1e29e6 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -9,6 +9,21 @@ test -z "$NO_UNIX_SOCKETS" || {
 	test_done
 }
 
+test_path_is_socket () {
+	test -S "$1"
+}
+
+# in Windows, Unix Sockets look just like regular files
+uname_s=$(uname -s)
+case $uname_s in
+*MINGW*)
+	test_socket_exist=test_path_exists
+	;;
+*)
+	test_socket_exist=test_path_is_socket
+	;;
+esac
+
 # don't leave a stale daemon running
 test_atexit 'git credential-cache exit'
 
@@ -21,7 +36,7 @@ test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
 		rmdir -p .cache/git/credential/
 	" &&
 	test_path_is_missing "$HOME/.git-credential-cache" &&
-	test -S "$HOME/.cache/git/credential/socket"
+	$test_socket_exist "$HOME/.cache/git/credential/socket"
 '
 
 XDG_CACHE_HOME="$HOME/xdg"
@@ -31,7 +46,7 @@ helper_test cache
 
 test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
 	test_when_finished "git credential-cache exit" &&
-	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
+	$test_socket_exist "$XDG_CACHE_HOME/git/credential/socket" &&
 	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
 	test_path_is_missing "$HOME/.cache/git/credential/socket"
 '
@@ -48,7 +63,7 @@ test_expect_success 'credential-cache --socket option overrides default location
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/dir/socket"
+	$test_socket_exist "$HOME/dir/socket"
 '
 
 test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
@@ -62,7 +77,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.cache/git/credential/socket" &&
+	$test_socket_exist "$HOME/.cache/git/credential/socket" &&
 	XDG_CACHE_HOME="$HOME/xdg" &&
 	export XDG_CACHE_HOME &&
 	check approve cache <<-\EOF &&
@@ -71,7 +86,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$XDG_CACHE_HOME/git/credential/socket"
+	$test_socket_exist "$XDG_CACHE_HOME/git/credential/socket"
 '
 
 test_expect_success 'use user socket if user directory exists' '
@@ -79,14 +94,15 @@ test_expect_success 'use user socket if user directory exists' '
 		git credential-cache exit &&
 		rmdir \"\$HOME/.git-credential-cache/\"
 	" &&
-	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
+	mkdir -p "$HOME/.git-credential-cache/" &&
+	chmod 700 "$HOME/.git-credential-cache/" &&
 	check approve cache <<-\EOF &&
 	protocol=https
 	host=example.com
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.git-credential-cache/socket"
+	$test_socket_exist "$HOME/.git-credential-cache/socket"
 '
 
 test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
@@ -103,7 +119,7 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.git-credential-cache/socket"
+	$test_socket_exist "$HOME/.git-credential-cache/socket"
 '
 
 helper_test_timeout cache --timeout=1
-- 
2.33.0.481.g26d3bed244

Re: [PATCH v2 1/3] t0301: fixes for windows compatibility

[Johannes Schindelin]

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

Hi Carlo,

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

> In preparation for a future patch that will allow building with
> Unix Sockets in Windows, workaround a couple of issues from the
> Mingw-W64 compatibility layer.
>
> test -S is not able to detect that a file is a socket, so use

Is that really true, even with recent MSYS2 versions? I thought I saw some
patch flying around on the Cygwin mailing list that added support for Unix
sockets...

> test -e instead (through a library function).
>
> `mkdir -m` will always fail with permission problems, so instead
> call mkdir followed by chmod.

Maybe explain that Windows' permission model is a lot more sophisticated
(using Access Control Lists) and is therefore unable to interpret
permissions like `0700`.

And we probably need to mention then, too, that it is funny that `mkdir
-m` complains while `chmod` does _not_... Ah, historical reasons...

Thanks,
Dscho

>
> The last invocation of mkdir would likely need the same treatment
> but SYMLINK is unlikely to be enabled on Windows so it has been
> punted for now.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> v2:
> * avoid the confusing -f test as suggested by Bagas
> * try to help casual readers as suggested by Junio
>
>  t/t0301-credential-cache.sh | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
> index ebd5fa5249..1f7b1e29e6 100755
> --- a/t/t0301-credential-cache.sh
> +++ b/t/t0301-credential-cache.sh
> @@ -9,6 +9,21 @@ test -z "$NO_UNIX_SOCKETS" || {
>  	test_done
>  }
>
> +test_path_is_socket () {
> +	test -S "$1"
> +}
> +
> +# in Windows, Unix Sockets look just like regular files
> +uname_s=$(uname -s)
> +case $uname_s in
> +*MINGW*)
> +	test_socket_exist=test_path_exists
> +	;;
> +*)
> +	test_socket_exist=test_path_is_socket
> +	;;
> +esac

A more canonical way would probably be to imitate what we do with `pwd` in
`t/test-lib.sh`:

	test_path_is_socket () {
		test -S "$1"
	}

	case $uname_s in
	*MINGW*)
		test_path_is_socket () {
			# `test -S` cannot detect Win10's Unix sockets
			test -e "$1"
		}
		;;
	esac

> +
>  # don't leave a stale daemon running
>  test_atexit 'git credential-cache exit'
>
> @@ -21,7 +36,7 @@ test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
>  		rmdir -p .cache/git/credential/
>  	" &&
>  	test_path_is_missing "$HOME/.git-credential-cache" &&
> -	test -S "$HOME/.cache/git/credential/socket"
> +	$test_socket_exist "$HOME/.cache/git/credential/socket"
>  '
>
>  XDG_CACHE_HOME="$HOME/xdg"
> @@ -31,7 +46,7 @@ helper_test cache
>
>  test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
>  	test_when_finished "git credential-cache exit" &&
> -	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
> +	$test_socket_exist "$XDG_CACHE_HOME/git/credential/socket" &&
>  	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
>  	test_path_is_missing "$HOME/.cache/git/credential/socket"
>  '
> @@ -48,7 +63,7 @@ test_expect_success 'credential-cache --socket option overrides default location
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$HOME/dir/socket"
> +	$test_socket_exist "$HOME/dir/socket"
>  '
>
>  test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
> @@ -62,7 +77,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$HOME/.cache/git/credential/socket" &&
> +	$test_socket_exist "$HOME/.cache/git/credential/socket" &&
>  	XDG_CACHE_HOME="$HOME/xdg" &&
>  	export XDG_CACHE_HOME &&
>  	check approve cache <<-\EOF &&
> @@ -71,7 +86,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$XDG_CACHE_HOME/git/credential/socket"
> +	$test_socket_exist "$XDG_CACHE_HOME/git/credential/socket"
>  '
>
>  test_expect_success 'use user socket if user directory exists' '
> @@ -79,14 +94,15 @@ test_expect_success 'use user socket if user directory exists' '
>  		git credential-cache exit &&
>  		rmdir \"\$HOME/.git-credential-cache/\"
>  	" &&
> -	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
> +	mkdir -p "$HOME/.git-credential-cache/" &&
> +	chmod 700 "$HOME/.git-credential-cache/" &&
>  	check approve cache <<-\EOF &&
>  	protocol=https
>  	host=example.com
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$HOME/.git-credential-cache/socket"
> +	$test_socket_exist "$HOME/.git-credential-cache/socket"
>  '
>
>  test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
> @@ -103,7 +119,7 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$HOME/.git-credential-cache/socket"
> +	$test_socket_exist "$HOME/.git-credential-cache/socket"
>  '
>
>  helper_test_timeout cache --timeout=1
> --
> 2.33.0.481.g26d3bed244
>
>

Re: [PATCH v2 1/3] t0301: fixes for windows compatibility

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +test_path_is_socket () {
>> +	test -S "$1"
>> +}
>> +
>> +# in Windows, Unix Sockets look just like regular files
>> +uname_s=$(uname -s)
>> +case $uname_s in
>> +*MINGW*)
>> +	test_socket_exist=test_path_exists
>> +	;;
>> +*)
>> +	test_socket_exist=test_path_is_socket
>> +	;;
>> +esac
>
> A more canonical way would probably be to imitate what we do with `pwd` in
> `t/test-lib.sh`:

Thanks for bringing up a better practice.

Referring to a variable when calling a function gives a "we are
doing something unusual" signal and it loses half its abstraction
value at the callsites.  E.g.

>>  	test_when_finished "git credential-cache exit" &&
>> -	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
>> +	$test_socket_exist "$XDG_CACHE_HOME/git/credential/socket" &&
>>  	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
>>  	test_path_is_missing "$HOME/.cache/git/credential/socket"

I actually do not think it is so bad to just use test_path_exists
without per-platform conditional in this case, but if we want to be
more conservative, I agree with you that

	case ... in
	*MINGW*)
                test_path_is_socket () {
                        test_path_exists "$@"
                }
        	;;
	*)
                test_path_is_socket () {
                        test -S "$1"
                }
		;;
	esac

is the way to go.

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

[Carlo Marcelo Arenas Belón]

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
+
+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

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

[Johannes Schindelin]

[-- 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
>
>

[PATCH v2 3/3] git-compat-util: include declaration for unix sockets

[Carlo Marcelo Arenas Belón]

Available since Windows 10 release 1803, therefore only added if
not using NO_UNIX_SOCKETS (which is not the current default).

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 git-compat-util.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index b46605300a..6a420d104c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -160,6 +160,9 @@
 # endif
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
 #include <winsock2.h>
+#ifndef NO_UNIX_SOCKETS
+#include <afunix.h>
+#endif
 #include <windows.h>
 #define GIT_WINDOWS_NATIVE
 #endif
-- 
2.33.0.481.g26d3bed244

Re: [PATCH v2 3/3] git-compat-util: include declaration for unix sockets

[Johannes Schindelin]

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

Hi Carlo,

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

> Available since Windows 10 release 1803, therefore only added if
> not using NO_UNIX_SOCKETS (which is not the current default).

I wouldn't mind one bit if we did not have a double negation ;-)

Other than that, looks good!

Ciao,
Dscho

>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  git-compat-util.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index b46605300a..6a420d104c 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -160,6 +160,9 @@
>  # endif
>  #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
>  #include <winsock2.h>
> +#ifndef NO_UNIX_SOCKETS
> +#include <afunix.h>
> +#endif
>  #include <windows.h>
>  #define GIT_WINDOWS_NATIVE
>  #endif
> --
> 2.33.0.481.g26d3bed244
>
>

Re: [PATCH v2 0/3] windows: allow building without NO_UNIX_SOCKETS

[Johannes Schindelin]

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

Hi Carlo,

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

> Eventhough NO_UNIX_SOCKETS was specifically added to support Windows,
> it might not be necessary in the future, because Windows added support
> for Unix Sockets in late 2017.

The fact that some recent Windows 10 versions support Unix sockets is
nice, no doubt. As far as Git for Windows is concerned (which does not
recompile itself during an installation based on the Windows version it
found), we cannot use it, of course. At least not until all of the
previous Windows versions are _long_ out of service. For example, we still
support Windows 7 even if it reached it's "End-Of-Life".

Having said that: thank you for working on this. For users who build their
Git themselves (which is definitely something Git for Windows makes
easy), the option to use Unix sockets is really nice.

Ciao,
Dscho

> The implementation of Unix Sockets, uses an internal NTFS mechanism
> and is therefore not visible at the filesystem layer, like it is in
> UNIX, but seems to be good enough to allow to build and run the
> `git credential-cache` and its daemon; additional testing to confirm
> trace2 (builds and doesn't fail any tests) is functional will be
> needed.
>
> V2 reuses the same third patch from V1, and applies all suggested
> feedback on patches 1 and 2 and should apply cleanly as a reroll of
> cb/unix-sockets-with-windows.
>
> Carlo Marcelo Arenas Belón (3):
>   t0301: fixes for windows compatibility
>   credential-cache: check for windows specific errors
>   git-compat-util: include declaration for unix sockets
>
>  builtin/credential-cache.c  | 30 ++++++++++++++++++++++++++++--
>  git-compat-util.h           |  3 +++
>  t/t0301-credential-cache.sh | 32 ++++++++++++++++++++++++--------
>  3 files changed, 55 insertions(+), 10 deletions(-)
>
> --
> 2.33.0.481.g26d3bed244
>
>

[PATCH v3 0/3] windows: allow building without NO_UNIX_SOCKETS

[Carlo Marcelo Arenas Belón]

Eventhough NO_UNIX_SOCKETS was specifically added to support Windows,
it might not be necessary in the future, because Windows added support
for Unix Sockets in late 2017.

The implementation of Unix Sockets, uses an internal NTFS mechanism
and is therefore not visible at the filesystem layer, like it is in
UNIX, but seems to be good enough to allow to build and run the
`git credential-cache` and its daemon, with not many changes.

Additional testing to confirm trace2 (which builds and doesn't fail any
tests) is functional will be also needed.

Interestingly, Cygwin has its own implementation of Unix Sockets that
are visible as such even in the Git Bash environment, but that are also
incompatible with the native ones.

V3 Changes the commit messages as suggested and fixes the original ugly
implementation of the workarounds for the first patch and shoule apply
cleanly as a reroll of cb/unix-sockets-with-windows.

Carlo Marcelo Arenas Belón (3):
  t0301: fixes for windows compatibility
  credential-cache: check for windows specific errors
  git-compat-util: include declaration for unix sockets in windows

 builtin/credential-cache.c  | 30 ++++++++++++++++++++++++++++--
 git-compat-util.h           |  3 +++
 t/t0301-credential-cache.sh | 32 ++++++++++++++++++++++++--------
 3 files changed, 55 insertions(+), 10 deletions(-)

-- 
2.33.0.481.g26d3bed244

[PATCH v3 1/3] t0301: fixes for windows compatibility

[Carlo Marcelo Arenas Belón]

In preparation for a future patch that will allow building with
Unix Sockets in Windows, workaround a couple of issues from the
Mingw-W64 compatibility layer.

test -S is not able to detect that a file is a socket, so use
test -e instead (through a library function).

`mkdir -m` can't represent a valid ACL directly and fails with
permission problems, so instead call mkdir followed by chmod, which
has been enhanced to do so.

The last invocation of mkdir would likely need the same treatment
but SYMLINK is unlikely to be enabled on Windows so it has been
punted for now.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v3:
* avoid using a variable to hold the function for testing and instead
  use the cleaner syntax suggested by Dscho
v2:
* avoid the confusing -f test as suggested by Bagas
* no more FLAG variable as suggested by Junio

 t/t0301-credential-cache.sh | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index ebd5fa5249..698b7159f0 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -9,6 +9,21 @@ test -z "$NO_UNIX_SOCKETS" || {
 	test_done
 }
 
+uname_s=$(uname -s)
+case $uname_s in
+*MINGW*)
+	test_path_is_socket () {
+		# `test -S` cannot detect Win10's Unix sockets
+		test_path_exists "$1"
+	}
+	;;
+*)
+	test_path_is_socket () {
+		test -S "$1"
+	}
+	;;
+esac
+
 # don't leave a stale daemon running
 test_atexit 'git credential-cache exit'
 
@@ -21,7 +36,7 @@ test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
 		rmdir -p .cache/git/credential/
 	" &&
 	test_path_is_missing "$HOME/.git-credential-cache" &&
-	test -S "$HOME/.cache/git/credential/socket"
+	test_path_is_socket "$HOME/.cache/git/credential/socket"
 '
 
 XDG_CACHE_HOME="$HOME/xdg"
@@ -31,7 +46,7 @@ helper_test cache
 
 test_expect_success "use custom XDG_CACHE_HOME if set and default sockets are not created" '
 	test_when_finished "git credential-cache exit" &&
-	test -S "$XDG_CACHE_HOME/git/credential/socket" &&
+	test_path_is_socket "$XDG_CACHE_HOME/git/credential/socket" &&
 	test_path_is_missing "$HOME/.git-credential-cache/socket" &&
 	test_path_is_missing "$HOME/.cache/git/credential/socket"
 '
@@ -48,7 +63,7 @@ test_expect_success 'credential-cache --socket option overrides default location
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/dir/socket"
+	test_path_is_socket "$HOME/dir/socket"
 '
 
 test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
@@ -62,7 +77,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.cache/git/credential/socket" &&
+	test_path_is_socket "$HOME/.cache/git/credential/socket" &&
 	XDG_CACHE_HOME="$HOME/xdg" &&
 	export XDG_CACHE_HOME &&
 	check approve cache <<-\EOF &&
@@ -71,7 +86,7 @@ test_expect_success "use custom XDG_CACHE_HOME even if xdg socket exists" '
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$XDG_CACHE_HOME/git/credential/socket"
+	test_path_is_socket "$XDG_CACHE_HOME/git/credential/socket"
 '
 
 test_expect_success 'use user socket if user directory exists' '
@@ -79,14 +94,15 @@ test_expect_success 'use user socket if user directory exists' '
 		git credential-cache exit &&
 		rmdir \"\$HOME/.git-credential-cache/\"
 	" &&
-	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
+	mkdir -p "$HOME/.git-credential-cache/" &&
+	chmod 700 "$HOME/.git-credential-cache/" &&
 	check approve cache <<-\EOF &&
 	protocol=https
 	host=example.com
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.git-credential-cache/socket"
+	test_path_is_socket "$HOME/.git-credential-cache/socket"
 '
 
 test_expect_success SYMLINKS 'use user socket if user directory is a symlink to a directory' '
@@ -103,7 +119,7 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
 	username=store-user
 	password=store-pass
 	EOF
-	test -S "$HOME/.git-credential-cache/socket"
+	test_path_is_socket "$HOME/.git-credential-cache/socket"
 '
 
 helper_test_timeout cache --timeout=1
-- 
2.33.0.481.g26d3bed244

Re: [PATCH v3 1/3] t0301: fixes for windows compatibility

[Ævar Arnfjörð Bjarmason]

On Tue, Sep 14 2021, Carlo Marcelo Arenas Belón wrote:

> [...]
> `mkdir -m` can't represent a valid ACL directly and fails with
> permission problems, so instead call mkdir followed by chmod, which
> has been enhanced to do so.

This looks like a good change:

>  test_expect_success 'use user socket if user directory exists' '
> @@ -79,14 +94,15 @@ test_expect_success 'use user socket if user directory exists' '
>  		git credential-cache exit &&
>  		rmdir \"\$HOME/.git-credential-cache/\"
>  	" &&
> -	mkdir -p -m 700 "$HOME/.git-credential-cache/" &&
> +	mkdir -p "$HOME/.git-credential-cache/" &&
> +	chmod 700 "$HOME/.git-credential-cache/" &&
>  	check approve cache <<-\EOF &&
>  	protocol=https
>  	host=example.com
>  	username=store-user
>  	password=store-pass
>  	EOF
> -	test -S "$HOME/.git-credential-cache/socket"
> +	test_path_is_socket "$HOME/.git-credential-cache/socket"
>  '

But this adjacent changes is also needed to make this pass on
AIX. I.e. for the adjacent test, which I assume "works" on your Windows
setup because it doesn't have SYMLINKS:

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 698b7159f03..120b50e8c14 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -111,7 +111,8 @@ test_expect_success SYMLINKS 'use user socket if user directory is a symlink to
                rmdir \"\$HOME/dir/\" &&
                rm \"\$HOME/.git-credential-cache\"
        " &&
-       mkdir -p -m 700 "$HOME/dir/" &&
+       mkdir -p "$HOME/dir/" &&
+       chmod 700 "$HOME/dir/" &&
        ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
        check approve cache <<-\EOF &&
        protocol=https

FWIW on AIX this fails because apparently they got quoting wrong and end
up trying to shell out to "chmod 700 [trash dir name up to the first
space]", i.e. the "trash directory" part.

Not an rc0 issue, this has been in "master" for a while...

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

[Carlo Marcelo Arenas Belón]

Connect and reset errors aren't what will be expected by POSIX but
are instead 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>
---
v3:
* use a better define as suggested by Dscho
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..78c02ad531 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -11,6 +11,32 @@
 #define FLAG_SPAWN 0x1
 #define FLAG_RELAY 0x2
 
+#ifdef GIT_WINDOWS_NATIVE
+
+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

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

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> @@ -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);

In my earlier review I suggested a helper that checks
recoverability, hence leading to a code structure like this:

		if (!connection_not_yet_open(errno))
			die_errno(...);
		/* otherwise, (re)establish connection and retry */
		...

but the phrasing and semantics you chose to check for unrecoverable
state reads much better.

Thanks.

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

[Junio C Hamano]

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(-)

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);
 }

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

[Jeff King]

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

[PATCH v3 3/3] git-compat-util: include declaration for unix sockets in windows

[Carlo Marcelo Arenas Belón]

Available since Windows 10 release 1803 and Windows Server 2019.

NO_UNIX_SOCKETS is still the default for Windows builds, as they need
to keep backward compatibility with releases up to Windows 7, but allow
including the header otherwise.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v3:
* better commit message as suggested by Dscho

 git-compat-util.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index b46605300a..6a420d104c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -160,6 +160,9 @@
 # endif
 #define WIN32_LEAN_AND_MEAN  /* stops windows.h including winsock.h */
 #include <winsock2.h>
+#ifndef NO_UNIX_SOCKETS
+#include <afunix.h>
+#endif
 #include <windows.h>
 #define GIT_WINDOWS_NATIVE
 #endif
-- 
2.33.0.481.g26d3bed244

Re: [PATCH v3 0/3] windows: allow building without NO_UNIX_SOCKETS

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> Eventhough NO_UNIX_SOCKETS was specifically added to support Windows,
> it might not be necessary in the future, because Windows added support
> for Unix Sockets in late 2017.

Thanks.  I found that this round was a pleasant read.

Will queue, and let's merge it down to 'next' soonish.