[PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

[PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

[COGONI Guillaume]

Use test_path_is_* to replace test [-d|-f] because that give more
explicit debugging information. And it doesn't change the semantics.

Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com>
Co-authored-by: BRESSAT Jonathan <git.jonathan.bressat@gmail.com>
---
 t/t3903-stash.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 686747e55a..d0a4613371 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -390,7 +390,7 @@ 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 file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
 	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
@@ -401,7 +401,7 @@ test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
 	git rm file &&
 	ln -s file2 file &&
 	git stash save "file to symlink (stage rm)" &&
-	test -f file &&
+	test_path_is_file file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
 	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
@@ -413,7 +413,7 @@ test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
 	ln -s file2 file &&
 	git add file &&
 	git stash save "file to symlink (full stage)" &&
-	test -f file &&
+	test_path_is_file file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
 	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
@@ -487,7 +487,7 @@ test_expect_failure 'stash directory to file' '
 	rm -fr dir &&
 	echo bar >dir &&
 	git stash save "directory to file" &&
-	test -d dir &&
+	test_path_is_dir dir &&
 	test foo = "$(cat dir/file)" &&
 	test_must_fail git stash apply &&
 	test bar = "$(cat dir)" &&
@@ -500,10 +500,10 @@ test_expect_failure 'stash file to directory' '
 	mkdir file &&
 	echo foo >file/file &&
 	git stash save "file to directory" &&
-	test -f file &&
+	test_path_is_file file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
-	test -f file/file &&
+	test_path_is_file file/file &&
 	test foo = "$(cat file/file)"
 '
 
-- 
2.25.1

Re: [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

[Junio C Hamano]

COGONI Guillaume <cogoni.guillaume@gmail.com> writes:

> @@ -390,7 +390,7 @@ 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 file &&

This is not wrong per-se, and I know I shouldn't demand too much
from a practice patch like this, but for a real patch, I hope
contributors carefully check if the original is doing the right
thing.

What does the code want to do?

 - The starting state, HEAD, has a 'file' that is a regular file.

 - We remove and replace 'file' with a symbolic link.

 - We stash.

So the expectation here is at this point, 'file' is a regular file
and not a symbolic link.  Some anticipated errors are that "stash
save" fails to turn 'file' back to a regular file include leaving it
as a symbolic link and successfully remove the symblic link version
but somehow failing to recreate a regular file.

Is "test -f file", which was used by the original, the right way to
detect these possible errors?

Whey file2 is a regular file that exists and file is a symbolic link
points at it, i.e. if "stash save" fails to operate, "test -f file" would
still say "Yes, it is a file".

    $ >regular-file
    $ rm -f missing-file
    $ ln -s regular-file link-to-file
    $ ln -s missing-file link-to-missing
    $ test -f regular-file; echo $?
    0
    $ test -f link-to-file; echo $?
    0
    $ test -f link-to-missing; echo $?
    1
    $ test ! -h regular-file && test -f regular-file; echo $?
    0
    $ test ! -h link-to-file && test -f link-to-file; echo $?
    1


As "test_path_is_file" is merely a wrapper around "test -f", this
patch may not make it any worse, but I am skeptical if this is a
good idea, given that possible follow-on project may be one or more
of these:

 * verify that all existing users of test_path_is_file want to
   reject a symlink to file, and add 'test ! -h "$1" &&' to the
   implementation of the test helper in t/test-lib-functions.sh
   (we may want to do the same for test_path_is_dir).

 * introduce test_path_is_symlink and use it appropriately.  This
   will be a more verbose version of "test -h".

 * introduce test_path_is_file_not_symlink and use it here.

If the proposed log message leaves a note on the issue, e.g.

    There are dubious uses of "test -f" in the original that should
    be differentiating a regular file and a symbolic link to an
    existing regular file, but this mechanical conversion patch does
    not fix them.

it would be nicer.

Thanks.

Re: [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

[Cogoni Guillaume]

First of all, sorry for the delay of this answer.

> On 02/11/2022 at 7:02 PM, Junio ​​C Hamano wrote:
>
> This is not wrong per-se, and I know I shouldn't demand too much
> from a practice patch like this, but for a real patch, I hope
> contributors carefully check if the original is doing the right
> thing.

It's good that you are demanding even for a practice patch because we 
are here to learn as much as we can. And, we will take a good attention 
to your ideas.

>   * verify that all existing users of test_path_is_file want to
>     reject a symlink to file, and add 'test ! -h "$1" &&' to the
>     implementation of the test helper in t/test-lib-functions.sh
>     (we may want to do the same for test_path_is_dir).
>
>   * introduce test_path_is_symlink and use it appropriately.  This
>     will be a more verbose version of "test -h".
>
>   * introduce test_path_is_file_not_symlink and use it here.

We wouldn't modify test_path_is_file because this function is already 
use and we won't verify if every uses of this are rejecting symlink.

However, we would like to try to implement test_path_is_symlink and 
test_path_is_file_not_symlink and the symmetric for directory.

Thanks for your review and the ideas.
COGONI Guillaume and BRESSAT Jonathan

Re: [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

[Ævar Arnfjörð Bjarmason]

On Mon, Feb 14 2022, Cogoni Guillaume wrote:

>>   * verify that all existing users of test_path_is_file want to
>>     reject a symlink to file, and add 'test ! -h "$1" &&' to the
>>     implementation of the test helper in t/test-lib-functions.sh
>>     (we may want to do the same for test_path_is_dir).
>>
>>   * introduce test_path_is_symlink and use it appropriately.  This
>>     will be a more verbose version of "test -h".
>>
>>   * introduce test_path_is_file_not_symlink and use it here.
>
> We wouldn't modify test_path_is_file because this function is already
> use and we won't verify if every uses of this are rejecting symlink.

Perhaps it's not a good idea (I haven't checked) to change it like that.

But it's fine to change these sorts of test functions even if there's
existing users of it, our test suite isn't a stable API.

Of course one still has to consider outstanding patches, anything in the
list archive we may want to dig up etc., so it's best not to do so
without good reason.

But the "verifying every use" should mostly be just running "make test",
and pushing to the GitHub CI.

[PATCH v2 0/2] replace test [-f|-d] with more verbose functions

[COGONI Guillaume]

> On 02/11/2022 at 7:02 PM, Junio ​​C Hamano wrote:

> * introduce test_path_is_symlink and use it appropriately.  This
>   will be a more verbose version of "test -h".

> * introduce test_path_is_file_not_symlink and use it here.

Replace test [-f|-d] in t/t3903-stash.sh by test_path_is_*
Add new functions like test_path_is_* to cover more specifics cases like
symbolic link or file that we explicitly refuse to be symbolic link.

COGONI Guillaume (2):
  t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*
  Add new tests functions like test_path_is_*

 t/t3903-stash.sh        | 21 +++++++++------------
 t/test-lib-functions.sh | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e8e933dc4e..0ec19a4499 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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_path_is_file file &&
+       test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply &&
-       case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+       git stash apply
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
@@ -401,10 +400,9 @@ test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
        git rm file &&
        ln -s file2 file &&
        git stash save "file to symlink (stage rm)" &&
-       test_path_is_file file &&
+       test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply &&
-       case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+       git stash apply
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
@@ -413,10 +411,9 @@ test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
        ln -s file2 file &&
        git add file &&
        git stash save "file to symlink (full stage)" &&
-       test_path_is_file file &&
+       test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply &&
-       case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+       git stash apply
 '
 
 # This test creates a commit with a symlink used for the following tests
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"
+       then
+               echo "$1 is a symbolic link"
+               false
+       fi
+}
+
 test_path_is_dir () {
        test "$#" -ne 1 && BUG "1 param"
        if ! test -d "$1"
@@ -865,6 +875,16 @@ test_path_is_dir () {
        fi
 }
 
+test_path_is_dir_not_symlink () {
+       test "$#" -ne 1 && BUG "1 param"
+       test_path_is_dir "$1" &&
+       if ! test ! -h "$1"
+       then
+               echo "$1 is a symbolic link"
+               false
+       fi
+}
+
 test_path_exists () {
        test "$#" -ne 1 && BUG "1 param"
        if ! test -e "$1"
-- 
2.25.1

[PATCH v2 0/2] replace test [-f|-d] with more verbose functions

[COGONI Guillaume]

> On 02/11/2022 at 7:02 PM, Junio ​​C Hamano wrote:

> * introduce test_path_is_symlink and use it appropriately.  This
>   will be a more verbose version of "test -h".

> * introduce test_path_is_file_not_symlink and use it here.

Replace test [-f|-d] in t/t3903-stash.sh by test_path_is_*
Add new functions like test_path_is_* to cover more specifics cases like
symbolic link or file that we explicitly refuse to be symbolic link.

COGONI Guillaume (2):
  t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*
  Add new tests functions like test_path_is_*

 t/t3903-stash.sh        | 21 +++++++++------------
 t/test-lib-functions.sh | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e8e933dc4e..0ec19a4499 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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_path_is_file file &&
+       test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply &&
-       case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+       git stash apply
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
@@ -401,10 +400,9 @@ test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
        git rm file &&
        ln -s file2 file &&
        git stash save "file to symlink (stage rm)" &&
-       test_path_is_file file &&
+       test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply &&
-       case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+       git stash apply
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
@@ -413,10 +411,9 @@ test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
        ln -s file2 file &&
        git add file &&
        git stash save "file to symlink (full stage)" &&
-       test_path_is_file file &&
+       test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply &&
-       case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+       git stash apply
 '
 
 # This test creates a commit with a symlink used for the following tests
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"
+       then
+               echo "$1 is a symbolic link"
+               false
+       fi
+}
+
 test_path_is_dir () {
        test "$#" -ne 1 && BUG "1 param"
        if ! test -d "$1"
@@ -865,6 +875,16 @@ test_path_is_dir () {
        fi
 }
 
+test_path_is_dir_not_symlink () {
+       test "$#" -ne 1 && BUG "1 param"
+       test_path_is_dir "$1" &&
+       if ! test ! -h "$1"
+       then
+               echo "$1 is a symbolic link"
+               false
+       fi
+}
+
 test_path_exists () {
        test "$#" -ne 1 && BUG "1 param"
        if ! test -e "$1"
-- 
2.25.1

[PATCH v2 1/2] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

[COGONI Guillaume]

Use test_path_is_* to replace test [-d|-f] because that give more
explicit debugging information. And it doesn't change the semantics.

Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com>
Co-authored-by: BRESSAT Jonathan <git.jonathan.bressat@gmail.com>
---
 t/t3903-stash.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b149e2af44..11a0856873 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -487,7 +487,7 @@ test_expect_failure 'stash directory to file' '
 	rm -fr dir &&
 	echo bar >dir &&
 	git stash save "directory to file" &&
-	test -d dir &&
+	test_path_is_dir dir &&
 	test foo = "$(cat dir/file)" &&
 	test_must_fail git stash apply &&
 	test bar = "$(cat dir)" &&
@@ -500,10 +500,10 @@ test_expect_failure 'stash file to directory' '
 	mkdir file &&
 	echo foo >file/file &&
 	git stash save "file to directory" &&
-	test -f file &&
+	test_path_is_file file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
-	test -f file/file &&
+	test_path_is_file file/file &&
 	test foo = "$(cat file/file)"
 '
 
-- 
2.25.1

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

[COGONI Guillaume]

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.

Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com>
Co-authored-by: BRESSAT Jonathan <git.jonathan.bressat@gmail.com>
---
 t/t3903-stash.sh        | 15 ++++++---------
 t/test-lib-functions.sh | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 11a0856873..0ec19a4499 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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 &&
 	test bar = "$(cat file)" &&
-	git stash apply &&
-	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+	git stash apply
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
@@ -401,10 +400,9 @@ test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
 	git rm file &&
 	ln -s file2 file &&
 	git stash save "file to symlink (stage rm)" &&
-	test -f file &&
+	test_path_is_file_not_symlink file &&
 	test bar = "$(cat file)" &&
-	git stash apply &&
-	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+	git stash apply
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
@@ -413,10 +411,9 @@ test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
 	ln -s file2 file &&
 	git add file &&
 	git stash save "file to symlink (full stage)" &&
-	test -f file &&
+	test_path_is_file_not_symlink file &&
 	test bar = "$(cat file)" &&
-	git stash apply &&
-	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+	git stash apply
 '
 
 # This test creates a commit with a symlink used for the following tests
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"
+	then
+		echo "$1 is a symbolic link"
+		false
+	fi
+}
+
 test_path_is_dir () {
 	test "$#" -ne 1 && BUG "1 param"
 	if ! test -d "$1"
@@ -865,6 +875,16 @@ test_path_is_dir () {
 	fi
 }
 
+test_path_is_dir_not_symlink () {
+	test "$#" -ne 1 && BUG "1 param"
+	test_path_is_dir "$1" &&
+	if ! test ! -h "$1"
+	then
+		echo "$1 is a symbolic link"
+		false
+	fi
+}
+
 test_path_exists () {
 	test "$#" -ne 1 && BUG "1 param"
 	if ! test -e "$1"
-- 
2.25.1

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

[Junio C Hamano]

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.

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

[PATCH v3 0/3] replace test [-f|-d] with more verbose functions

[COGONI Guillaume]

Make the code more readable in t/t3903-stash.sh and give more 
friendly error message by replacing test [-f|-d] by the right 
test_path_is_* functions.
Add new functions like test_path_is_* to cover more specifics 
cases like symbolic link or file that we explicitly refuse
to be symbolic link.

> On 18/11/2022, Junio ​​C Hamano wrote:

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

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

-	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac    
+	test_path_is_symlink file &&
+	test "$(test_readlink file)" = file2

Firstly, we check if file is a symbolic link then if it is a symbolic link
on file2.
We check if it is a symbolic link because test_readlink() raise an error 
if we give it something that is not a symbolic link and this error is less
readable.

> Why not
> 	if test -h "$1"
> instead???  I think "is truly a dir not a symlink" has the same
> "Huh?" puzzle.

We fixed it.



COGONI Guillaume (3):
  t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*
  tests: allow testing if a path is truly a file or a directory
  tests: make the code more readable

 t/t3903-stash.sh        | 21 ++++++++++++---------
 t/test-lib-functions.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 9 deletions(-)


Difference between V2 and V3.
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 0ec19a4499..d5ecee4fcc 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -392,7 +392,9 @@ test_expect_success SYMLINKS 'stash file to symlink' '
        git stash save "file to symlink" &&
        test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply
+       git stash apply &&
+       test_path_is_symlink file &&
+       test "$(test_readlink file)" = file2
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
@@ -402,7 +404,9 @@ test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
        git stash save "file to symlink (stage rm)" &&
        test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply
+       git stash apply &&
+       test_path_is_symlink file &&
+       test "$(test_readlink file)" = file2
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
@@ -413,7 +417,9 @@ test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
        git stash save "file to symlink (full stage)" &&
        test_path_is_file_not_symlink file &&
        test bar = "$(cat file)" &&
-       git stash apply
+       git stash apply &&
+       test_path_is_symlink file &&
+       test "$(test_readlink file)" = file2
 '
 
 # This test creates a commit with a symlink used for the following tests
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 61fc5f37e3..0f439c99d6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -859,9 +859,9 @@ test_path_is_file () {
 test_path_is_file_not_symlink () {
        test "$#" -ne 1 && BUG "1 param"
        test_path_is_file "$1" &&
-       if ! test ! -h "$1"
+       if test -h "$1"
        then
-               echo "$1 is a symbolic link"
+               echo "$1 shouldn't be a symbolic link"
                false
        fi
 }
@@ -878,9 +878,9 @@ test_path_is_dir () {
 test_path_is_dir_not_symlink () {
        test "$#" -ne 1 && BUG "1 param"
        test_path_is_dir "$1" &&
-       if ! test ! -h "$1"
+       if test -h "$1"
        then
-               echo "$1 is a symbolic link"
+               echo "$1 shouldn't be a symbolic link"
                false
        fi
 }
@@ -894,6 +894,15 @@ test_path_exists () {
        fi
 }
 
+test_path_is_symlink () {
+       test "$#" -ne 1 && BUG "1 param"
+       if ! test -h "$1"
+       then
+               echo "Symbolic link $1 doesn't exist"
+               false
+       fi
+}
+

-- 
2.25.1

[PATCH v3 1/3] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*

[COGONI Guillaume]

Use test_path_is_* to replace test [-d|-f] because that give more
explicit debugging information. And it doesn't change the semantics.

Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com>
Co-authored-by: BRESSAT Jonathan <git.jonathan.bressat@gmail.com>
---
 t/t3903-stash.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b149e2af44..11a0856873 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -487,7 +487,7 @@ test_expect_failure 'stash directory to file' '
 	rm -fr dir &&
 	echo bar >dir &&
 	git stash save "directory to file" &&
-	test -d dir &&
+	test_path_is_dir dir &&
 	test foo = "$(cat dir/file)" &&
 	test_must_fail git stash apply &&
 	test bar = "$(cat dir)" &&
@@ -500,10 +500,10 @@ test_expect_failure 'stash file to directory' '
 	mkdir file &&
 	echo foo >file/file &&
 	git stash save "file to directory" &&
-	test -f file &&
+	test_path_is_file file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
-	test -f file/file &&
+	test_path_is_file file/file &&
 	test foo = "$(cat file/file)"
 '
 
-- 
2.25.1

[PATCH v3 2/3] tests: allow testing if a path is truly a file or a directory

[COGONI Guillaume]

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.
Give more friendly error message.

Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com>
Co-authored-by: BRESSAT Jonathan <git.jonathan.bressat@gmail.com>
---
 t/t3903-stash.sh        |  6 +++---
 t/test-lib-functions.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 11a0856873..a6ad52db9f 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -390,7 +390,7 @@ 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 &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
 	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
@@ -401,7 +401,7 @@ test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
 	git rm file &&
 	ln -s file2 file &&
 	git stash save "file to symlink (stage rm)" &&
-	test -f file &&
+	test_path_is_file_not_symlink file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
 	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
@@ -413,7 +413,7 @@ test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
 	ln -s file2 file &&
 	git add file &&
 	git stash save "file to symlink (full stage)" &&
-	test -f file &&
+	test_path_is_file_not_symlink file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
 	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 85385d2ede..0f439c99d6 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"
+	then
+		echo "$1 shouldn't be a symbolic link"
+		false
+	fi
+}
+
 test_path_is_dir () {
 	test "$#" -ne 1 && BUG "1 param"
 	if ! test -d "$1"
@@ -865,6 +875,16 @@ test_path_is_dir () {
 	fi
 }
 
+test_path_is_dir_not_symlink () {
+	test "$#" -ne 1 && BUG "1 param"
+	test_path_is_dir "$1" &&
+	if test -h "$1"
+	then
+		echo "$1 shouldn't be a symbolic link"
+		false
+	fi
+}
+
 test_path_exists () {
 	test "$#" -ne 1 && BUG "1 param"
 	if ! test -e "$1"
@@ -874,6 +894,15 @@ test_path_exists () {
 	fi
 }
 
+test_path_is_symlink () {
+	test "$#" -ne 1 && BUG "1 param"
+	if ! test -h "$1"
+	then
+		echo "Symbolic link $1 doesn't exist"
+		false
+	fi
+}
+
 # Check if the directory exists and is empty as expected, barf otherwise.
 test_dir_is_empty () {
 	test "$#" -ne 1 && BUG "1 param"
-- 
2.25.1

[PATCH v3 3/3] tests: make the code more readable

[COGONI Guillaume]

Replace the parsing of the output of "ls -l" by test_path_is_symlink() and
test_readlink().

Signed-off-by: COGONI Guillaume <cogoni.guillaume@gmail.com>
Co-authored-by: BRESSAT Jonathan <git.jonathan.bressat@gmail.com>
---
 t/t3903-stash.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index a6ad52db9f..d5ecee4fcc 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -393,7 +393,8 @@ test_expect_success SYMLINKS 'stash file to symlink' '
 	test_path_is_file_not_symlink file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
-	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+	test_path_is_symlink file &&
+	test "$(test_readlink file)" = file2
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
@@ -404,7 +405,8 @@ test_expect_success SYMLINKS 'stash file to symlink (stage rm)' '
 	test_path_is_file_not_symlink file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
-	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+	test_path_is_symlink file &&
+	test "$(test_readlink file)" = file2
 '
 
 test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
@@ -416,7 +418,8 @@ test_expect_success SYMLINKS 'stash file to symlink (full stage)' '
 	test_path_is_file_not_symlink file &&
 	test bar = "$(cat file)" &&
 	git stash apply &&
-	case "$(ls -l file)" in *" file -> file2") :;; *) false;; esac
+	test_path_is_symlink file &&
+	test "$(test_readlink file)" = file2
 '
 
 # This test creates a commit with a symlink used for the following tests
-- 
2.25.1

Re: [PATCH v3 0/3] replace test [-f|-d] with more verbose functions

[Junio C Hamano]

COGONI Guillaume <cogoni.guillaume@gmail.com> writes:

> Make the code more readable in t/t3903-stash.sh and give more 
> friendly error message by replacing test [-f|-d] by the right 
> test_path_is_* functions.
> Add new functions like test_path_is_* to cover more specifics 
> cases like symbolic link or file that we explicitly refuse
> to be symbolic link.

All three look good to me.

Will queue.

As a possible #leftoverbits material, I suspect that we would
eventually want to be able to say

	test_path_is_file ! "$error_if_I_am_a_file"
	test_path_is_dir ! "$error_if_I_am_a_dir"
	test_path_is_symlink ! "$error_if_I_am_a_symlink"

so that we do not have to have the two ugly-looking special-case
combination "test_path_is_X_not_symlink" but just express what we
want with

	test_path_is_file "$path" && test_path_is_symlink ! "$path"

Once that happens, the two helpers introduced with 2/3 of this
series would become

	test_path_is_file_not_symlink () {
		test $# = 1 || BUG "1 param"
		test_path_is_file "$1" &&
		test_path_is_symlink ! "$1"
	}

But I do not want to see that as part of this series.  Let's
conclude this series and declare a success.

Thanks.

Re: [PATCH v3 0/3] replace test [-f|-d] with more verbose functions

[Cogoni Guillaume]

Hello,

Thanks everyone for your help and reviews.
See you next time in the mailing list for some new patches or reviews.

Sincerly,

COGONI Guillaume and BRESSAT Jonathan