* [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* @ 2022-02-11 13:46 COGONI Guillaume 2022-02-11 18:02 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: COGONI Guillaume @ 2022-02-11 13:46 UTC (permalink / raw) To: git; +Cc: git.jonathan.bressat, guillaume.cogoni, matthieu.moy, 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* 2022-02-11 13:46 [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume @ 2022-02-11 18:02 ` Junio C Hamano 2022-02-14 20:22 ` Cogoni Guillaume 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2022-02-11 18:02 UTC (permalink / raw) To: COGONI Guillaume Cc: git, git.jonathan.bressat, guillaume.cogoni, matthieu.moy 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* 2022-02-11 18:02 ` Junio C Hamano @ 2022-02-14 20:22 ` Cogoni Guillaume 2022-02-15 22:13 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 15+ messages in thread From: Cogoni Guillaume @ 2022-02-14 20:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, git.jonathan.bressat, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* 2022-02-14 20:22 ` Cogoni Guillaume @ 2022-02-15 22:13 ` Ævar Arnfjörð Bjarmason 2022-02-18 17:10 ` [PATCH v2 0/2] replace test [-f|-d] with more verbose functions COGONI Guillaume 2022-02-18 17:12 ` COGONI Guillaume 0 siblings, 2 replies; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-02-15 22:13 UTC (permalink / raw) To: Cogoni Guillaume Cc: Junio C Hamano, git, git.jonathan.bressat, guillaume.cogoni, matthieu.moy 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/2] replace test [-f|-d] with more verbose functions 2022-02-15 22:13 ` Ævar Arnfjörð Bjarmason @ 2022-02-18 17:10 ` COGONI Guillaume 2022-02-18 17:12 ` COGONI Guillaume 1 sibling, 0 replies; 15+ messages in thread From: COGONI Guillaume @ 2022-02-18 17:10 UTC (permalink / raw) To: avarab Cc: cogoni.guillaume, git.jonathan.bressat, git, gitster, guillaume.cogoni, matthieu.moy > 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 0/2] replace test [-f|-d] with more verbose functions 2022-02-15 22:13 ` Ævar Arnfjörð Bjarmason 2022-02-18 17:10 ` [PATCH v2 0/2] replace test [-f|-d] with more verbose functions COGONI Guillaume @ 2022-02-18 17:12 ` COGONI Guillaume 2022-02-18 17:12 ` [PATCH v2 1/2] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume 2022-02-18 17:12 ` [PATCH v2 2/2] Add new tests functions like test_path_is_* COGONI Guillaume 1 sibling, 2 replies; 15+ messages in thread From: COGONI Guillaume @ 2022-02-18 17:12 UTC (permalink / raw) To: avarab Cc: cogoni.guillaume, git.jonathan.bressat, git, gitster, guillaume.cogoni, matthieu.moy > 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* 2022-02-18 17:12 ` COGONI Guillaume @ 2022-02-18 17:12 ` COGONI Guillaume 2022-02-18 17:12 ` [PATCH v2 2/2] Add new tests functions like test_path_is_* COGONI Guillaume 1 sibling, 0 replies; 15+ messages in thread From: COGONI Guillaume @ 2022-02-18 17:12 UTC (permalink / raw) To: avarab Cc: cogoni.guillaume, git.jonathan.bressat, git, gitster, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] Add new tests functions like test_path_is_* 2022-02-18 17:12 ` COGONI Guillaume 2022-02-18 17:12 ` [PATCH v2 1/2] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume @ 2022-02-18 17:12 ` COGONI Guillaume 2022-02-18 18:48 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: COGONI Guillaume @ 2022-02-18 17:12 UTC (permalink / raw) To: avarab Cc: cogoni.guillaume, git.jonathan.bressat, git, gitster, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] Add new tests functions like test_path_is_* 2022-02-18 17:12 ` [PATCH v2 2/2] Add new tests functions like test_path_is_* COGONI Guillaume @ 2022-02-18 18:48 ` Junio C Hamano 2022-02-22 21:54 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions COGONI Guillaume 0 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2022-02-18 18:48 UTC (permalink / raw) To: COGONI Guillaume Cc: avarab, git.jonathan.bressat, git, guillaume.cogoni, matthieu.moy 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 0/3] replace test [-f|-d] with more verbose functions 2022-02-18 18:48 ` Junio C Hamano @ 2022-02-22 21:54 ` COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 1/3] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: COGONI Guillaume @ 2022-02-22 21:54 UTC (permalink / raw) To: gitster Cc: avarab, cogoni.guillaume, git.jonathan.bressat, git, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 1/3] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* 2022-02-22 21:54 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions COGONI Guillaume @ 2022-02-22 21:54 ` COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 2/3] tests: allow testing if a path is truly a file or a directory COGONI Guillaume ` (2 subsequent siblings) 3 siblings, 0 replies; 15+ messages in thread From: COGONI Guillaume @ 2022-02-22 21:54 UTC (permalink / raw) To: gitster Cc: avarab, cogoni.guillaume, git.jonathan.bressat, git, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/3] tests: allow testing if a path is truly a file or a directory 2022-02-22 21:54 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 1/3] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume @ 2022-02-22 21:54 ` COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 3/3] tests: make the code more readable COGONI Guillaume 2022-02-23 22:59 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions Junio C Hamano 3 siblings, 0 replies; 15+ messages in thread From: COGONI Guillaume @ 2022-02-22 21:54 UTC (permalink / raw) To: gitster Cc: avarab, cogoni.guillaume, git.jonathan.bressat, git, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 3/3] tests: make the code more readable 2022-02-22 21:54 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 1/3] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 2/3] tests: allow testing if a path is truly a file or a directory COGONI Guillaume @ 2022-02-22 21:54 ` COGONI Guillaume 2022-02-23 22:59 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions Junio C Hamano 3 siblings, 0 replies; 15+ messages in thread From: COGONI Guillaume @ 2022-02-22 21:54 UTC (permalink / raw) To: gitster Cc: avarab, cogoni.guillaume, git.jonathan.bressat, git, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/3] replace test [-f|-d] with more verbose functions 2022-02-22 21:54 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions COGONI Guillaume ` (2 preceding siblings ...) 2022-02-22 21:54 ` [PATCH v3 3/3] tests: make the code more readable COGONI Guillaume @ 2022-02-23 22:59 ` Junio C Hamano 2022-02-24 18:22 ` Cogoni Guillaume 3 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2022-02-23 22:59 UTC (permalink / raw) To: COGONI Guillaume Cc: avarab, git.jonathan.bressat, git, guillaume.cogoni, matthieu.moy 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/3] replace test [-f|-d] with more verbose functions 2022-02-23 22:59 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions Junio C Hamano @ 2022-02-24 18:22 ` Cogoni Guillaume 0 siblings, 0 replies; 15+ messages in thread From: Cogoni Guillaume @ 2022-02-24 18:22 UTC (permalink / raw) To: Junio C Hamano Cc: avarab, git.jonathan.bressat, git, guillaume.cogoni, matthieu.moy 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-02-24 18:23 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-11 13:46 [PATCH] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume 2022-02-11 18:02 ` Junio C Hamano 2022-02-14 20:22 ` Cogoni Guillaume 2022-02-15 22:13 ` Ævar Arnfjörð Bjarmason 2022-02-18 17:10 ` [PATCH v2 0/2] replace test [-f|-d] with more verbose functions COGONI Guillaume 2022-02-18 17:12 ` COGONI Guillaume 2022-02-18 17:12 ` [PATCH v2 1/2] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume 2022-02-18 17:12 ` [PATCH v2 2/2] Add new tests functions like test_path_is_* COGONI Guillaume 2022-02-18 18:48 ` Junio C Hamano 2022-02-22 21:54 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 1/3] t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 2/3] tests: allow testing if a path is truly a file or a directory COGONI Guillaume 2022-02-22 21:54 ` [PATCH v3 3/3] tests: make the code more readable COGONI Guillaume 2022-02-23 22:59 ` [PATCH v3 0/3] replace test [-f|-d] with more verbose functions Junio C Hamano 2022-02-24 18:22 ` Cogoni Guillaume
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox