| Message ID | 20210912202830.25720-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | windows: allow building without NO_UNIX_SOCKETS | expand |
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
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?
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
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.
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
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(-)