Re: [GSoC] [PATCH] t1011: replace test -f with test_path_is_file

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

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> Use test_path_is_file() instead of 'test -f' for better debugging
> information.
> ---

missing Sign-off.

>  	test_cmp expected.swt result &&
> -	! test -f init.t &&
> -	! test -f sub/added
> +	! test_path_is_file init.t &&
> +	! test_path_is_file sub/added
>  '

Given the definition of the helper function, i.e.

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

the new test will _complain_ "init.t doesn't exist" when we have
successfully run the test, while it will be _silent_ when init.t
that _should_ not exist is there.

Which is the complete opposite of the spirit of why we want to use
the helper when we expect the path "$1" to exist, i.e. loudly fail
when our expectation is _not_ met.

$ git grep '! test_path_is' t/

shows that we already have such a misuse of test_path_is_dir in one
place, but luckily we do not have any for test_path_is_file or other
similar helpers.  test_path_is_hidden is sort-of OK as that is not
about verbosity.

In these two test, we do not expect init.t or sub/added to _exist_
at all.  It's not like we are happy if we see init.d exist as a
directory (which is not a file).  test_path_is_missing is probably
the right helper to use.

It is not very plausible that we'd want to assert that existence of
a path as a file the only bad condition (i.e. we are happy if the
path did not exist or it is a directory, symlink, or a socket), so I
think the simple 

	Never use '! test_path_is_file'; test_path_is_missing may be
	what you are looking for.

is a good enough rule.

If not, we could allow the caller to write such a convoluted "only
existence of a path as a file is unacceptable and everything else is
good" assertion as

    test_path_is_file ! init.d

with something like

        test_path_is_file () {
		expecting_file=true
		if test "$1" = "!"
		then
			expecting_file=false
			shift
		fi
                test "$#" -ne 1 && BUG "1 param"
                if test -f "$1"
                then
                	$expecting_file || echo "File $1 exists"
                        $expecting_file
		else
			$expecting_file && echo "File $1 doesn't exist"
                        ! $expecting_file
                fi
        }

but I do not think we want to go that way.