Re: [PATCH 11/14] test: completion: tests for __gitcomp regression

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2019, 17:38 UTC
Message-ID: <xmqqo92b10it.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190621223107.8022-12-felipe.contreras@gmail.com>

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

> +offgit ()
> +{

Style: opening brace comes on the same line, like

	offgit () {

Show 6 quoted lines

> +	GIT_CEILING_DIRECTORIES="$ROOT" &&
> +	export GIT_CEILING_DIRECTORIES &&
> +	test_when_finished "ROOT='$ROOT'; cd '$TRASH_DIRECTORY'; unset GIT_CEILING_DIRECTORIES" &&
> +	ROOT="$ROOT"/non-repo &&
> +	cd "$ROOT"
> +}

All of these means that anytime some test uses offgit outside a subshell, all the subsequent test will start from outside a repository, with nonstandard GIT_CEILING_DIRECTORIES settings.

The test should avoid using this outside a subshell when able (and if it apparently cannot easily, we should try to find a way).

Show 18 quoted lines

> @@ -358,10 +367,8 @@ test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symli
>  '
>  
>  test_expect_success '__git_find_repo_path - not a git repository' '
> +	offgit &&
>  	(
> -		cd non-repo &&
> -		GIT_CEILING_DIRECTORIES="$ROOT" &&
> -		export GIT_CEILING_DIRECTORIES &&
>  		test_must_fail __git_find_repo_path &&
>  		printf "$__git_repo_path" >"$actual"
>  	) &&
> @@ -1388,6 +1395,7 @@ test_expect_success '__git_pretty_aliases' '
>  '
>  
>  test_expect_success 'basic' '
> +	offgit &&
>  	run_completion "git " &&

Adding "offgit" everywhere like this patch does means that this "basic" test, for example, no longer is performed in the condition we have been testing the completion script for, doesn't it? If so, the patch is trading test coverage outside repo with coverage inside repo, which is not a very good tradeoff.