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

shejialuo <shejialuo@gmail.com> writes:

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:

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?

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.