Subject: Re: [PATCH v3 1/2] t1092: add tests for `git diff-files`

From: Junio C Hamano <[email-masked]>
Date: Wed, 08 Mar 2023 19:00:12 -0800

Shuqi Liang <[email-masked]> writes:

> +	! test_file_not_empty sparse-checkout-out &&

If you looked at existing uses of this test helper, you probably
wouldn't have written this.

    $ git grep -e test_file_not_empty t/t[0-9]\*.sh | wc -l
    34
    $ git grep -e '! test_file_not_empty' t/t[0-9]\*.sh | wc -l
    0

This is because test_file_not_empty is designed to fail loudly with
a complaint when the given file is empty.  Its implementation reads
like so:

        # Check if the file exists and has a size greater than zero
        test_file_not_empty () {
                test "$#" = 2 && BUG "2 param"
                if ! test -s "$1"
                then
                        echo "'$1' is not a non-empty file."
                        false
                fi
        }

In the successful case in your test, you expect the file to be empty
(I didn't check if it should be empty or not---I am just taking your
word for it).  It means that the "! test_file_not_empty" is expected
to keep complaining that it is NOT a non-empty file.

Not very nice, no?

Perhaps test_must_be_empty is what you wanted to use.

> +	! test_file_not_empty sparse-index-out &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	! test_file_not_empty sparse-checkout-out &&
> +	! test_file_not_empty sparse-index-out &&
> +
> +	# file present on-disk with modifications
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_file_not_empty sparse-checkout-out

Now, are we happy if the file is not empty and has any garbage in
it?  Don't we know what the list of different paths are and what the
common output between the two should look like?

In general "as long as it is not empty, any garbage is fine" is a
poor primitive to use in tests, unless (1) we are testing output
that is deliberately designed to be unstable, or (2) we know the
program that produces output will always show an empty result when
it fails in any way.

> +'
> +
>  test_done

Thanks.