| Message ID | pull.1661.git.1707663197543.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | t9146: replace test -d/-f with appropriate test_path_is_* function | expand |
On Sun, Feb 11, 2024 at 9:53 AM Chandra Pratap via GitGitGadget <gitgitgadget@gmail.com> wrote: > The helper functions test_path_is_* provide better debugging > information than test -d/-e/-f. > > Replace "! test -d" with "test_path_is_missing" at places where > we check for non-existent directories. > > Replace "test -f" with "test_path_is_file" and "test -d" with > "test_path_is_dir" at places where we expect files or directories > to exist. The aim of this patch makes sense, but the implementation needs some refinement... > diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh > @@ -20,7 +20,7 @@ test_expect_success 'empty directories exist' ' > for i in a b c d d/e d/e/f "weird file name" > do > - if ! test -d "$i" > + if test_path_is_missing "$i" > then > echo >&2 "$i does not exist" && > exit 1 The point of functions such as test_path_is_missing() is to _assert_ that some condition is true, thus allowing the test to succeed; if the condition is not true, then the function prints an error message and the test aborts and fails. test_path_is_missing () { if test -e "$1" then echo "Path exists:" ls -ld "$1" false fi } It is meant to replace noisy code such as: if ! test -f bloop then echo >&2 "error message" && exit 1 fi && other-code with much simpler: test_path_exists bloop && other-code So, the changes made by this patch are incorrect in two ways... First, the patch retains code which prints an error message even though this code becomes redundant since the test_path_foo() functions already take care of printing the error message. Second, and more problematic, the patch incorrectly inverts the sense of what is being tested. For instance, the title of this test is "empty directories exist", and the body of the test asserts that the empty directories _do_ exist, but the patch changes the condition to assert that the directories do _not_ exist, which is wrong. Taking these observations into account, this test should become: test_expect_success 'empty directories exist' ' ( cd cloned && for i in a b c d d/e d/e/f "weird file name" do test_path_exists "$i" || exit 1 done ) ' Many of the other changes made by this patch suffer similar problems > @@ -138,16 +138,16 @@ test_expect_success 'git svn gc-ed files work' ' > : Compress::Zlib may not be available && > - if test -f "$unhandled".gz > + if test_path_is_file "$unhandled".gz > then > svn_cmd mkdir -m gz "$svnrepo"/gz && > git reset --hard $(git rev-list HEAD | tail -1) && This change is wrong/undesirable for a different reason. Taking into account what was said above about test_path_is_file() being an _assertion_ that some condition is true, coupled with the comment above this `if` statement which says "Compress::Zlib may not be available", then this `test -f` is legitimately part of the control-flow of the function. It is not a mere assertion. Thus, replacing it with the assertion function test_path_is_file() breaks the test for the case when Compress::Zlib is not available. > - test -f "$unhandled".gz && > - test -f "$unhandled" && > + test_path_is_file "$unhandled".gz && > + test_path_is_file "$unhandled" && These replacements are correct in that they replace the _assertion_ `test -f` with the equivalent assertion `test_path_is_file`.
diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh index 09606f1b3cf..532f5baa208 100755 --- a/t/t9146-git-svn-empty-dirs.sh +++ b/t/t9146-git-svn-empty-dirs.sh @@ -20,7 +20,7 @@ test_expect_success 'empty directories exist' ' cd cloned && for i in a b c d d/e d/e/f "weird file name" do - if ! test -d "$i" + if test_path_is_missing "$i" then echo >&2 "$i does not exist" && exit 1 @@ -37,7 +37,7 @@ test_expect_success 'option automkdirs set to false' ' git svn fetch && for i in a b c d d/e d/e/f "weird file name" do - if test -d "$i" + if test_path_is_dir "$i" then echo >&2 "$i exists" && exit 1 @@ -52,7 +52,7 @@ test_expect_success 'more emptiness' ' test_expect_success 'git svn rebase creates empty directory' ' ( cd cloned && git svn rebase ) && - test -d cloned/"! !" + test_path_is_dir cloned/"! !" ' test_expect_success 'git svn mkdirs recreates empty directories' ' @@ -62,7 +62,7 @@ test_expect_success 'git svn mkdirs recreates empty directories' ' git svn mkdirs && for i in a b c d d/e d/e/f "weird file name" "! !" do - if ! test -d "$i" + if test_path_is_missing "$i" then echo >&2 "$i does not exist" && exit 1 @@ -78,21 +78,21 @@ test_expect_success 'git svn mkdirs -r works' ' git svn mkdirs -r7 && for i in a b c d d/e d/e/f "weird file name" do - if ! test -d "$i" + if test_path_is_missing "$i" then echo >&2 "$i does not exist" && exit 1 fi done && - if test -d "! !" + if test_path_is_dir "! !" then echo >&2 "$i should not exist" && exit 1 fi && git svn mkdirs -r8 && - if ! test -d "! !" + if test_path_is_missing "! !" then echo >&2 "$i not exist" && exit 1 @@ -114,7 +114,7 @@ test_expect_success 'empty directories in trunk exist' ' cd trunk && for i in a "weird file name" do - if ! test -d "$i" + if test_path_is_missing "$i" then echo >&2 "$i does not exist" && exit 1 @@ -138,16 +138,16 @@ test_expect_success 'git svn gc-ed files work' ' cd removed && git svn gc && : Compress::Zlib may not be available && - if test -f "$unhandled".gz + if test_path_is_file "$unhandled".gz then svn_cmd mkdir -m gz "$svnrepo"/gz && git reset --hard $(git rev-list HEAD | tail -1) && git svn rebase && - test -f "$unhandled".gz && - test -f "$unhandled" && + test_path_is_file "$unhandled".gz && + test_path_is_file "$unhandled" && for i in a b c "weird file name" gz "! !" do - if ! test -d "$i" + if test_path_is_missing "$i" then echo >&2 "$i does not exist" && exit 1