Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

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

Tim Henigan <tim.henigan@gmail.com> writes:

> When running 'git diff --quiet <file1> <file2>', if file1 or file2
> is outside the repository, it will exit(0) even if the files differ.
> It should exit(1) when they differ.
>
> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
> ---
>
>
> v3 improves the test coverage to include variations of 'diff --quiet'
> where one or both of the files are outside the repository. Tests for
> '--ignore-space-at-eol' and '--ignore-all-space' are included as well.
>
>
>  diff-no-index.c       |  3 +-
>  t/t4035-diff-quiet.sh | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index f0b0010..b935d2a 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,
>  	 * The return code for --no-index imitates diff(1):
>  	 * 0 = no changes, 1 = changes, else error
>  	 */
> -	exit(revs->diffopt.found_changes);
> +	int result = diff_result_code(&revs->diffopt, 0);
> +	exit(result);
>  }

Decl-after-stmt.

> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
> index cdb9202..33d8980 100755
> --- a/t/t4035-diff-quiet.sh
> +++ b/t/t4035-diff-quiet.sh
> @@ -10,7 +10,22 @@ test_expect_success 'setup' '
>  	git commit -m first &&
>  	echo 2 >b &&
>  	git add . &&
> -	git commit -a -m second
> +	git commit -a -m second &&
> +	mkdir -p test-outside/repo && (
> +		cd test-outside/repo &&
> +		git init &&
> +		echo "1 1" > a &&

Please drop extra SP between ">" and "a".

> +		git add . &&
> +		git commit -m 1
> +	) &&
> +	mkdir -p test-outside/no-repo && (
> +		cd test-outside/no-repo &&
> +		echo "1 1" >a &&
> +		echo "1 1" >matching-file &&
> +		echo "1 1 " >trailing-space &&
> +		echo "1   1" >extra-space &&
> +		echo "2" >never-match
> +	)

The inspiration of using CEILING comes from the existing t7810-grep
test, and I would have preferred if you used the same non/git for a
non-git repository for easier greppability ("git grep CEIL t/" to
notice the use of the technique and then "git grep non/git t/" to
verify, for example).

>  '
>  
>  test_expect_success 'git diff-tree HEAD^ HEAD' '
> @@ -77,4 +92,66 @@ test_expect_success 'git diff-index --cached HEAD' '
>  	}
>  '
>  
> +test_expect_success 'git diff, one file outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&

Do you even need these two lines for this test?  your test runs
inside test-outside/repo that _is_ a git repository, and that
repository knows that ../no-repo is not part of it already.

> +		cd test-outside/repo &&
> +		test_expect_code 0 git diff --quiet a "$TRASH_DIRECTORY/test-outside/no-repo/matching-file" &&
> +		test_expect_code 1 git diff --quiet a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space"
> +	)
> +'
> +
> +test_expect_success 'git diff, both files outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd test-outside/no-repo &&
> +		test_expect_code 0 git diff --quiet a matching-file &&
> +		test_expect_code 1 git diff --quiet a extra-space

This one does need the ceiling to prevent git from finding the trash
directory.

> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-space-at-eol, one file outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&

This one does not, I think (please correct me; I am not being very careful).

> +		cd test-outside/repo &&
> +		test_expect_code 0 git diff --quiet --ignore-space-at-eol a "$TRASH_DIRECTORY/test-outside/no-repo/trailing-space" &&
> +		test_expect_code 1 git diff --quiet --ignore-space-at-eol a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space"
> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-space-at-eol, both files outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd test-outside/no-repo &&
> +		test_expect_code 0 git diff --quiet --ignore-space-at-eol a trailing-space &&
> +		test_expect_code 1 git diff --quiet --ignore-space-at-eol a extra-space
> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-all-space, one file outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&

This one does not, I think (please correct me; I am not being very careful).

> +		cd test-outside/repo &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/trailing-space" &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space" &&
> +		test_expect_code 1 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/never-match"
> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-all-space, both files outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd test-outside/no-repo &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a trailing-space &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a extra-space &&
> +		test_expect_code 1 git diff --quiet --ignore-all-space a never-match
> +	)
> +'
> +
>  test_done