Do test-path_is_{file,dir,exists} make sense anymore with -x?

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Feb 26 2019, Duy Nguyen wrote:

> On Tue, Feb 26, 2019 at 8:42 PM Rohit Ashiwal via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
>>
>> t3600-rm.sh: Previously we were using `test -(d|f)`
>> to verify the presencee of a directory/file, but we
>> already have helper functions, viz, test_path_is_dir
>> and test_path_is_file with same functionality. This
>
> It's not just the same (no point replacing then). It's better. When
> test_path_is_xxx fails, you get an error message. If "test -xxx"
> fails, you get a failed test with no clue what caused it.

I swear I'm not just on a mission to ruin everyone's GSOC projects. This
patch definitely looks good, and given that we have this / document it
makes sense.

However. I wonder in general if we've re-visited the utility of these
wrappers and maybe other similar wrappers after -x was added.

Back when this was added in 2caf20c52b ("test-lib: user-friendly
alternatives to test [-d|-f|-e]", 2010-08-10) we didn't have -x. So we'd
at best fail like this:

    $ ./t0001-init.sh  -v -i
    Initialized empty Git repository in /home/avar/g/git/t/trash directory.t0001-init/.git/
    expecting success:
            test -d .git &&
            test -f doesnotexist &&
            test -f .git/config

    not ok 1 - check files
    #
    #               test -d .git &&
    #               test -f doesnotexist &&
    #               test -f .git/config
    #

At that point this was a definite improvement:

    expecting success:
            test_path_is_dir .git &&
            test_path_is_file doesnotexist &&
            test_path_is_file .git/config

    File doesnotexist doesn't exist.
    not ok 1 - check files

But 4 years after this was added in a136f6d8ff ("test-lib.sh: support -x
option for shell-tracing", 2014-10-10) we got -x, and then with "-i -v -x":

    expecting success:
            test_path_is_dir .git &&
            test_path_is_file doesnotexist &&
            test_path_is_file .git/config

    + test_path_is_dir .git
    + test -d .git
    + test_path_is_file doesnotexist
    + test -f doesnotexist
    + echo File doesnotexist doesn't exist.
    File doesnotexist doesn't exist.
    + false
    error: last command exited with $?=1
    not ok 1 - check files

But by just using "test -d/-e": the much shorter:

    + test -d .git
    + test -f doesnotexist
    error: last command exited with $?=1
    not ok 1 - check files

So I wonder if these days we shouldn't do this the other way around and
get rid of these. Every test_* wrapper we add adds a bit of cognitive
overload when you have to remember Git's specific shellscript dialect.

And at least to me whenever I have a test failure the first thing I do
is try with -x (if I wasn't already using it). Under that the wrapper
output is more verbose and no more helpful. It's immediately clear
what's going on with:

    + test -f doesnotexist
    error: last command exited with $?=1

Whereas:

    + test -f doesnotexist
    + echo File doesnotexist doesn't exist.
    File doesnotexist doesn't exist.
    + false
    error: last command exited with $?=1

Gives me the same thing, but I have to read 5 lines instead of 2 that
ultimately don't tell me any more (and a bit of "huh, 'false' returned
1? Of course! Oh! It's faking things up and it's the 'echo' that
matters...").

Looking over test-lib-functions.sh this patch would do it. I couldn't
spot any other functions redundant to -x:

    diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
    index 80402a428f..b3a95b4968 100644
    --- a/t/test-lib-functions.sh
    +++ b/t/test-lib-functions.sh
    @@ -555,33 +555,6 @@ test_external_without_stderr () {
     	fi
     }

    -# debugging-friendly alternatives to "test [-f|-d|-e]"
    -# The commands test the existence or non-existence of $1. $2 can be
    -# given to provide a more precise diagnosis.
    -test_path_is_file () {
    -	if ! test -f "$1"
    -	then
    -		echo "File $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
    -test_path_is_dir () {
    -	if ! test -d "$1"
    -	then
    -		echo "Directory $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
    -test_path_exists () {
    -	if ! test -e "$1"
    -	then
    -		echo "Path $1 doesn't exist. $2"
    -		false
    -	fi
    -}
    -
     # Check if the directory exists and is empty as expected, barf otherwise.
     test_dir_is_empty () {
     	test_path_is_dir "$1" &&
    @@ -593,19 +566,6 @@ test_dir_is_empty () {
     	fi
     }

    -test_path_is_missing () {
    -	if test -e "$1"
    -	then
    -		echo "Path exists:"
    -		ls -ld "$1"
    -		if test $# -ge 1
    -		then
    -			echo "$*"
    -		fi
    -		false
    -	fi
    -}
    -
     # test_line_count checks that a file has the number of lines it
     # ought to. For example:
     #
    @@ -849,6 +809,9 @@ verbose () {
     # otherwise.

     test_must_be_empty () {
    +	# We don't want to remove this as noted in ec10b018e7 ("tests:
    +	# use 'test_must_be_empty' instead of '! test -s'",
    +	# 2018-08-19)
     	test_path_is_file "$1" &&
     	if test -s "$1"
     	then