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

[Junio C Hamano <gitster@pobox.com>]

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

> 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 ...

> 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.

> 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.

>  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.