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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2023, 18:53 UTC
Message-ID: <xmqqy1o8xuis.fsf@gitster.g>
In-Reply-To: <20230307065813.77059-2-cheskaqiqi@gmail.com>

Shuqi Liang <cheskaqiqi@gmail.com> writes:

Show 14 quoted lines

> 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.

Show 5 quoted lines

> +	#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.