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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2021, 01:04 UTC
Message-ID: <xmqq1r5ti8vn.fsf@gitster.g>
In-Reply-To: <20210912202830.25720-2-carenas@gmail.com>

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

Show 6 quoted lines

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

Show 20 quoted lines

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

Show 20 quoted lines

>  	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