COGONI Guillaume <cogoni.guillaume@xxxxxxxxx> writes:
> Subject: Re: [PATCH v2 2/2] Add new tests functions like test_path_is_*
I'd retitle the commit to "tests: allow testing if a path is truly
a file or a directory", to follow the convention to highlight that
this change is about the tests.
> Add test_path_is_file_not_symlink(), test_path_is_dir_not_symlink()
> and test_path_is_symlink(). Case of use for the first one
> in test t/t3903-stash.sh to replace "test -f" because that function
> explicitly want the file not to be a symlink by parsing the output
> of "ls -l". Make the code more readable and give more friendly error
> message.
Interesting. I'll mention why I think you want to rewrite that "by
parsing the output of 'ls -l'" later.
I initially didn't like the "is file and not symlink" suggestion I
made, simply because it looked like it is asking for combinatorial
explosion, but because the only types of filesystem entities that
are not symlink that we care about are files and directories, so we
only need two new variants that say "_not_symlink" in the name,
it is probably not too bad.
> @@ -390,10 +390,9 @@ test_expect_success SYMLINKS 'stash file to symlink' '
> rm file &&
> ln -s file2 file &&
> git stash save "file to symlink" &&
> - test -f file &&
> + test_path_is_file_not_symlink file &&
And this is exactly the new helper is meant to be used. It was
originally a regular file, the test tentatively made it into a
symbolic link, but that tentative change is supposed to be reverted
by the "stash save", so we do want it to be a true regular file.
> test bar = "$(cat file)" &&
> - git stash apply &&
> - case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
> + git stash apply
However, the removal of the "make sure file is a symbolic link and
it points at file2" is not justifiable with the proposed commit
message. If the original code were like this ...
test bar = "$(cat file)" &&
case "$(ls -l file)" in *" file -> file2") false;; esac &&
git stash apply &&
case "$(ls -l file)" in *" file -> file2") :;; *) false ;;esac
... the test _before_ "stash apply" is checking if "file" is a
regular file, the "ls -l" output is used to make sure file is not a
symbolic link that points at file2. But that is not the original
code did, which invalidates the part of the proposed commit log
message.
The "ls -l" parsing the original does is to check how "stash apply"
recovers the stashed state, where "file" wants to be a symbolic link
and it wants to be pointing at "file2".
It seems we have test_readlink() available these days, so with a
separate clean-up patch, you may want to make the final version
to read something like this, perhaps?
test_path_is_file_not_symlink file &&
test bar ="$(cat file") &&
git stash apply &&
test "$(test_readlink file)" = file2
I am not sure what test_readlink which is a one-liner Perl script
does when it is fed a non symbolic link, so I do not know if the
"path is truly a file and not a symlink" can be done as
test -f file && ! test_readlink file &&
I think the other two hunks to this file have identical issues.
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 85385d2ede..61fc5f37e3 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -856,6 +856,16 @@ test_path_is_file () {
> fi
> }
>
> +test_path_is_file_not_symlink () {
> + test "$#" -ne 1 && BUG "1 param"
> + test_path_is_file "$1" &&
> + if ! test ! -h "$1"
Why not
if test -h "$1"
instead??? I think "is truly a dir not a symlink" has the same
"Huh?" puzzle.
Thanks.
