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

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

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..9382428352 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,42 @@ 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

(Documentation/CodingGuidelines)

 - Redirection operators should be written with space before, but no
   space after them.  In other words, write 'echo test >"$file"'
   instead of 'echo test> $file' or 'echo test > $file'.  Note that
   even though it is not required by POSIX to double-quote the
   redirection target in a variable (as shown above), our code does so
   because some versions of bash issue a warning without the quotes.

> +	#add file to the index but outside of cone

Can you have a SP after "#" here to make it more readable?

> +	run_on_sparse mkdir newdirectory &&
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git add --sparse newdirectory/testfile &&

We create a new directory that is outside the cone, with or without
using the sparse-index feature.  We know we are violating the cone,
and have to override the safety with the "--sparse" option.  OK.

What output do we expect out of "git add" to match in the two cases?

> +	#file present on-disk without modifications
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&

As "diff-files" is about comparing between the index and the working
tree, the new path should not appear in the output when the sparse
checkout feature with or without the sparse-index feature is NOT in
use.  Does the picture get different when we are sparse?  IOW, would
we notice that we now have newdirectory/testfile that is supposed to
be missing in the index and show that in the output?

> +	test_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile &&

What does HEAD:testfile refer to in this test?  This expects "diff-files"
invocation to fail, and perhaps in your test it failed in both test
repositories the same way, but are they failing for the right reason?

In a non-sparse repository whose HEAD commit does not have
'testfile' (e.g. "git" source tree), I get

    $ git diff-files --find-object=HEAD:testfile
    error: unable to resolve 'HEAD:testfile'

without sparse checkout or sparse index.  It is unclear what value
we get out of having this test here.

> +	#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_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile

Ditto.

> +'
> +
>  test_done

Thanks.