patcht9146: replace test -d/-f with appropriate test_path_is_* function

From: Chandra Pratap <chandrapratap3519@gmail.com>

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.

Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
    t9146: replace test -d/-f with appropriate test_path_is_* function

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1661

 t/t9146-git-svn-empty-dirs.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

On Sun, Feb 11, 2024 at 9:53 AM Chandra Pratap via GitGitGadget <gitgitgadget@gmail.com> wrote:

The aim of this patch makes sense, but the implementation needs some refinement...

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

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.

These replacements are correct in that they replace the _assertion_ `test -f` with the equivalent assertion `test_path_is_file`.

From: Chandra Pratap <chandrapratap3519@gmail.com>

The helper functions test_path_is_* provide better debugging information than test -d/-e/-f.

Replace "if ! test -d then <error message>" with "test_path_exists" and "test -d" with "test_path_is_dir" at places where we check for existent directories.

Replace "test -f" with "test_path_is_file" at places where we check for existent files.

Replace "test ! -e" with "test_path_is_missing" where we check for non-existent directories.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
    t9146: replace test -d/-f with appropriate test_path_is_* function
    
    cc: Eric Sunshine sunshine@sunshineco.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1661

Range-diff vs v1:

 1:  93fe9e9eef7 ! 1:  5734b9edd61 t9146: replace test -d/-f with appropriate test_path_is_* function
     @@ Metadata
      Author: Chandra Pratap <chandrapratap3519@gmail.com>
      
       ## Commit message ##
     -    t9146: replace test -d/-f with appropriate test_path_is_* function
     +    t9146: replace test -d/-e/-f with appropriate test_path_is_* function
      
          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 "if ! test -d then <error message>" with "test_path_exists"
     +    and "test -d" with "test_path_is_dir" at places where we check for
     +    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.
     +    Replace "test -f" with "test_path_is_file" at places where we check
     +    for existent files.
      
     +    Replace "test ! -e" with "test_path_is_missing" where we check for
     +    non-existent directories.
     +
     +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
      
       ## t/t9146-git-svn-empty-dirs.sh ##
     @@ t/t9146-git-svn-empty-dirs.sh: 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
     +-			then
     +-				echo >&2 "$i does not exist" &&
     +-				exit 1
     +-			fi
     ++			test_path_exists "$i" || exit 1
     + 		done
     + 	)
     + '
      @@ t/t9146-git-svn-empty-dirs.sh: 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
     +-			then
     +-				echo >&2 "$i exists" &&
     +-				exit 1
     +-			fi
     ++			test_path_is_missing "$i" || exit 1
     + 		done
     + 	)
     + '
      @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'more emptiness' '
       
       test_expect_success 'git svn rebase creates empty directory' '
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs recreates emp
       		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
     +-			then
     +-				echo >&2 "$i does not exist" &&
     +-				exit 1
     +-			fi
     ++			test_path_exists "$i" || exit 1
     + 		done
     + 	)
     + '
      @@ t/t9146-git-svn-empty-dirs.sh: 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
     +-			then
     +-				echo >&2 "$i does not exist" &&
     +-				exit 1
     +-			fi
     ++			test_path_exists "$i" || exit 1
       		done &&
       
      -		if test -d "! !"
     -+		if test_path_is_dir "! !"
     - 		then
     - 			echo >&2 "$i should not exist" &&
     - 			exit 1
     - 		fi &&
     +-		then
     +-			echo >&2 "$i should not exist" &&
     +-			exit 1
     +-		fi &&
     ++		test_path_is_missing "! !" || exit 1 &&
       
       		git svn mkdirs -r8 &&
      -		if ! test -d "! !"
     -+		if test_path_is_missing "! !"
     - 		then
     - 			echo >&2 "$i not exist" &&
     - 			exit 1
     +-		then
     +-			echo >&2 "$i not exist" &&
     +-			exit 1
     +-		fi
     ++		test_path_exists "! !" || exit 1
     + 	)
     + '
     + 
      @@ t/t9146-git-svn-empty-dirs.sh: 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
     +-			then
     +-				echo >&2 "$i does not exist" &&
     +-				exit 1
     +-			fi
     ++			test_path_exists "$i" || exit 1
     + 		done
     + 	)
     + '
     +@@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'remove a top-level directory from svn' '
     + 
     + test_expect_success 'removed top-level directory does not exist' '
     + 	git svn clone "$svnrepo" removed &&
     +-	test ! -e removed/d
     ++	test_path_is_missing removed/d
     + 
     + '
     + unhandled=.git/svn/refs/remotes/git-svn/unhandled.log
      @@ t/t9146-git-svn-empty-dirs.sh: 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 &&
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn gc-ed files work' '
       			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
     +-				then
     +-					echo >&2 "$i does not exist" &&
     +-					exit 1
     +-				fi
     ++				test_path_exists "$i" || exit 1
     + 			done
     + 		fi
     + 	)

 t/t9146-git-svn-empty-dirs.sh | 56 ++++++++---------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)

"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:

Correct.

The former could result in misconversion, if the intention of the test was "we cannot have directory here; a regular file is OK", so we have to be a bit more careful than mechanical conversion.

OK.

OK.

We were saying that we are OK if "$i" existed as a file (not a directory), but now we complain regardless of what "$i" is. Is that closer to what the test originally wanted to do? Just checking.

Ditto; are we sure the intention of the original is that nothing should be at "$i" (instead of "as long as it is not a directory, we are OK")? Just checking.

The same comment applies to all conversions to test_path_exists and test_path_is_missing where the original was not "test -e" or "! test -e". The other ones, like the change from "test -f" to "test_path_is_file", looked all correct.

Thanks.

From: Chandra Pratap <chandrapratap3519@gmail.com>

The helper functions test_path_is_* provide better debugging information than test -d/-e/-f.

Replace "if ! test -d then <error message>" and "test -d" with "test_path_is_dir" at places where we check for existent directories.

Replace "test -f" with "test_path_is_file" at places where we check for existent files.

Replace "test ! -e" and "if test -d then <error message>" with "test_path_is_missing" where we check for non-existent directories.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
    t9146: replace test -d/-f with appropriate test_path_is_* function
    
    I chose to retain "test_path_is_misssing" as a replacement for "if test
    -d then " because we initialize the repository at the start of the test
    with:
    
    for i in a b c d d/e d/e/f "weird file name" do svn_cmd mkdir -m "mkdir
    $i" "$svnrepo"/"$i" || return 1 done
    
    and then check for the existence of these directories in the following
    tests. I think this reproduces the behavior of the original tests close
    enough.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1661%2FChand-ra%2Ftestfix-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1661/Chand-ra/testfix-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1661

Range-diff vs v2:

 1:  5734b9edd61 ! 1:  5024389e7a9 t9146: replace test -d/-e/-f with appropriate test_path_is_* function
     @@ Commit message
          The helper functions test_path_is_* provide better debugging
          information than test -d/-e/-f.
      
     -    Replace "if ! test -d then <error message>" with "test_path_exists"
     -    and "test -d" with "test_path_is_dir" at places where we check for
     -    existent directories.
     +    Replace "if ! test -d then <error message>" and "test -d" with
     +    "test_path_is_dir" at places where we check for existent directories.
      
          Replace "test -f" with "test_path_is_file" at places where we check
          for existent files.
      
     -    Replace "test ! -e" with "test_path_is_missing" where we check for
     -    non-existent directories.
     +    Replace "test ! -e" and "if test -d then <error message>" with
     +    "test_path_is_missing" where we check for non-existent directories.
      
          Helped-by: Eric Sunshine <sunshine@sunshineco.com>
          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories exist' '
      -				echo >&2 "$i does not exist" &&
      -				exit 1
      -			fi
     -+			test_path_exists "$i" || exit 1
     ++			test_path_is_dir "$i" || exit 1
       		done
       	)
       '
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs recreates emp
      -				echo >&2 "$i does not exist" &&
      -				exit 1
      -			fi
     -+			test_path_exists "$i" || exit 1
     ++			test_path_is_dir "$i" || exit 1
       		done
       	)
       '
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs -r works' '
      -				echo >&2 "$i does not exist" &&
      -				exit 1
      -			fi
     -+			test_path_exists "$i" || exit 1
     ++			test_path_is_dir "$i" || exit 1
       		done &&
       
      -		if test -d "! !"
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn mkdirs -r works' '
      -			echo >&2 "$i not exist" &&
      -			exit 1
      -		fi
     -+		test_path_exists "! !" || exit 1
     ++		test_path_is_dir "! !" || exit 1
       	)
       '
       
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'empty directories in trunk e
      -				echo >&2 "$i does not exist" &&
      -				exit 1
      -			fi
     -+			test_path_exists "$i" || exit 1
     ++			test_path_is_dir "$i" || exit 1
       		done
       	)
       '
     @@ t/t9146-git-svn-empty-dirs.sh: test_expect_success 'git svn gc-ed files work' '
      -					echo >&2 "$i does not exist" &&
      -					exit 1
      -				fi
     -+				test_path_exists "$i" || exit 1
     ++				test_path_is_dir "$i" || exit 1
       			done
       		fi
       	)

 t/t9146-git-svn-empty-dirs.sh | 56 ++++++++---------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)