Re: [PATCH 1/1] t9117: prefer test_path_* helper functions

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

shejialuo <shejialuo@gmail.com> writes:

>  test_expect_success 'basic clone' '
> -	test ! -d trunk &&
> +	! test_path_is_dir trunk &&

This is not quite right.  Step back and think why we are trying to
use the test_path_* helpers instead of "test [!] -d".  What are the
differences between them?

The answer is that, unlike "test [!] -d dir" that is silent whether
"dir" exists or missing, "test_path_is_dir dir" is *not* always
silent.  It gives useful messages as necessary.  When does it do so?

Here is the definition, from t/test-lib-functions.sh around line
930:

        test_path_is_dir () {
                test "$#" -ne 1 && BUG "1 param"
                if ! test -d "$1"
                then
                        echo "Directory $1 doesn't exist"
                        false
                fi
        }

It succeeds silently when "test -d dir" is true, but it complains
loudly when "test -d dir" does not hold.  You will be told that the
test is unhappy because "dir" does not exist.  That would be easier
to debug than one step among many in &&-chain silently fails.

Now, let's look at the original you rewrote again:

> -	test ! -d trunk &&

It says "it is a failure if 'trunk' exists as a directory".  If
'trunk' does not exist, it is a very happy state for us.  So instead
of silently failing when 'trunk' exists as a directory, you would
want to improve it so that you will get a complaint in such a case,
saying "trunk should *not* exist but it does".

Did you succeed to do so with this rewrite?

> +	! test_path_is_dir trunk &&

The helper "test_path_is_dir" is called with "trunk".  As we saw, we
will see complaint when "trunk" does *NOT* exist.  When "trunk" does
exist, it will be silent and "test_path_is_dir" will return a success,
which will be inverted with "!" to make it a failure, causing &&-chain
to fail.

So the exit status is not wrong, but it issues a complaint under the
wrong condition.  That is not an improvement.

Let's step back one more time.  Is the original test happy when
"trunk" existed as a regular file?  "test ! -d trunk" says so, but
should it really be?  Think.

I suspect that the test is not happy as long as 'trunk' exists,
whether it is a directory or a regular file or a symbolic link.
IOW, it says "I am unhappy if 'trunk' is a directory", but what it
really meant to say was "I am unhappy if there is anything at the
path 'trunk'".  IOW, "test ! -e trunk" would be what it really
meant, no?

So the correct rewrite for it would rather be something like

	test_path_is_missing trunk &&

instead.  This will fail if anything is at path 'trunk', with an
error message saying there shouldn't be anything but there is.

In a peculiar case, which I do not think this one is, a test may
legitimately accept "path" to either (1) exist as long as it is not
a directory, or (2) be missing, as success.  In such a case, the
original construct '! test -d path" (or "test ! -d path") would be
appropriate.

But I do not think we have a suitable wrapper to express such a
case, i.e. we do not have a helper like this.

	test_path_is_not_dir () {
		if test -d "$1"
		then
			echo "$1 is a directory but it should not be"
			false
		fi
	}

If such a use case were common, we might even do this:

	# "test_path_is_dir <dir>" expects <dir> to be a directory.
	# "test_path_is_dir ! <dir>"  expects <dir> not to be a
	# directory.
	# In either case, complain only when the expectation is not met.
	test_path_is_dir () {
		if test "$1" = "!"
		then
			shift
                        if test -d "$1"
			then
				echo "$1 is a directory but it should not be"
				return 1
			fi
		else
			if test ! -d "$1"
			then
				echo "$1 is not a directory"
				return 1
			fi
		fi
		true
	}

but "we are happy even if path exists as long as it is not a
directory" is a very uncommon thing we want to say in our tests, so
that is why we do not have such a helper function.

HTH.