Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2025, 21:14 UTC
Message-ID: <xmqq5xk611o3.fsf@gitster.g>
In-Reply-To: <pull.1923.git.git.1742329571265.gitgitgadget@gmail.com>

"Sampriyo Guin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Sampriyo Guin <sampriyoguin@gmail.com>
>
> test -(e|f|d) does not provide a proper error message when hit
> test failures. So test_path_exists, test_path_is_dir,
> test_parh_is_file used.

Correct but ...

Show 13 quoted lines

> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index 2b60317758c..6a8fe69c089 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -156,7 +156,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
>  test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
>  	shellpath=$(git var GIT_SHELL_PATH) &&
>  	case "$shellpath" in
> -	[A-Z]:/*/sh.exe) test -f "$shellpath";;
> +	[A-Z]:/*/sh.exe) test_path_is_file "$shellpath";;
>  	*) return 1;;
>  	esac
>  '

... this one is iffy. How well does it mesh with the "return 1" case in the same case/esac? I do not use Windows, but if GIT_SHELL_PATH set by that system is not in a form that the platform expects (i.e. a drive letter and then path to a file "sh.exe"), it is just as grave a problem worth reporting as the path given is not a regular file, yet "return 1" case does not give any specific error message (instead it just lets the test fail), so it feels a bit uneven to make the "test -f" failure alone louder than it currently is.

Show 11 quoted lines

> diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
> index aa7f6ecd813..f76471a3375 100755
> --- a/t/t0601-reffiles-pack-refs.sh
> +++ b/t/t0601-reffiles-pack-refs.sh
> @@ -78,13 +78,13 @@ test_expect_success 'see if a branch still exists after git pack-refs --prune' '
>  test_expect_success 'see if git pack-refs --prune remove ref files' '
>  	git branch f &&
>  	git pack-refs --all --prune &&
> -	! test -f .git/refs/heads/f
> +	! test_path_is_file .git/refs/heads/f
>  '

This conversion is wrong. We are expecting that the file does *not* exist, and your complaint and the reason why you are rewriting this line is that "! test -f" does not give loud message when that file does exist. What does "! test_path_is_file foo" do when 'foo' exists?

You can find the implementation of test_path_is_file with "git grep" and see for yourself.

    # debugging-friendly alternatives to "test [-f|-d|-e]"
    # The commands test the existence or non-existence of $1
    test_path_is_file () {
            test "$#" -ne 1 && BUG "1 param"
            if ! test -f "$1"
            then
                    echo "File $1 doesn't exist"
                    false
            fi
    }

The test wants to make sure that 'f' is not a file. So you want to see a loud complaint when 'f' exists as a file. Does it do so when you say

	! test_path_is_file .git/refs/heads/f

in this test? No, it will not enter the "then" block and silently succeed, and that return status is negated by that "!", so the test will notice that the expectation is not met, but you didn't achieve your goal of making it louder when it fail. Worse, if the file is not there, as the test expects when Git is working correctly, your

	! test_path_is_file .git/refs/heads/f

will enter the "then" block, complain that the file does not exist, returns a failure, and your "!" will turn it into a success. The test passes, but the user is given an error message when the test is run with "-v" option.

Show 6 quoted lines

>  test_expect_success 'see if git pack-refs --prune removes empty dirs' '
>  	git branch r/s/t &&
>  	git pack-refs --all --prune &&
> -	! test -e .git/refs/heads/r
> +	! test_path_exists .git/refs/heads/r
>  '

Ditto.

I'll stop here. I think all "! test_path_foo" changes in this patch are wrong.

Unlike "test_grep" that lets you write "test_grep ! foo bar" to make sure that file 'bar' has no 'foo' in it (and complains loudly if 'foo' appears in 'bar'), test_path_foo family of helper functions do not let you write "test_path_exists ! no-such-file" unfortunately. So my recommendation for a microproject sample is to just drop these negations from the changes and stop there.

If this were a patch for real, it would make sense to give a preliminary update to test_path_foo helpers to allow them to take negated "test_path_exists ! no-such-file" form, and then convert the negative form as well, but I think that would be a bit more than what a typical microproject would take.

Thanks.