| Message ID | 20220211134655.1149320-1-cogoni.guillaume@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t/t3903-stash.sh: replace test [-d|-f] with test_path_is_* | expand |
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.
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
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.
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)" '
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(-)