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

Commit Message

Shuqi Liang March 9, 2023, 1:33 a.m. UTC

Before integrating the 'git diff-files' builtin
with the sparse index feature, add tests to
t1092-sparse-checkout-compatibility.sh to ensure it currently works
with sparse-checkout and will still work with sparse index
after that integration.

When adding tests against a sparse-checkout
definition, we test two modes: all changes are
within the sparse-checkout cone and some changes are outside
the sparse-checkout cone.

In order to have staged changes outside of
the sparse-checkout cone, create a 'newdirectory/testfile' and
add it to the index, while leaving it outside of
the sparse-checkout definition.Test 'newdirectory/testfile'
being present on-disk without modifications, then change content inside
'newdirectory/testfile' in order to test 'newdirectory/testfile'
being present on-disk with modifications.

Signed-off-by: Shuqi Liang <[e-mail]>
---
 t/t1092-sparse-checkout-compatibility.sh | 40 ++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Comments

Junio C Hamano March 9, 2023, 3 a.m. UTC | #1

Shuqi Liang <[e-mail]> 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.

Patch

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..bdf3cf25d4 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,44 @@  test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'diff-files with pathspec inside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+
+	test_all_match git diff-files  &&
+	test_all_match git diff-files deep/a 
+'
+
+test_expect_success 'diff-files with pathspec outside sparse definition' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# add file to the index but outside of cone
+	run_on_sparse mkdir newdirectory &&
+	run_on_sparse ../edit-contents newdirectory/testfile &&
+	test_sparse_match git add --sparse newdirectory/testfile &&
+
+	# file present on-disk without modifications
+	test_sparse_match git diff-files &&
+	! test_file_not_empty sparse-checkout-out &&
+	! 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
+'
+
 test_done