Re: [PATCH v2 2/2] Add new tests functions like test_path_is_*

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2022, 18:48 UTC
Message-ID: <xmqqbkz4105s.fsf@gitster.g>
In-Reply-To: <20220218171224.262698-3-cogoni.guillaume@gmail.com>

COGONI Guillaume <cogoni.guillaume@gmail.com> 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.

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 12 quoted lines

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