Re: [PATCH v2 2/2] t: use test_config whenever possible

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2021, 05:02 UTC
Message-ID: <xmqqbl9b6zla.fsf@gitster.g>
In-Reply-To: <20210515152721.885728-3-firminmartin24@gmail.com>

Firmin Martin <firminmartin24@gmail.com> writes:

"whenever possible" in the subject is not wrong per-se but because we do not want to accept a patch that uses it where it is impossible to be used anyway, it does not mean all that much.

Use of test_config to replace #1 below is a correctness improvement, and use of test_config to replace #2 below is a readability thing.

    t: use more test_config for correctness and readability

perhaps?

Show 5 quoted lines

> Replace patterns of the form
> 1.
>         git config <config-option> <value> &&
>         ...
>         git config --unset <config-option> &&

Adding "because this form fails to unset the config if an earlier step fails." would be helpful to the readers to clarify that this is a correctness "fix".

Show 8 quoted lines

> 2.
>         test_when_finished "git config --unset <config-option>" &&
>         git config <config-option> <value> &&
>         ...
>
> to the concise
>
>         test_config <config-option> <value>

Nice.

> In t5526, two tests have been further simplified as the output file is
> written before "git config --global --unset".

Show 5 quoted lines

> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index ed11569d8d..ff18263171 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> ...

@@ -429,11 +429,7 @@ test_expect_success "'fetch.recurseSu
>  	(
>  		cd downstream &&
>  		git config fetch.recurseSubmodules on-demand &&
> -		git fetch >../actual.out 2>../actual.err
> -	) &&
> -	git config --global --unset fetch.recurseSubmodules &&
> -	(
> -		cd downstream &&
> +		git fetch >../actual.out 2>../actual.err &&
>  		git config --unset fetch.recurseSubmodules
>  	) &&
>  	test_must_be_empty actual.out &&

The original seems to be making sure that the config set in
"downstream" repository is used successfully before it is unset from
the global configuration and then the same fetch in the downstream
will work even when the configuration is removed from the global
configuration.  The squashing of the two tests into one would change
what is being tested, wouldn't it?

Surely, the actual.out and actual.err in the first invocation *was*
overwritten without being looked at, but the exit status of that
fetch (i.e. fetch before global config gets unset) was still checked
in the original.  So I'd call this a "simplification"

>  	(
>  		cd downstream &&
>  		git config fetch.recurseSubmodules on-demand &&
> -		git fetch >../actual.out 2>../actual.err
> +		git fetch
> 	) &&
> 	git config --global --unset fetch.recurseSubmodules &&

I.e. these redirected output files were not used and the next fetch
will overwrite them.

The patch I am responding to claims that 

 - "make sure it succeeds with global set, and then without global
   set, make sure it still works" is not worth testing.

 - it is OK to replace it with a single "it succeeds without
   unsetting the global config".

It is not clear if either is true.  Same for the other hunk that
squashes two tests into one for this path.