Re: [PATCH v2 1/9] completion: add file completion tests

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 27, 2013, 21:24 UTC
Message-ID: <7vehdvu0og.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1367093407-8898-2-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 19 quoted lines

> The commit fea16b4 (git-completion.bash: add support for path
> completion) introduced quite a few changes, without the usual tests.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 6d9d141..385e1e4 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -347,4 +347,72 @@ test_expect_success 'send-email' '
>  	test_completion "git send-email ma" "master "
>  '
>  
> +test_expect_success 'complete files' '
> +	git init tmp && cd tmp &&
> +	test_when_finished "cd .. && rm -rf tmp" &&

Hmm, this construct is not very nice.

Later part of the code can be modified carelessly and chdir around even more, and "cd .." may end up at an unexpected place. Can't this be done in a way to make later reviews of patches that come after this change lands doable without looking at the whole 70 lines? For example, if the test ran the whole "these are done in a subdirectry" part, starting from the "cd tmp", inside a subshell, and "rm -rf tmp" is done in the original shell that ran "git init tmp" to create it, it would make it much harder to make mistakes for anybody who add new code to the body of the test to affect the clean-up part.

> +
> +	echo "expected" > .gitignore &&
> +	echo "out" >> .gitignore &&

Style: we tend to omit SP between redireciton operator and targets.

Show 62 quoted lines

> +	git add .gitignore &&
> +	test_completion "git commit " ".gitignore" &&
> +
> +	git commit -m ignore &&
> +
> +	touch new &&
> +	test_completion "git add " "new" &&
> +
> +	git add new &&
> +	git commit -a -m new &&
> +	test_completion "git add " "" &&
> +
> +	git mv new modified &&
> +	echo modify > modified &&
> +	test_completion "git add " "modified" &&
> +
> +	touch untracked &&
> +
> +	: TODO .gitignore should not be here &&
> +	test_completion "git rm " <<-\EOF &&
> +	.gitignore
> +	modified
> +	EOF
> +
> +	test_completion "git clean " "untracked" &&
> +
> +	: TODO .gitignore should not be here &&
> +	test_completion "git mv " <<-\EOF &&
> +	.gitignore
> +	modified
> +	EOF
> +
> +	mkdir dir &&
> +	touch dir/file-in-dir &&
> +	git add dir/file-in-dir &&
> +	git commit -m dir &&
> +
> +	mkdir untracked-dir &&
> +
> +	: TODO .gitignore should not be here &&
> +	test_completion "git mv modified " <<-\EOF &&
> +	.gitignore
> +	dir
> +	modified
> +	untracked
> +	untracked-dir
> +	EOF
> +
> +	test_completion "git commit " "modified" &&
> +
> +	: TODO .gitignore should not be here &&
> +	test_completion "git ls-files " <<-\EOF
> +	.gitignore
> +	dir
> +	modified
> +	EOF
> +
> +	touch momified &&
> +	test_completion "git add mom" "momified"
> +'
> +
>  test_done