Subject: Re: [PATCH 2/2] config: respect includes in protected config

From: Junio C Hamano <[email-masked]>
Date: Wed, 12 Oct 2022 13:48:13 -0700

"Glen Choo via GitGitGadget" <[email-masked]> writes:

Not commenting on the code yet as I am in the middle of today's
integration run, but as I notice a bad pattern being followed, let
me comment before it spreads too widely.

The "add failing test first and then fix the code with flipping the
test to success" is very much unwelcome.  For whoever gets curious
enough (me included when accepting posted patch), it is easy to
revert only the part of the commit outside t/ tentatively to see how
the original code breaks.  Keeping the fix and protection of the fix
together will avoid mistakes.  The post context of the hunk that
changes test_expect_failure to test_expect_success does not cover
the test script, thereby hiding the body of the test that changes
behaviour while reviewing the patch text, which is another downside.

> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 720d6cdd60b..dc3496897ab 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -71,7 +71,7 @@ test_expect_success 'safe.directory=*, but is reset' '
>  	expect_rejected_dir
>  '
>  
> -test_expect_failure 'safe.directory in included file' '
> +test_expect_success 'safe.directory in included file' '
>  	cat >gitconfig-include <<-EOF &&
>  	[safe]
>  		directory = "$(pwd)"
> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index aa6a6a8c3fd..fa33839704b 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -51,7 +51,7 @@ test_expect_success 'safe.bareRepository on the command line' '
>  		-c safe.bareRepository=all
>  '
>  
> -test_expect_failure 'safe.bareRepository in included file' '
> +test_expect_success 'safe.bareRepository in included file' '
>  	cat >gitconfig-include <<-EOF &&
>  	[safe]
>  		bareRepository = explicit